Clear the error queue on entry to core SSL operations.
OpenSSL historically made some poor API decisions. Rather than returning a
status enum in SSL_read, etc., these functions must be paired with
SSL_get_error which determines the cause of the last error's failure. This
requires SSL_read communicate with SSL_get_error with some stateful flag,
rwstate.
Further, probably as workarounds for bugs elsewhere, SSL_get_error does not
trust rwstate. Among other quirks, if the error queue is non-empty,
SSL_get_error overrides rwstate and returns a value based on that. This
requires that SSL_read, etc., be called with an empty error queue. (Or we hit
one of the spurious ERR_clear_error calls in the handshake state machine,
likely added as further self-workarounds.)
Since requiring callers consistently clear the error queue everywhere is
unreasonable (crbug.com/567501), clear ERR_clear_error *once* at the entry
point. Until/unless[*] we make SSL_get_error sane, this is the most reasonable
way to get to the point that clearing the error queue on error is optional.
With those in place, the calls in the handshake state machine are no longer
needed. (I suspect all the ERR_clear_system_error calls can also go, but I'll
investigate and think about that separately.)
[*] I'm not even sure it's possible anymore, thanks to the possibility of
BIO_write pushing to the error queue.
BUG=567501,593963
Change-Id: I564ace199e5a4a74b2554ad3335e99cd17120741
Reviewed-on: https://boringssl-review.googlesource.com/7455
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 7b311d6..7fd61b7 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -143,7 +143,6 @@
assert(!ssl->server);
assert(SSL_IS_DTLS(ssl));
- ERR_clear_error();
ERR_clear_system_error();
if (ssl->info_callback != NULL) {
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 81079f4..60057a8 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -272,6 +272,9 @@
}
int DTLSv1_handle_timeout(SSL *ssl) {
+ /* Functions which use SSL_get_error must clear the error queue on entry. */
+ ERR_clear_error();
+
if (!SSL_IS_DTLS(ssl)) {
return -1;
}
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index 5c9624a..38b5557 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -141,7 +141,6 @@
assert(ssl->server);
assert(SSL_IS_DTLS(ssl));
- ERR_clear_error();
ERR_clear_system_error();
if (ssl->info_callback != NULL) {
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 1cabde0..869e5ac 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -182,7 +182,6 @@
assert(!ssl->server);
assert(!SSL_IS_DTLS(ssl));
- ERR_clear_error();
ERR_clear_system_error();
if (ssl->info_callback != NULL) {
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 9ed0e2e..cfaddc9 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -185,7 +185,6 @@
assert(ssl->server);
assert(!SSL_IS_DTLS(ssl));
- ERR_clear_error();
ERR_clear_system_error();
if (ssl->info_callback != NULL) {
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c1e6a00..d9cea99 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -550,6 +550,9 @@
BIO *SSL_get_wbio(const SSL *ssl) { return ssl->wbio; }
int SSL_do_handshake(SSL *ssl) {
+ /* Functions which use SSL_get_error must clear the error queue on entry. */
+ ERR_clear_error();
+
if (ssl->handshake_func == NULL) {
OPENSSL_PUT_ERROR(SSL, SSL_R_CONNECTION_TYPE_NOT_SET);
return -1;
@@ -568,12 +571,9 @@
SSL_set_connect_state(ssl);
}
- if (ssl->handshake_func != ssl->method->ssl_connect) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return -1;
- }
+ assert(ssl->handshake_func == ssl->method->ssl_connect);
- return ssl->handshake_func(ssl);
+ return SSL_do_handshake(ssl);
}
int SSL_accept(SSL *ssl) {
@@ -582,15 +582,15 @@
SSL_set_accept_state(ssl);
}
- if (ssl->handshake_func != ssl->method->ssl_accept) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
- return -1;
- }
+ assert(ssl->handshake_func == ssl->method->ssl_accept);
- return ssl->handshake_func(ssl);
+ return SSL_do_handshake(ssl);
}
-int SSL_read(SSL *ssl, void *buf, int num) {
+static int ssl_read_impl(SSL *ssl, void *buf, int num, int peek) {
+ /* Functions which use SSL_get_error must clear the error queue on entry. */
+ ERR_clear_error();
+
if (ssl->handshake_func == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNINITIALIZED);
return -1;
@@ -602,24 +602,21 @@
}
ERR_clear_system_error();
- return ssl->method->ssl_read_app_data(ssl, buf, num, 0);
+ return ssl->method->ssl_read_app_data(ssl, buf, num, peek);
+}
+
+int SSL_read(SSL *ssl, void *buf, int num) {
+ return ssl_read_impl(ssl, buf, num, 0 /* consume bytes */);
}
int SSL_peek(SSL *ssl, void *buf, int num) {
- if (ssl->handshake_func == 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_UNINITIALIZED);
- return -1;
- }
-
- if (ssl->shutdown & SSL_RECEIVED_SHUTDOWN) {
- return 0;
- }
-
- ERR_clear_system_error();
- return ssl->method->ssl_read_app_data(ssl, buf, num, 1);
+ return ssl_read_impl(ssl, buf, num, 1 /* peek */);
}
int SSL_write(SSL *ssl, const void *buf, int num) {
+ /* Functions which use SSL_get_error must clear the error queue on entry. */
+ ERR_clear_error();
+
if (ssl->handshake_func == 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_UNINITIALIZED);
return -1;
@@ -636,6 +633,9 @@
}
int SSL_shutdown(SSL *ssl) {
+ /* Functions which use SSL_get_error must clear the error queue on entry. */
+ ERR_clear_error();
+
/* Note that this function behaves differently from what one might expect.
* Return values are 0 for no success (yet), 1 for success; but calling it
* once is usually not enough, even if blocking I/O is used (see
