PR feedback: add proper return error codes, more memory safety checks

include/openssl/ssl.h

@@ -5077,7 +5077,8 @@ OPENSSL_EXPORT int SSL_cutthrough_complete(const SSL *ssl);
 // SSL_num_renegotiations calls |SSL_total_renegotiations|.
 OPENSSL_EXPORT int SSL_num_renegotiations(const SSL *ssl);
 
-// SSL_CTX_get_read_ahead returns 1 if read ahead is enabled, and 0 otherwise.
+// SSL_CTX_get_read_ahead returns 1 if |ctx| is not null and read ahead is
+// enabled, otherwise it returns 0.
 OPENSSL_EXPORT int SSL_CTX_get_read_ahead(const SSL_CTX *ctx);
 
 // SSL_CTX_set_read_ahead enables or disables read ahead on |ctx|:
@@ -5095,7 +5096,8 @@ OPENSSL_EXPORT int SSL_CTX_get_read_ahead(const SSL_CTX *ctx);
 // a blocking BIO is used and never returns the read could get stuck forever.
 OPENSSL_EXPORT int SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes);
 
-// SSL_get_read_ahead returns 1 if read ahead is enabled or 0 if it is not.
+// SSL_get_read_ahead returns 1 if |ssl| is not null and read ahead is enabled
+// otherwise it returns 0.
 OPENSSL_EXPORT int SSL_get_read_ahead(const SSL *ssl);
 
 // SSL_set_read_ahead enables or disables read ahead on |ssl|:
@@ -5114,15 +5116,15 @@ OPENSSL_EXPORT int SSL_set_read_ahead(SSL *ssl, int yes);
 
 // SSL_CTX_set_default_read_buffer_len sets the size of the buffer reads will use on
 // |ctx| if read ahead has been enabled. 0 is the minimum and 65535 is the maximum.
-// A |len| of 0 is the default behavior with read ahead turned off: each call to
+// A |len| of 0 is the same behavior as read ahead turned off: each call to
 // |SSL_read| reads the amount specified in the TLS Record Header.
-OPENSSL_EXPORT void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len);
+OPENSSL_EXPORT int SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len);
 
-// SSL_CTX_set_default_read_buffer_len sets the size of the buffer reads will use on
+// SSL_set_default_read_buffer_len sets the size of the buffer reads will use on
 // |ssl| if read ahead has been enabled. 0 is the minimum and 65535 is the maximum.
-// A |len| of 0 is the default behavior with read ahead turned off: each call to
+// A |len| of 0 is the same behavior as read ahead turned off: each call to
 // |SSL_read| reads the amount specified in the TLS Record Header.
-OPENSSL_EXPORT void SSL_set_default_read_buffer_len(SSL *ssl, size_t len);
+OPENSSL_EXPORT int SSL_set_default_read_buffer_len(SSL *ssl, size_t len);
 
 // SSL_MODE_HANDSHAKE_CUTTHROUGH is the same as SSL_MODE_ENABLE_FALSE_START.
 #define SSL_MODE_HANDSHAKE_CUTTHROUGH SSL_MODE_ENABLE_FALSE_START

ssl/internal.h

