Narrow BIO_read and BIO_write error values Instead of returning arbitrary negative values, always return -1, even if the callback returns an arbitrary value. Also BIO_write should return -1, not 0, on error. (BIO_read returns 0 to signal EOF, not error, so that one is left alone.) This is in preparation for BIO_read_ex and BIO_write_ex, which are unable to return such arbitrary values. Update-Note: BIO_read and BIO_write now each only have one error return. Test runs suggest no code is relying on the distinction between all the different return values for error. Fixed: 42290372 Change-Id: I3385df2f996444d15b2f6918318cae6c78bc7b3a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/96108 Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/bio/bio.cc b/crypto/bio/bio.cc index 9250be0..793389c 100644 --- a/crypto/bio/bio.cc +++ b/crypto/bio/bio.cc
@@ -80,11 +80,11 @@ auto *impl = FromOpaque(bio); if (impl == nullptr || impl->method->bread == nullptr) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD); - return -2; + return -1; } if (!impl->init) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNINITIALIZED); - return -2; + return -1; } if (len <= 0) { return 0; @@ -92,6 +92,9 @@ int ret = impl->method->bread(impl, reinterpret_cast<char *>(buf), len); if (ret > 0) { impl->num_read += ret; + } else if (ret < 0) { + // In preparation for |BIO_read_ex|, canonicalize error returns to -1. + ret = -1; } return ret; } @@ -100,11 +103,11 @@ auto *impl = FromOpaque(bio); if (impl == nullptr || impl->method->bgets == nullptr) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD); - return -2; + return -1; } if (!impl->init) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNINITIALIZED); - return -2; + return -1; } if (len <= 0) { return 0; @@ -120,11 +123,11 @@ auto *impl = FromOpaque(bio); if (impl == nullptr || impl->method->bwrite == nullptr) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD); - return -2; + return -1; } if (!impl->init) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNINITIALIZED); - return -2; + return -1; } if (inl <= 0) { return 0; @@ -132,6 +135,9 @@ int ret = impl->method->bwrite(impl, reinterpret_cast<const char *>(in), inl); if (ret > 0) { impl->num_write += ret; + } else { + // In preparation for |BIO_write_ex|, canonicalize error returns to -1. + ret = -1; } return ret; } @@ -171,7 +177,7 @@ if (impl->method->ctrl == nullptr) { OPENSSL_PUT_ERROR(BIO, BIO_R_UNSUPPORTED_METHOD); - return -2; + return -1; } return impl->method->ctrl(impl, cmd, larg, parg);
diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index bca8463..faaa87f 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc
@@ -1123,9 +1123,7 @@ { UniquePtr<BIO> bio(BIO_new_file(temp.path().c_str(), "rb")); ASSERT_TRUE(bio); - // TODO(crbug.com/42290372): File BIOs currently return zero instead of -1 - // on write error. - EXPECT_EQ(BIO_write(bio.get(), "foo", 3), 0); + EXPECT_EQ(BIO_write(bio.get(), "foo", 3), -1); EXPECT_FALSE(BIO_should_retry(bio.get())); } @@ -1280,9 +1278,9 @@ // |bio1| no longer has the "init" flag set, so reads and writes will fail at // the BIO framework. EXPECT_FALSE(BIO_get_init(bio1.get())); - EXPECT_EQ(-2, BIO_write(bio1.get(), "12345", 5)); + EXPECT_EQ(-1, BIO_write(bio1.get(), "12345", 5)); EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_BIO, BIO_R_UNINITIALIZED)); - EXPECT_EQ(-2, BIO_read(bio1.get(), buf, sizeof(buf))); + EXPECT_EQ(-1, BIO_read(bio1.get(), buf, sizeof(buf))); EXPECT_TRUE(ErrorEquals(ERR_get_error(), ERR_LIB_BIO, BIO_R_UNINITIALIZED)); // Although in this disconnected state, |BIO_ctrl| works. |bio1| should @@ -1414,5 +1412,42 @@ {method2->first, method3->first, method4->first}); } +TEST(BIOTest, CanonicalizeErrors) { + // Make a custom BIO with a programmable return value. + UniquePtr<BIO_METHOD> method(BIO_meth_new(0, nullptr)); + ASSERT_TRUE(method); + BIO_meth_set_read(method.get(), [](BIO *bio, char *, int) -> int { + return *static_cast<int *>(BIO_get_data(bio)); + }); + BIO_meth_set_write(method.get(), [](BIO *bio, const char *, int) -> int { + return *static_cast<int *>(BIO_get_data(bio)); + }); + + int return_value = -1; + UniquePtr<BIO> bio(BIO_new(method.get())); + ASSERT_TRUE(bio); + BIO_set_data(bio.get(), &return_value); + BIO_set_init(bio.get(), 1); + + // Any return value < 0 from BIO_read is an error, which should be -1. + char buf[10]; + return_value = -2; + EXPECT_EQ(BIO_read(bio.get(), buf, sizeof(buf)), -1); + return_value = -1; + EXPECT_EQ(BIO_read(bio.get(), buf, sizeof(buf)), -1); + + // Zero is EOF and should be preserved. + return_value = 0; + EXPECT_EQ(BIO_read(bio.get(), buf, sizeof(buf)), 0); + + // Any return value <= 0 from BIO_write is an error, which should be -1. + return_value = -2; + EXPECT_EQ(BIO_write(bio.get(), "foo", 3), -1); + return_value = -1; + EXPECT_EQ(BIO_write(bio.get(), "foo", 3), -1); + return_value = 0; + EXPECT_EQ(BIO_write(bio.get(), "foo", 3), -1); +} + } // namespace BSSL_NAMESPACE_END
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index bb9f71f..59c89ca 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -10645,22 +10645,6 @@ EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); EXPECT_TRUE(write_failed); write_failed = false; - - // Cause |BIO_write| to fail with a return value of zero instead. - // |SSL_get_error| should not misinterpret this as a close_notify. - // - // This is not actually a correct implementation of |BIO_write|, but the rest - // of the code treats zero from |BIO_write| as an error, so ensure it does so - // correctly. Fixing https://crbug.com/boringssl/503 will make this case moot. - BIO_meth_set_write(method.get(), [](BIO *, const char *, int) -> int { - write_failed = true; - return 0; - }); - ret = SSL_write(client.get(), data, sizeof(data)); - EXPECT_EQ(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); - EXPECT_TRUE(write_failed); - write_failed = false; } // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving