From cfc19c3780ea2a7ab78ba38b0f211b420518c366 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Fri, 1 Dec 2023 14:35:09 -0500 Subject: [PATCH 01/15] Don't return SSL_ERROR_SYSCALL for EOF --- ssl/ssl_lib.cc | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 2560e6804b1..57cf0df30eb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,14 +1382,8 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } - if (ret_code == 0) { - if (ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { - return SSL_ERROR_ZERO_RETURN; - } - // An EOF was observed which violates the protocol, and the underlying - // transport does not participate in the error queue. Bubble up to the - // caller. - return SSL_ERROR_SYSCALL; + if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { + return SSL_ERROR_ZERO_RETURN; } switch (ssl->s3->rwstate) { From c7a55aa97fd5cc9bcdfddbc247a8db30c07ad43f Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Mon, 18 Dec 2023 21:14:15 +0000 Subject: [PATCH 02/15] Try returning SSL_ERROR_SSL and set reason --- include/openssl/ssl.h | 5 ++- ssl/ssl_lib.cc | 8 +++-- ssl/ssl_test.cc | 81 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 4bdd0df2567..eb7ca2b0495 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -570,9 +570,7 @@ OPENSSL_EXPORT int SSL_get_error(const SSL *ssl, int ret_code); // SSL_ERROR_SYSCALL indicates the operation failed externally to the library. // The caller should consult the system-specific error mechanism. This is -// typically |errno| but may be something custom if using a custom |BIO|. It -// may also be signaled if the transport returned EOF, in which case the -// operation's return value will be zero. +// typically |errno| but may be something custom if using a custom |BIO|. #define SSL_ERROR_SYSCALL 5 // SSL_ERROR_ZERO_RETURN indicates the operation failed because the connection @@ -6189,5 +6187,6 @@ BSSL_NAMESPACE_END #define SSL_R_TLSV1_ALERT_CERTIFICATE_REQUIRED 1116 #define SSL_R_TLSV1_ALERT_NO_APPLICATION_PROTOCOL 1120 #define SSL_R_TLSV1_ALERT_ECH_REQUIRED 1121 +#define SSL_R_UNEXPECTED_EOF_WHILE_READING 1122 #endif // OPENSSL_HEADER_SSL_H diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 57cf0df30eb..c74d2351f13 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,8 +1382,12 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } - if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { - return SSL_ERROR_ZERO_RETURN; + if (ret_code == 0) { + if (ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { + return SSL_ERROR_ZERO_RETURN; + } + OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EOF_WHILE_READING); + return SSL_ERROR_SSL; } switch (ssl->s3->rwstate) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 470333971a1..c48ec587f6b 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10458,6 +10458,87 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { write_failed = false; } +// TODO +TEST(SSLTest, ErrorSyscallAfterEof) { + // Make a custom |BIO| where writes fail, but without pushing to the error + // queue. + bssl::UniquePtr method(BIO_meth_new(0, nullptr)); + ASSERT_TRUE(method); + BIO_meth_set_create(method.get(), [](BIO *b) -> int { + BIO_set_init(b, 1); + return 1; + }); + static bool write_failed = false; + BIO_meth_set_write(method.get(), [](BIO *, const char *, int) -> int { + // Fail the operation and don't add to the error queue. + write_failed = true; + return -1; + }); + bssl::UniquePtr wbio_silent_error(BIO_new(method.get())); + ASSERT_TRUE(wbio_silent_error); + + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + + // Replace the write |BIO| with |wbio_silent_error|. + SSL_set0_wbio(client.get(), wbio_silent_error.release()); + + // Writes should fail. There is nothing in the error queue, so + // |SSL_ERROR_SYSCALL| indicates the caller needs to check out-of-band. + const uint8_t data[1] = {0}; + int ret = SSL_write(client.get(), data, sizeof(data)); + EXPECT_EQ(ret, -1); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); + EXPECT_TRUE(write_failed); + write_failed = false; + + // Send a close_notify from the server. It should return 0 because + // close_notify was sent, but not received. Confusingly, this is a success + // output for |SSL_shutdown|'s API. + EXPECT_EQ(SSL_shutdown(server.get()), 0); + + // Read the close_notify on the client. + uint8_t buf[1]; + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); + + // Further calls to |SSL_read| continue to report |SSL_ERROR_ZERO_RETURN|. + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); + + // Although the client has seen close_notify, it should continue to report + // |SSL_ERROR_SYSCALL| when its writes fail. + ret = SSL_write(client.get(), data, sizeof(data)); + EXPECT_EQ(ret, -1); + 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 // a close_notify, down to |SSL_read| reporting |SSL_ERROR_ZERO_RETURN|. TEST(SSLTest, QuietShutdown) { From a06c57b1e4447725ec84171f4e5ef1970748bd33 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Tue, 19 Dec 2023 15:06:08 -0500 Subject: [PATCH 03/15] Revert "Try returning SSL_ERROR_SSL and set reason" This reverts commit 5626fe8550e6cfb9d18bd1752be02ad7b11f3ee8. --- include/openssl/ssl.h | 5 +-- ssl/ssl_lib.cc | 8 ++--- ssl/ssl_test.cc | 81 ------------------------------------------- 3 files changed, 5 insertions(+), 89 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index eb7ca2b0495..4bdd0df2567 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -570,7 +570,9 @@ OPENSSL_EXPORT int SSL_get_error(const SSL *ssl, int ret_code); // SSL_ERROR_SYSCALL indicates the operation failed externally to the library. // The caller should consult the system-specific error mechanism. This is -// typically |errno| but may be something custom if using a custom |BIO|. +// typically |errno| but may be something custom if using a custom |BIO|. It +// may also be signaled if the transport returned EOF, in which case the +// operation's return value will be zero. #define SSL_ERROR_SYSCALL 5 // SSL_ERROR_ZERO_RETURN indicates the operation failed because the connection @@ -6187,6 +6189,5 @@ BSSL_NAMESPACE_END #define SSL_R_TLSV1_ALERT_CERTIFICATE_REQUIRED 1116 #define SSL_R_TLSV1_ALERT_NO_APPLICATION_PROTOCOL 1120 #define SSL_R_TLSV1_ALERT_ECH_REQUIRED 1121 -#define SSL_R_UNEXPECTED_EOF_WHILE_READING 1122 #endif // OPENSSL_HEADER_SSL_H diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index c74d2351f13..57cf0df30eb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,12 +1382,8 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } - if (ret_code == 0) { - if (ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { - return SSL_ERROR_ZERO_RETURN; - } - OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EOF_WHILE_READING); - return SSL_ERROR_SSL; + if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { + return SSL_ERROR_ZERO_RETURN; } switch (ssl->s3->rwstate) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index c48ec587f6b..470333971a1 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10458,87 +10458,6 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { write_failed = false; } -// TODO -TEST(SSLTest, ErrorSyscallAfterEof) { - // Make a custom |BIO| where writes fail, but without pushing to the error - // queue. - bssl::UniquePtr method(BIO_meth_new(0, nullptr)); - ASSERT_TRUE(method); - BIO_meth_set_create(method.get(), [](BIO *b) -> int { - BIO_set_init(b, 1); - return 1; - }); - static bool write_failed = false; - BIO_meth_set_write(method.get(), [](BIO *, const char *, int) -> int { - // Fail the operation and don't add to the error queue. - write_failed = true; - return -1; - }); - bssl::UniquePtr wbio_silent_error(BIO_new(method.get())); - ASSERT_TRUE(wbio_silent_error); - - bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); - bssl::UniquePtr server_ctx = - CreateContextWithTestCertificate(TLS_method()); - ASSERT_TRUE(client_ctx); - ASSERT_TRUE(server_ctx); - bssl::UniquePtr client, server; - ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), - server_ctx.get())); - - // Replace the write |BIO| with |wbio_silent_error|. - SSL_set0_wbio(client.get(), wbio_silent_error.release()); - - // Writes should fail. There is nothing in the error queue, so - // |SSL_ERROR_SYSCALL| indicates the caller needs to check out-of-band. - const uint8_t data[1] = {0}; - int ret = SSL_write(client.get(), data, sizeof(data)); - EXPECT_EQ(ret, -1); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); - EXPECT_TRUE(write_failed); - write_failed = false; - - // Send a close_notify from the server. It should return 0 because - // close_notify was sent, but not received. Confusingly, this is a success - // output for |SSL_shutdown|'s API. - EXPECT_EQ(SSL_shutdown(server.get()), 0); - - // Read the close_notify on the client. - uint8_t buf[1]; - ret = SSL_read(client.get(), buf, sizeof(buf)); - EXPECT_EQ(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); - - // Further calls to |SSL_read| continue to report |SSL_ERROR_ZERO_RETURN|. - ret = SSL_read(client.get(), buf, sizeof(buf)); - EXPECT_EQ(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_ZERO_RETURN); - - // Although the client has seen close_notify, it should continue to report - // |SSL_ERROR_SYSCALL| when its writes fail. - ret = SSL_write(client.get(), data, sizeof(data)); - EXPECT_EQ(ret, -1); - 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 // a close_notify, down to |SSL_read| reporting |SSL_ERROR_ZERO_RETURN|. TEST(SSLTest, QuietShutdown) { From 8d9b2fb21b014ba986f09e637674da75d4a5245c Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Tue, 19 Dec 2023 15:16:45 -0500 Subject: [PATCH 04/15] Move SSL_ERROR_ZERO_RETURN check to match OSSL 1.1.1 --- ssl/ssl_lib.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 57cf0df30eb..4ca894ef0a9 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,10 +1382,6 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } - if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { - return SSL_ERROR_ZERO_RETURN; - } - switch (ssl->s3->rwstate) { case SSL_ERROR_PENDING_SESSION: case SSL_ERROR_PENDING_CERTIFICATE: @@ -1398,6 +1394,7 @@ int SSL_get_error(const SSL *ssl, int ret_code) { case SSL_ERROR_WANT_CERTIFICATE_VERIFY: case SSL_ERROR_WANT_RENEGOTIATE: case SSL_ERROR_HANDSHAKE_HINTS_READY: + case SSL_ERROR_ZERO_RETURN: return ssl->s3->rwstate; case SSL_ERROR_WANT_READ: { From 871c8f1246d45ca261ab5a4ac0fc495db0af81e2 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 3 Jan 2024 19:57:56 +0000 Subject: [PATCH 05/15] Revert "Move SSL_ERROR_ZERO_RETURN check to match OSSL 1.1.1" This reverts commit 0ddfd60f9d356b29cb7b0d25f79835c43b63c8f2. --- ssl/ssl_lib.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 4ca894ef0a9..57cf0df30eb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,6 +1382,10 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } + if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { + return SSL_ERROR_ZERO_RETURN; + } + switch (ssl->s3->rwstate) { case SSL_ERROR_PENDING_SESSION: case SSL_ERROR_PENDING_CERTIFICATE: @@ -1394,7 +1398,6 @@ int SSL_get_error(const SSL *ssl, int ret_code) { case SSL_ERROR_WANT_CERTIFICATE_VERIFY: case SSL_ERROR_WANT_RENEGOTIATE: case SSL_ERROR_HANDSHAKE_HINTS_READY: - case SSL_ERROR_ZERO_RETURN: return ssl->s3->rwstate; case SSL_ERROR_WANT_READ: { From 97e02300f324f91b72284e7eae03f33186de2a4b Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 3 Jan 2024 19:58:25 +0000 Subject: [PATCH 06/15] Revert "Don't return SSL_ERROR_SYSCALL for EOF" This reverts commit d54629b247a0ec907ad6ea05737b91e7678da175. --- ssl/ssl_lib.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 57cf0df30eb..2560e6804b1 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1382,8 +1382,14 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_SSL; } - if (ret_code == 0 && ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { - return SSL_ERROR_ZERO_RETURN; + if (ret_code == 0) { + if (ssl->s3->rwstate == SSL_ERROR_ZERO_RETURN) { + return SSL_ERROR_ZERO_RETURN; + } + // An EOF was observed which violates the protocol, and the underlying + // transport does not participate in the error queue. Bubble up to the + // caller. + return SSL_ERROR_SYSCALL; } switch (ssl->s3->rwstate) { From a8829dc8238f796e851fdfc1fa90720093677aea Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 3 Jan 2024 20:08:09 +0000 Subject: [PATCH 07/15] Set out len before returning on err --- ssl/ssl_lib.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 2560e6804b1..667387ecd5b 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1044,12 +1044,12 @@ int SSL_read_ex(SSL *ssl, void *buf, size_t num, size_t *read_bytes) { return 1; } int ret = SSL_read(ssl, buf, (int)num); - if (ret <= 0) { - return 0; - } if (read_bytes != nullptr) { *read_bytes = ret; } + if (ret <= 0) { + return 0; + } return 1; } @@ -1143,12 +1143,12 @@ int SSL_write_ex(SSL *ssl, const void *buf, size_t num, size_t *written) { return 1; } int ret = SSL_write(ssl, buf, (int)num); - if (ret <= 0) { - return 0; - } if (written != nullptr) { *written = ret; } + if (ret <= 0) { + return 0; + } return 1; } From 19d26a094da065a4288d0e1f580cb193d16dfaad Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 3 Jan 2024 21:41:44 +0000 Subject: [PATCH 08/15] Return raw retval on error --- ssl/ssl_lib.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 667387ecd5b..73f5b5b6efa 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1044,12 +1044,12 @@ int SSL_read_ex(SSL *ssl, void *buf, size_t num, size_t *read_bytes) { return 1; } int ret = SSL_read(ssl, buf, (int)num); + if (ret <= 0) { + return ret; + } if (read_bytes != nullptr) { *read_bytes = ret; } - if (ret <= 0) { - return 0; - } return 1; } @@ -1143,12 +1143,12 @@ int SSL_write_ex(SSL *ssl, const void *buf, size_t num, size_t *written) { return 1; } int ret = SSL_write(ssl, buf, (int)num); + if (ret <= 0) { + return ret; + } if (written != nullptr) { *written = ret; } - if (ret <= 0) { - return 0; - } return 1; } From 0aa083482866a90f0e2d27231a64bb9ae5b1a7a8 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 4 Jan 2024 15:43:49 +0000 Subject: [PATCH 09/15] Revert other changes, guard ERR_SYSCALL return --- ssl/ssl_lib.cc | 11 +++++++---- ssl/ssl_test.cc | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 73f5b5b6efa..561736cd4bb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1045,7 +1045,7 @@ int SSL_read_ex(SSL *ssl, void *buf, size_t num, size_t *read_bytes) { } int ret = SSL_read(ssl, buf, (int)num); if (ret <= 0) { - return ret; + return 0; } if (read_bytes != nullptr) { *read_bytes = ret; @@ -1144,7 +1144,7 @@ int SSL_write_ex(SSL *ssl, const void *buf, size_t num, size_t *written) { } int ret = SSL_write(ssl, buf, (int)num); if (ret <= 0) { - return ret; + return 0; } if (written != nullptr) { *written = ret; @@ -1388,8 +1388,11 @@ int SSL_get_error(const SSL *ssl, int ret_code) { } // An EOF was observed which violates the protocol, and the underlying // transport does not participate in the error queue. Bubble up to the - // caller. - return SSL_ERROR_SYSCALL; + // caller. Do not consider retryable |rwstate| EOF. + if (ssl->s3->rwstate != SSL_ERROR_WANT_READ + && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) { + return SSL_ERROR_SYSCALL; + } } switch (ssl->s3->rwstate) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 470333971a1..3611baf155e 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10458,6 +10458,57 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { write_failed = false; } +// Test that intermittent inability to read results in retryable error +TEST(SSLTest, IntermittentEmptyRead) { + bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); + bssl::UniquePtr server_ctx = + CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + bssl::UniquePtr client, server; + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + + // Create a fake read BIO that returns 0 on read to simulate empty read + bssl::UniquePtr method(BIO_meth_new(0, nullptr)); + ASSERT_TRUE(method); + BIO_meth_set_create(method.get(), [](BIO *b) -> int { + BIO_set_init(b, 1); + return 1; + }); + BIO_meth_set_read(method.get(), [](BIO *, char *, int) -> int { + return 0; + }); + bssl::UniquePtr rbio_empty(BIO_new(method.get())); + ASSERT_TRUE(rbio_empty); + BIO_set_flags(rbio_empty.get(), BIO_FLAGS_READ); + + // Save off client rbio and use empty read BIO + bssl::UniquePtr client_rbio(SSL_get_rbio(client.get())); + ASSERT_TRUE(client_rbio); + // Up-ref |client_rbio| as SSL_CTX dtor will also attempt to free it + ASSERT_TRUE(BIO_up_ref(client_rbio.get())); + SSL_set0_rbio(client.get(), rbio_empty.release()); + + // Server writes some data to the client + const uint8_t data[1] = {0}; + int ret = SSL_write(server.get(), data, (int) sizeof(data)); + EXPECT_EQ(ret, (int) sizeof(data)); + EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); + + // On empty read, client should still want a read so caller will retry + uint8_t buf[1]; + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); + + // Reset client rbio, read should succeed + SSL_set0_rbio(client.get(), client_rbio.release()); + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_EQ(ret, (int) sizeof(buf)); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE); +} + // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving // a close_notify, down to |SSL_read| reporting |SSL_ERROR_ZERO_RETURN|. TEST(SSLTest, QuietShutdown) { From 8b160bff8e1091a6db5b2eff9bf0289469cc5139 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 10 Jan 2024 19:17:15 +0000 Subject: [PATCH 10/15] Revert guard, impl. SSL_MODE_AUTO_RETRY --- include/openssl/ssl.h | 4 +++- ssl/ssl_lib.cc | 12 ++++++------ ssl/ssl_test.cc | 4 ++++ 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 4bdd0df2567..137871f1bc5 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -869,6 +869,9 @@ OPENSSL_EXPORT uint32_t SSL_get_options(const SSL *ssl); // session resumption is used for a given SSL*. #define SSL_MODE_NO_SESSION_CREATION 0x00000200L +// TODO [childw] +#define SSL_MODE_AUTO_RETRY 0x00000004L + // SSL_MODE_SEND_FALLBACK_SCSV sends TLS_FALLBACK_SCSV in the ClientHello. // To be set only by applications that reconnect with a downgraded protocol // version; see RFC 7507 for details. @@ -5283,7 +5286,6 @@ DEFINE_STACK_OF(SSL_COMP) // The following flags do nothing and are included only to make it easier to // compile code with BoringSSL. -#define SSL_MODE_AUTO_RETRY 0 #define SSL_MODE_RELEASE_BUFFERS 0 #define SSL_MODE_SEND_CLIENTHELLO_TIME 0 #define SSL_MODE_SEND_SERVERHELLO_TIME 0 diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 561736cd4bb..411b6a05557 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1027,7 +1027,10 @@ static int ssl_read_impl(SSL *ssl) { &alert, ssl->s3->read_buffer.span()); bool retry; int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert); - if (bio_ret <= 0) { + + if (bio_ret == 0 && retry && (ssl->ctx->mode & SSL_MODE_AUTO_RETRY)) { + continue; + } else if (bio_ret <= 0) { return bio_ret; } if (!retry) { @@ -1388,11 +1391,8 @@ int SSL_get_error(const SSL *ssl, int ret_code) { } // An EOF was observed which violates the protocol, and the underlying // transport does not participate in the error queue. Bubble up to the - // caller. Do not consider retryable |rwstate| EOF. - if (ssl->s3->rwstate != SSL_ERROR_WANT_READ - && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) { - return SSL_ERROR_SYSCALL; - } + // caller. + return SSL_ERROR_SYSCALL; } switch (ssl->s3->rwstate) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 3611baf155e..9fe08c9dcbb 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10507,6 +10507,10 @@ TEST(SSLTest, IntermittentEmptyRead) { ret = SSL_read(client.get(), buf, sizeof(buf)); EXPECT_EQ(ret, (int) sizeof(buf)); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE); + + ret = SSL_read(client.get(), buf, sizeof(buf)); + EXPECT_LE(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SSL); } // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving From 16836270de71afb50f558c5abe21ec9c94f9c2eb Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 10 Jan 2024 21:38:47 +0000 Subject: [PATCH 11/15] Reinstate guard, abandon SSL_MODE_AUTO_RETRY --- include/openssl/ssl.h | 4 +--- ssl/ssl_lib.cc | 12 ++++++------ ssl/ssl_test.cc | 22 ++++++++++++---------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 137871f1bc5..4bdd0df2567 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -869,9 +869,6 @@ OPENSSL_EXPORT uint32_t SSL_get_options(const SSL *ssl); // session resumption is used for a given SSL*. #define SSL_MODE_NO_SESSION_CREATION 0x00000200L -// TODO [childw] -#define SSL_MODE_AUTO_RETRY 0x00000004L - // SSL_MODE_SEND_FALLBACK_SCSV sends TLS_FALLBACK_SCSV in the ClientHello. // To be set only by applications that reconnect with a downgraded protocol // version; see RFC 7507 for details. @@ -5286,6 +5283,7 @@ DEFINE_STACK_OF(SSL_COMP) // The following flags do nothing and are included only to make it easier to // compile code with BoringSSL. +#define SSL_MODE_AUTO_RETRY 0 #define SSL_MODE_RELEASE_BUFFERS 0 #define SSL_MODE_SEND_CLIENTHELLO_TIME 0 #define SSL_MODE_SEND_SERVERHELLO_TIME 0 diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 411b6a05557..561736cd4bb 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1027,10 +1027,7 @@ static int ssl_read_impl(SSL *ssl) { &alert, ssl->s3->read_buffer.span()); bool retry; int bio_ret = ssl_handle_open_record(ssl, &retry, ret, consumed, alert); - - if (bio_ret == 0 && retry && (ssl->ctx->mode & SSL_MODE_AUTO_RETRY)) { - continue; - } else if (bio_ret <= 0) { + if (bio_ret <= 0) { return bio_ret; } if (!retry) { @@ -1391,8 +1388,11 @@ int SSL_get_error(const SSL *ssl, int ret_code) { } // An EOF was observed which violates the protocol, and the underlying // transport does not participate in the error queue. Bubble up to the - // caller. - return SSL_ERROR_SYSCALL; + // caller. Do not consider retryable |rwstate| EOF. + if (ssl->s3->rwstate != SSL_ERROR_WANT_READ + && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) { + return SSL_ERROR_SYSCALL; + } } switch (ssl->s3->rwstate) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 9fe08c9dcbb..baf82a22ad4 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10491,26 +10491,28 @@ TEST(SSLTest, IntermittentEmptyRead) { SSL_set0_rbio(client.get(), rbio_empty.release()); // Server writes some data to the client - const uint8_t data[1] = {0}; - int ret = SSL_write(server.get(), data, (int) sizeof(data)); - EXPECT_EQ(ret, (int) sizeof(data)); + const uint8_t write_data[] = {1, 2, 3}; + int ret = SSL_write(server.get(), write_data, (int) sizeof(write_data)); + EXPECT_EQ(ret, (int) sizeof(write_data)); EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); // On empty read, client should still want a read so caller will retry - uint8_t buf[1]; - ret = SSL_read(client.get(), buf, sizeof(buf)); + uint8_t read_data[] = {0, 0, 0}; + ret = SSL_read(client.get(), read_data, sizeof(read_data)); EXPECT_EQ(ret, 0); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); // Reset client rbio, read should succeed SSL_set0_rbio(client.get(), client_rbio.release()); - ret = SSL_read(client.get(), buf, sizeof(buf)); - EXPECT_EQ(ret, (int) sizeof(buf)); + ret = SSL_read(client.get(), read_data, sizeof(read_data)); + EXPECT_EQ(ret, (int) sizeof(write_data)); + EXPECT_EQ(OPENSSL_memcmp(read_data, write_data, sizeof(write_data)), 0); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE); - ret = SSL_read(client.get(), buf, sizeof(buf)); - EXPECT_LE(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SSL); + // Subsequent attempts to read should fail + ret = SSL_read(client.get(), read_data, sizeof(read_data)); + EXPECT_LT(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); } // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving From a866c1132da61b219fc2f10edecf05189d51f5a4 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 12 Jan 2024 00:46:25 +0000 Subject: [PATCH 12/15] Condition SSL_ERROR_SYSCALL suppression on SSL_MODE_AUTO_RETRY --- include/openssl/ssl.h | 5 ++++- ssl/ssl_lib.cc | 10 +++++---- ssl/ssl_test.cc | 49 +++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 4bdd0df2567..c4f1506b990 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -834,6 +834,10 @@ OPENSSL_EXPORT uint32_t SSL_get_options(const SSL *ssl); // |write|. In DTLS, it does nothing. #define SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER 0x00000002L +// SSL_MODE_AUTO_RETRY suppresses terminal errors on empty reads if the +// underlying connection state is retryable, allowing for automatic retries. +#define SSL_MODE_AUTO_RETRY 0x00000004L + // SSL_MODE_NO_AUTO_CHAIN disables automatically building a certificate chain // before sending certificates to the peer. This flag is set (and the feature // disabled) by default. @@ -5283,7 +5287,6 @@ DEFINE_STACK_OF(SSL_COMP) // The following flags do nothing and are included only to make it easier to // compile code with BoringSSL. -#define SSL_MODE_AUTO_RETRY 0 #define SSL_MODE_RELEASE_BUFFERS 0 #define SSL_MODE_SEND_CLIENTHELLO_TIME 0 #define SSL_MODE_SEND_SERVERHELLO_TIME 0 diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index 561736cd4bb..a61a7212a26 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1387,10 +1387,12 @@ int SSL_get_error(const SSL *ssl, int ret_code) { return SSL_ERROR_ZERO_RETURN; } // An EOF was observed which violates the protocol, and the underlying - // transport does not participate in the error queue. Bubble up to the - // caller. Do not consider retryable |rwstate| EOF. - if (ssl->s3->rwstate != SSL_ERROR_WANT_READ - && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) { + // transport does not participate in the error queue. If + // |SSL_MODE_AUTO_RETRY| is unset or we're in a non-retryable state, bubble + // up to the caller. + if ((ssl->ctx->mode & SSL_MODE_AUTO_RETRY) == 0 + || (ssl->s3->rwstate != SSL_ERROR_WANT_READ + && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE)) { return SSL_ERROR_SYSCALL; } } diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index baf82a22ad4..9c5f78adc95 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10458,7 +10458,8 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { write_failed = false; } -// Test that intermittent inability to read results in retryable error +// Test that |SSL_MODE_AUTO_RETRY| suppresses failure on (potentially) +// transient empty reads. TEST(SSLTest, IntermittentEmptyRead) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); bssl::UniquePtr server_ctx = @@ -10490,16 +10491,60 @@ TEST(SSLTest, IntermittentEmptyRead) { ASSERT_TRUE(BIO_up_ref(client_rbio.get())); SSL_set0_rbio(client.get(), rbio_empty.release()); + // |SSL_MODE_AUTO_RETRY| is off by default + ASSERT_FALSE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); + // Server writes some data to the client const uint8_t write_data[] = {1, 2, 3}; int ret = SSL_write(server.get(), write_data, (int) sizeof(write_data)); EXPECT_EQ(ret, (int) sizeof(write_data)); EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); - // On empty read, client should still want a read so caller will retry + // On empty read, client should error out signaling EOF uint8_t read_data[] = {0, 0, 0}; ret = SSL_read(client.get(), read_data, sizeof(read_data)); EXPECT_EQ(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); + + // Reset client rbio, read should succeed + SSL_set0_rbio(client.get(), client_rbio.release()); + ret = SSL_read(client.get(), read_data, sizeof(read_data)); + EXPECT_EQ(ret, (int) sizeof(write_data)); + EXPECT_EQ(OPENSSL_memcmp(read_data, write_data, sizeof(write_data)), 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE); + + // Subsequent attempts to read should fail + ret = SSL_read(client.get(), read_data, sizeof(read_data)); + EXPECT_LT(ret, 0); + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); + + // Next, setu up the same test with |SSL_MODE_AUTO_RETRY| set + client_ctx.reset(SSL_CTX_new(TLS_method())); + server_ctx = CreateContextWithTestCertificate(TLS_method()); + ASSERT_TRUE(client_ctx); + ASSERT_TRUE(server_ctx); + ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), + server_ctx.get())); + rbio_empty.reset(BIO_new(method.get())); + ASSERT_TRUE(rbio_empty); + BIO_set_flags(rbio_empty.get(), BIO_FLAGS_READ); + client_rbio.reset(SSL_get_rbio(client.get())); + ASSERT_TRUE(client_rbio); + ASSERT_TRUE(BIO_up_ref(client_rbio.get())); + SSL_set0_rbio(client.get(), rbio_empty.release()); + + // Set flag under test + ASSERT_TRUE(SSL_CTX_set_mode(client_ctx.get(), SSL_MODE_AUTO_RETRY)); + ASSERT_TRUE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); + + // Server writes some data to the client + ret = SSL_write(server.get(), write_data, (int) sizeof(write_data)); + EXPECT_EQ(ret, (int) sizeof(write_data)); + EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); + + // On empty read, client should still want a read so caller will retry + ret = SSL_read(client.get(), read_data, sizeof(read_data)); + EXPECT_EQ(ret, 0); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); // Reset client rbio, read should succeed From 6f4c696784db5e3da95fb593483b2d89290713cc Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 12 Jan 2024 16:40:41 +0000 Subject: [PATCH 13/15] Add assertions around BIO method setters --- ssl/ssl_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 9c5f78adc95..b7977201f44 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10473,13 +10473,13 @@ TEST(SSLTest, IntermittentEmptyRead) { // Create a fake read BIO that returns 0 on read to simulate empty read bssl::UniquePtr method(BIO_meth_new(0, nullptr)); ASSERT_TRUE(method); - BIO_meth_set_create(method.get(), [](BIO *b) -> int { + ASSERT_TRUE(BIO_meth_set_create(method.get(), [](BIO *b) -> int { BIO_set_init(b, 1); return 1; - }); - BIO_meth_set_read(method.get(), [](BIO *, char *, int) -> int { + })); + ASSERT_TRUE(BIO_meth_set_read(method.get(), [](BIO *, char *, int) -> int { return 0; - }); + })); bssl::UniquePtr rbio_empty(BIO_new(method.get())); ASSERT_TRUE(rbio_empty); BIO_set_flags(rbio_empty.get(), BIO_FLAGS_READ); From 0457256d73bf3bb91fa269e2870ea8a5d0a67800 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Tue, 16 Jan 2024 23:13:55 +0000 Subject: [PATCH 14/15] Consolidate test logic --- ssl/ssl_test.cc | 69 ++++++++++++++++--------------------------------- 1 file changed, 22 insertions(+), 47 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index b7977201f44..a1c934579a0 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -10458,9 +10458,7 @@ TEST(SSLTest, ErrorSyscallAfterCloseNotify) { write_failed = false; } -// Test that |SSL_MODE_AUTO_RETRY| suppresses failure on (potentially) -// transient empty reads. -TEST(SSLTest, IntermittentEmptyRead) { +static void TestIntermittentEmptyRead(bool auto_retry) { bssl::UniquePtr client_ctx(SSL_CTX_new(TLS_method())); bssl::UniquePtr server_ctx = CreateContextWithTestCertificate(TLS_method()); @@ -10491,8 +10489,14 @@ TEST(SSLTest, IntermittentEmptyRead) { ASSERT_TRUE(BIO_up_ref(client_rbio.get())); SSL_set0_rbio(client.get(), rbio_empty.release()); - // |SSL_MODE_AUTO_RETRY| is off by default - ASSERT_FALSE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); + if (auto_retry) { + // Set flag under test + ASSERT_TRUE(SSL_CTX_set_mode(client_ctx.get(), SSL_MODE_AUTO_RETRY)); + ASSERT_TRUE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); + } else { + // |SSL_MODE_AUTO_RETRY| is off by default + ASSERT_FALSE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); + } // Server writes some data to the client const uint8_t write_data[] = {1, 2, 3}; @@ -10500,11 +10504,16 @@ TEST(SSLTest, IntermittentEmptyRead) { EXPECT_EQ(ret, (int) sizeof(write_data)); EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); - // On empty read, client should error out signaling EOF uint8_t read_data[] = {0, 0, 0}; ret = SSL_read(client.get(), read_data, sizeof(read_data)); EXPECT_EQ(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); + if (auto_retry) { + // On empty read, client should still want a read so caller will retry + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); + } else { + // On empty read, client should error out signaling EOF + EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_SYSCALL); + } // Reset client rbio, read should succeed SSL_set0_rbio(client.get(), client_rbio.release()); @@ -10517,47 +10526,13 @@ TEST(SSLTest, IntermittentEmptyRead) { ret = SSL_read(client.get(), read_data, sizeof(read_data)); EXPECT_LT(ret, 0); EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); +} - // Next, setu up the same test with |SSL_MODE_AUTO_RETRY| set - client_ctx.reset(SSL_CTX_new(TLS_method())); - server_ctx = CreateContextWithTestCertificate(TLS_method()); - ASSERT_TRUE(client_ctx); - ASSERT_TRUE(server_ctx); - ASSERT_TRUE(ConnectClientAndServer(&client, &server, client_ctx.get(), - server_ctx.get())); - rbio_empty.reset(BIO_new(method.get())); - ASSERT_TRUE(rbio_empty); - BIO_set_flags(rbio_empty.get(), BIO_FLAGS_READ); - client_rbio.reset(SSL_get_rbio(client.get())); - ASSERT_TRUE(client_rbio); - ASSERT_TRUE(BIO_up_ref(client_rbio.get())); - SSL_set0_rbio(client.get(), rbio_empty.release()); - - // Set flag under test - ASSERT_TRUE(SSL_CTX_set_mode(client_ctx.get(), SSL_MODE_AUTO_RETRY)); - ASSERT_TRUE(SSL_CTX_get_mode(client_ctx.get()) & SSL_MODE_AUTO_RETRY); - - // Server writes some data to the client - ret = SSL_write(server.get(), write_data, (int) sizeof(write_data)); - EXPECT_EQ(ret, (int) sizeof(write_data)); - EXPECT_EQ(SSL_get_error(server.get(), ret), SSL_ERROR_NONE); - - // On empty read, client should still want a read so caller will retry - ret = SSL_read(client.get(), read_data, sizeof(read_data)); - EXPECT_EQ(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); - - // Reset client rbio, read should succeed - SSL_set0_rbio(client.get(), client_rbio.release()); - ret = SSL_read(client.get(), read_data, sizeof(read_data)); - EXPECT_EQ(ret, (int) sizeof(write_data)); - EXPECT_EQ(OPENSSL_memcmp(read_data, write_data, sizeof(write_data)), 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_NONE); - - // Subsequent attempts to read should fail - ret = SSL_read(client.get(), read_data, sizeof(read_data)); - EXPECT_LT(ret, 0); - EXPECT_EQ(SSL_get_error(client.get(), ret), SSL_ERROR_WANT_READ); +// Test that |SSL_MODE_AUTO_RETRY| suppresses failure on (potentially) +// transient empty reads. +TEST(SSLTest, IntermittentEmptyRead) { + TestIntermittentEmptyRead(false); + TestIntermittentEmptyRead(true); } // Test that |SSL_shutdown|, when quiet shutdown is enabled, simulates receiving From 85c669fb4e4435c57243815003bb39158638d52c Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Tue, 16 Jan 2024 23:39:54 +0000 Subject: [PATCH 15/15] Make source change more readable --- ssl/ssl_lib.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index a61a7212a26..9ca5533bcca 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -1388,11 +1388,13 @@ int SSL_get_error(const SSL *ssl, int ret_code) { } // An EOF was observed which violates the protocol, and the underlying // transport does not participate in the error queue. If - // |SSL_MODE_AUTO_RETRY| is unset or we're in a non-retryable state, bubble - // up to the caller. - if ((ssl->ctx->mode & SSL_MODE_AUTO_RETRY) == 0 - || (ssl->s3->rwstate != SSL_ERROR_WANT_READ - && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE)) { + // |SSL_MODE_AUTO_RETRY| is unset, bubble up to the caller. + if ((ssl->ctx->mode & SSL_MODE_AUTO_RETRY) == 0) { + return SSL_ERROR_SYSCALL; + } + // If |SSL_MODE_AUTO_RETRY| is set, proceed if in a retryable state. + if (ssl->s3->rwstate != SSL_ERROR_WANT_READ + && ssl->s3->rwstate != SSL_ERROR_WANT_WRITE) { return SSL_ERROR_SYSCALL; } }