@@ -4126,7 +4126,7 @@ struct ssl_st {
   // If enable_early_data is true, early data can be sent and accepted.
   bool enable_early_data : 1;
 
-  // enable_read_ahead indicates whether the |SSL_CTX| is configured to read as much
+  // enable_read_ahead indicates whether the |SSL| is configured to read as much
   // as will fit in the SSLBuffer from the BIO, or just enough to read the record
   // header and then the length of the body
   bool enable_read_ahead : 1;

ssl/ssl_buffer.cc

@@ -29,8 +29,8 @@
 
 BSSL_NAMESPACE_BEGIN
 
-// BIO uses int instead of size_t. No lengths will exceed uint16_t, so this will
-// not overflow.
+// BIO uses int instead of size_t. No lengths will exceed SSLBUFFER_MAX_CAPACITY
+// (uint16_t), so this will not overflow.
 static_assert(SSLBUFFER_MAX_CAPACITY <= INT_MAX, "uint16_t does not fit in int");
 
 static_assert((SSL3_ALIGN_PAYLOAD & (SSL3_ALIGN_PAYLOAD - 1)) == 0,
@@ -240,6 +240,7 @@ static int tls_read_buffer_extend_to(SSL *ssl, size_t len) {
     // the while loop will bail once we have at least the requested len amount
     // of data. If not enable_read_ahead, only read as much to get to len bytes,
     // at this point we know len is less than the overall size of the buffer.
+    assert(buf->cap() >= buf->size());
     size_t read_amount = ssl->enable_read_ahead ? buf->cap() - buf->size() :
                                                      len - buf->size();
     assert(read_amount <= buf->cap() - buf->size());

ssl/ssl_lib.cc

@@ -1764,14 +1764,17 @@ int SSL_get_extms_support(const SSL *ssl) {
 }
 
 int SSL_CTX_get_read_ahead(const SSL_CTX *ctx) {
+  GUARD_PTR(ctx);
   return ctx->enable_read_ahead;
 }
 
 int SSL_get_read_ahead(const SSL *ssl) {
+  GUARD_PTR(ssl);
   return ssl->enable_read_ahead;
 }
 
-void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len) {
+int SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len) {
+  GUARD_PTR(ctx);
   // SSLBUFFER_MAX_CAPACITY(0xffff) is the maximum SSLBuffer supports reading at one time
   if (len > SSLBUFFER_MAX_CAPACITY) {
     len = SSLBUFFER_MAX_CAPACITY;
@@ -1780,9 +1783,11 @@ void SSL_CTX_set_default_read_buffer_len(SSL_CTX *ctx, size_t len) {
   // will always read at least the amount of data specified in the TLS record
   // header
   ctx->read_ahead_buffer_size = len;
+  return 1;
 }
 
-void SSL_set_default_read_buffer_len(SSL *ssl, size_t len) {
+int SSL_set_default_read_buffer_len(SSL *ssl, size_t len) {
+  GUARD_PTR(ssl);
   // SSLBUFFER_MAX_CAPACITY(0xffff) is the maximum SSLBuffer supports reading at one time
   if (len > SSLBUFFER_MAX_CAPACITY) {
     len = SSLBUFFER_MAX_CAPACITY;
@@ -1791,26 +1796,29 @@ void SSL_set_default_read_buffer_len(SSL *ssl, size_t len) {
   // will always read at least the amount of data specified in the TLS record
   // header
   ssl->read_ahead_buffer_size = len;
+  return 1;
 }
 
 int SSL_CTX_set_read_ahead(SSL_CTX *ctx, int yes) {
+  GUARD_PTR(ctx);
   if (yes == 0) {
-    ctx->enable_read_ahead = 0;
+    ctx->enable_read_ahead = false;
     return 1;
   } else if (yes == 1) {
-    ctx->enable_read_ahead = 1;
+    ctx->enable_read_ahead = true;
     return 1;
   } else {
     return 0;
   }
 }
 
 int SSL_set_read_ahead(SSL *ssl, int yes) {
+  GUARD_PTR(ssl);
   if (yes == 0) {
-    ssl->enable_read_ahead = 0;
+    ssl->enable_read_ahead = false;
     return 1;
   } else if (yes == 1) {
-    ssl->enable_read_ahead = 1;
+    ssl->enable_read_ahead = true;
     return 1;
   } else {
     return 0;

ssl/ssl_test.cc

@@ -6829,7 +6829,7 @@ TEST_P(SSLVersionTest, ReadAhead) {
     ASSERT_EQ(1, SSL_write_ex(server_.get(), &i, 1, &buf_len));
   }
 
-  char *buf = new char[test_string.length()];
+  char buf[13];
   size_t starting_size = BIO_pending(client_.get()->rbio.get());
 
   ASSERT_NE(0UL, starting_size);