Set rwstate consistently.
We reset it to SSL_NOTHING at the start of ever SSL_get_error-using operation.
Then we only set it to a non-NOTHING value in the rest of the stack on error
paths.
Currently, ssl->rwstate is set all over the place. Sometimes the pattern is:
ssl->rwstate = SSL_WRITING;
if (BIO_write(...) <= 0) {
goto err;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we only set it to the non-NOTHING value on error.
if (BIO_write(...) <= 0) {
ssl->rwstate = SSL_WRITING;
}
ssl->rwstate = SSL_NOTHING;
Sometimes we just set it to SSL_NOTHING far from any callback in random places.
The third case is arbitrary and clearly should be removed.
But, in the second case, we sometimes forget to undo it afterwards. This is
largely harmless since an error in the error queue overrides rwstate, but we
don't always put something in the error queue (falling back to
SSL_ERROR_SYSCALL for "I'm not sure why it failed. Perhaps it was one of your
callbacks? Check your errno equivalent."), but in that case a stray rwstate
value will cause it to be wrong.
We could fix the cases where we fail to set SSL_NOTHING on success cases, but
this doesn't account for there being multiple SSL_get_error operations. The
consumer may have an SSL_read and an SSL_write running concurrently. Instead,
it seems the best option is to lift the SSL_NOTHING reset to the operations and
set SSL_WRITING and friends as in the second case.
(Someday hopefully we can fix this to just be an enum that is internally
returned. It can convert to something stateful at the API layer.)
Change-Id: I54665ec066a64eb0e48a06e2fcd0d2681a42df7f
Reviewed-on: https://boringssl-review.googlesource.com/7453
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index 8596f32..cc95a70 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -290,12 +290,11 @@
/* During the handshake, wbio is buffered to pack messages together. Flush the
* buffer if the ChangeCipherSpec would not fit in a packet. */
if (dtls1_max_record_size(ssl) == 0) {
- ssl->rwstate = SSL_WRITING;
int ret = BIO_flush(SSL_get_wbio(ssl));
if (ret <= 0) {
+ ssl->rwstate = SSL_WRITING;
return ret;
}
- ssl->rwstate = SSL_NOTHING;
}
static const uint8_t kChangeCipherSpec[1] = {SSL3_MT_CCS};
@@ -340,13 +339,12 @@
/* During the handshake, wbio is buffered to pack messages together. Flush
* the buffer if there isn't enough room to make progress. */
if (dtls1_max_record_size(ssl) < DTLS1_HM_HEADER_LENGTH + 1) {
- ssl->rwstate = SSL_WRITING;
int flush_ret = BIO_flush(SSL_get_wbio(ssl));
if (flush_ret <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = flush_ret;
goto err;
}
- ssl->rwstate = SSL_NOTHING;
assert(BIO_wpending(SSL_get_wbio(ssl)) == 0);
}
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index b29d56c..2fc9094 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -447,12 +447,11 @@
break;
case SSL3_ST_CW_FLUSH:
- ssl->rwstate = SSL_WRITING;
if (BIO_flush(ssl->wbio) <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = -1;
goto end;
}
- ssl->rwstate = SSL_NOTHING;
ssl->state = ssl->s3->tmp.next_state;
break;
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 4298580..e48fbf1 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -271,6 +271,7 @@
}
int DTLSv1_handle_timeout(SSL *ssl) {
+ ssl->rwstate = SSL_NOTHING;
/* Functions which use SSL_get_error must clear the error queue on entry. */
ERR_clear_error();
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 72ef8ac..4690486 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -246,8 +246,6 @@
}
start:
- ssl->rwstate = SSL_NOTHING;
-
/* ssl->s3->rrec.type - is the type of record
* ssl->s3->rrec.data - data
* ssl->s3->rrec.off - offset into 'data' for next read
@@ -279,7 +277,6 @@
* 'peek' mode) */
if (ssl->shutdown & SSL_RECEIVED_SHUTDOWN) {
rr->length = 0;
- ssl->rwstate = SSL_NOTHING;
return 0;
}
@@ -364,7 +361,6 @@
} else if (alert_level == SSL3_AL_FATAL) {
char tmp[16];
- ssl->rwstate = SSL_NOTHING;
OPENSSL_PUT_ERROR(SSL, SSL_AD_REASON_OFFSET + alert_descr);
BIO_snprintf(tmp, sizeof tmp, "%d", alert_descr);
ERR_add_error_data(2, "SSL alert number ", tmp);
@@ -462,7 +458,6 @@
int dtls1_write_bytes(SSL *ssl, int type, const void *buf, int len,
enum dtls1_use_epoch_t use_epoch) {
assert(len <= SSL3_RT_MAX_PLAIN_LENGTH);
- ssl->rwstate = SSL_NOTHING;
return do_dtls1_write(ssl, type, buf, len, use_epoch);
}
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index fef8541..d353281 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -302,12 +302,11 @@
break;
case SSL3_ST_SW_FLUSH:
- ssl->rwstate = SSL_WRITING;
if (BIO_flush(ssl->wbio) <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = -1;
goto end;
}
- ssl->rwstate = SSL_NOTHING;
ssl->state = ssl->s3->tmp.next_state;
break;
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 7bb31de..5d364ab 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -393,7 +393,6 @@
int bytes_read =
ssl3_read_bytes(ssl, SSL3_RT_HANDSHAKE, &p[ssl->init_num], n, 0);
if (bytes_read <= 0) {
- ssl->rwstate = SSL_READING;
*ok = 0;
return bytes_read;
}
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 8dd4abd..6f381cf 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -501,12 +501,11 @@
break;
case SSL3_ST_CW_FLUSH:
- ssl->rwstate = SSL_WRITING;
if (BIO_flush(ssl->wbio) <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = -1;
goto end;
}
- ssl->rwstate = SSL_NOTHING;
ssl->state = ssl->s3->tmp.next_state;
break;
@@ -1823,10 +1822,8 @@
switch (sign_result) {
case ssl_private_key_success:
- ssl->rwstate = SSL_NOTHING;
break;
case ssl_private_key_failure:
- ssl->rwstate = SSL_NOTHING;
goto err;
case ssl_private_key_retry:
ssl->rwstate = SSL_PRIVATE_KEY_OPERATION;
@@ -1868,7 +1865,6 @@
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return -1;
}
- ssl->rwstate = SSL_NOTHING;
}
if (ssl3_has_client_certificate(ssl)) {
@@ -1887,7 +1883,6 @@
ssl->rwstate = SSL_X509_LOOKUP;
return -1;
}
- ssl->rwstate = SSL_NOTHING;
int setup_error = ret == 1 && (!SSL_use_certificate(ssl, x509) ||
!SSL_use_PrivateKey(ssl, pkey));
@@ -1984,7 +1979,6 @@
ssl->rwstate = SSL_CHANNEL_ID_LOOKUP;
return -1;
}
- ssl->rwstate = SSL_NOTHING;
EC_KEY *ec_key = EVP_PKEY_get0_EC_KEY(ssl->tlsext_channel_id_private);
if (ec_key == NULL) {
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 9038fda..d9c21d4 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -193,7 +193,6 @@
const uint8_t *buf = buf_;
unsigned tot, n, nw;
- ssl->rwstate = SSL_NOTHING;
assert(ssl->s3->wnum <= INT_MAX);
tot = ssl->s3->wnum;
ssl->s3->wnum = 0;
@@ -378,8 +377,6 @@
}
start:
- ssl->rwstate = SSL_NOTHING;
-
/* ssl->s3->rrec.type - is the type of record
* ssl->s3->rrec.data - data
* ssl->s3->rrec.off - offset into 'data' for next read
@@ -400,7 +397,6 @@
* 'peek' mode) */
if (ssl->shutdown & SSL_RECEIVED_SHUTDOWN) {
rr->length = 0;
- ssl->rwstate = SSL_NOTHING;
return 0;
}
@@ -564,7 +560,6 @@
} else if (alert_level == SSL3_AL_FATAL) {
char tmp[16];
- ssl->rwstate = SSL_NOTHING;
OPENSSL_PUT_ERROR(SSL, SSL_AD_REASON_OFFSET + alert_descr);
BIO_snprintf(tmp, sizeof(tmp), "%d", alert_descr);
ERR_add_error_data(2, "SSL alert number ", tmp);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 0821b3a..f06ee56 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -371,12 +371,11 @@
* in PR#1939. The proposed fix doesn't completely resolve this issue
* as buggy implementations of BIO_CTRL_PENDING still exist. So instead
* we just flush unconditionally. */
- ssl->rwstate = SSL_WRITING;
if (BIO_flush(ssl->wbio) <= 0) {
+ ssl->rwstate = SSL_WRITING;
ret = -1;
goto end;
}
- ssl->rwstate = SSL_NOTHING;
ssl->state = ssl->s3->tmp.next_state;
break;
@@ -1036,7 +1035,6 @@
ssl->rwstate = SSL_X509_LOOKUP;
goto err;
}
- ssl->rwstate = SSL_NOTHING;
}
c = ssl3_choose_cipher(ssl, ciphers, ssl_get_cipher_preferences(ssl));
@@ -1341,13 +1339,11 @@
switch (sign_result) {
case ssl_private_key_success:
- ssl->rwstate = SSL_NOTHING;
if (!CBB_did_write(&child, sig_len)) {
goto err;
}
break;
case ssl_private_key_failure:
- ssl->rwstate = SSL_NOTHING;
goto err;
case ssl_private_key_retry:
/* Discard the unfinished signature and save the state of |cbb| for the
@@ -1564,10 +1560,8 @@
switch (decrypt_result) {
case ssl_private_key_success:
- ssl->rwstate = SSL_NOTHING;
break;
case ssl_private_key_failure:
- ssl->rwstate = SSL_NOTHING;
goto err;
case ssl_private_key_retry:
ssl->rwstate = SSL_PRIVATE_KEY_OPERATION;
diff --git a/ssl/ssl_buffer.c b/ssl/ssl_buffer.c
index 7fd74e4..272b13b 100644
--- a/ssl/ssl_buffer.c
+++ b/ssl/ssl_buffer.c
@@ -113,12 +113,11 @@
}
/* Read a single packet from |ssl->rbio|. |buf->cap| must fit in an int. */
- ssl->rwstate = SSL_READING;
int ret = BIO_read(ssl->rbio, buf->buf + buf->offset, (int)buf->cap);
if (ret <= 0) {
+ ssl->rwstate = SSL_READING;
return ret;
}
- ssl->rwstate = SSL_NOTHING;
/* |BIO_read| was bound by |buf->cap|, so this cannot overflow. */
buf->len = (uint16_t)ret;
return 1;
@@ -136,13 +135,12 @@
while (buf->len < len) {
/* The amount of data to read is bounded by |buf->cap|, which must fit in an
* int. */
- ssl->rwstate = SSL_READING;
int ret = BIO_read(ssl->rbio, buf->buf + buf->offset + buf->len,
(int)(len - buf->len));
if (ret <= 0) {
+ ssl->rwstate = SSL_READING;
return ret;
}
- ssl->rwstate = SSL_NOTHING;
/* |BIO_read| was bound by |buf->cap - buf->len|, so this cannot
* overflow. */
buf->len += (uint16_t)ret;
@@ -268,12 +266,11 @@
SSL3_BUFFER *buf = &ssl->s3->write_buffer;
while (buf->len > 0) {
- ssl->rwstate = SSL_WRITING;
int ret = BIO_write(ssl->wbio, buf->buf + buf->offset, buf->len);
if (ret <= 0) {
+ ssl->rwstate = SSL_WRITING;
return ret;
}
- ssl->rwstate = SSL_NOTHING;
consume_buffer(buf, (size_t)ret);
}
ssl_write_buffer_clear(ssl);
@@ -286,16 +283,15 @@
return 1;
}
- ssl->rwstate = SSL_WRITING;
int ret = BIO_write(ssl->wbio, buf->buf + buf->offset, buf->len);
if (ret <= 0) {
+ ssl->rwstate = SSL_WRITING;
/* If the write failed, drop the write buffer anyway. Datagram transports
* can't write half a packet, so the caller is expected to retry from the
* top. */
ssl_write_buffer_clear(ssl);
return ret;
}
- ssl->rwstate = SSL_NOTHING;
ssl_write_buffer_clear(ssl);
return 1;
}
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 1d4ccb7..d62cdae 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -548,6 +548,7 @@
BIO *SSL_get_wbio(const SSL *ssl) { return ssl->wbio; }
int SSL_do_handshake(SSL *ssl) {
+ ssl->rwstate = SSL_NOTHING;
/* Functions which use SSL_get_error must clear the error queue on entry. */
ERR_clear_error();
@@ -586,6 +587,7 @@
}
static int ssl_read_impl(SSL *ssl, void *buf, int num, int peek) {
+ ssl->rwstate = SSL_NOTHING;
/* Functions which use SSL_get_error must clear the error queue on entry. */
ERR_clear_error();
ERR_clear_system_error();
@@ -596,7 +598,6 @@
}
if (ssl->shutdown & SSL_RECEIVED_SHUTDOWN) {
- ssl->rwstate = SSL_NOTHING;
return 0;
}
@@ -626,6 +627,7 @@
}
int SSL_write(SSL *ssl, const void *buf, int num) {
+ ssl->rwstate = SSL_NOTHING;
/* Functions which use SSL_get_error must clear the error queue on entry. */
ERR_clear_error();
ERR_clear_system_error();
@@ -636,7 +638,6 @@
}
if (ssl->shutdown & SSL_SENT_SHUTDOWN) {
- ssl->rwstate = SSL_NOTHING;
OPENSSL_PUT_ERROR(SSL, SSL_R_PROTOCOL_IS_SHUTDOWN);
return -1;
}
@@ -657,6 +658,7 @@
}
int SSL_shutdown(SSL *ssl) {
+ ssl->rwstate = SSL_NOTHING;
/* Functions which use SSL_get_error must clear the error queue on entry. */
ERR_clear_error();
