Remove the push argument to ssl_init_wbio_buffer.
Having bbio be tri-state (not allocated, allocated but not active, and
allocated and active) is confusing.
The extra state is only used in the client handshake, where ClientHello is
special-cased to not go through the buffer while everything else is. This dates
to OpenSSL's initial commit and doesn't seem to do much. I do not believe it
can affect renego as the buffer only affects writes; although OpenSSL accepted
interleave on read (though this logic predates it slightly), it never sent
application data while it believed a handshake was active. The handshake would
always be driven to completion first.
My guess is this was to save a copy since the ClientHello is a one-message
flight so it wouldn't need to be buffered? This is probably not worth the extra
variation in the state. (Especially with the DTLS state machine going through
ClientHello twice and pushing the BIO in between the two. Though I suspect that
was a mistake in itself. If the optimization guess is correct, there was no
need to do that.)
Change-Id: I6726f866e16ee7213cab0c3e6abb133981444d47
Reviewed-on: https://boringssl-review.googlesource.com/7873
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index e14eae7..9f60db4 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -744,7 +744,7 @@
/* Ensure we are packing handshake messages. */
const int was_buffered = ssl_is_wbio_buffered(ssl);
assert(was_buffered == SSL_in_init(ssl));
- if (!was_buffered && !ssl_init_wbio_buffer(ssl, 1)) {
+ if (!was_buffered && !ssl_init_wbio_buffer(ssl)) {
return -1;
}
assert(ssl_is_wbio_buffered(ssl));
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 2fc9094..f62aae2 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -169,13 +169,11 @@
buf = NULL;
}
- if (!ssl_init_wbio_buffer(ssl, 0)) {
+ if (!ssl_init_wbio_buffer(ssl)) {
ret = -1;
goto end;
}
- /* don't push the buffering BIO quite yet */
-
ssl->state = SSL3_ST_CW_CLNT_HELLO_A;
ssl->init_num = 0;
ssl->d1->send_cookie = 0;
@@ -192,17 +190,13 @@
}
if (ssl->d1->send_cookie) {
- ssl->state = SSL3_ST_CW_FLUSH;
ssl->s3->tmp.next_state = SSL3_ST_CR_SRVR_HELLO_A;
} else {
- ssl->state = DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
+ ssl->s3->tmp.next_state = DTLS1_ST_CR_HELLO_VERIFY_REQUEST_A;
}
+ ssl->state = SSL3_ST_CW_FLUSH;
ssl->init_num = 0;
- /* turn on buffering for the next lot of output */
- if (ssl->bbio != ssl->wbio) {
- ssl->wbio = BIO_push(ssl->bbio, ssl->wbio);
- }
break;
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index d353281..093694b 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -168,7 +168,7 @@
ssl->init_num = 0;
- if (!ssl_init_wbio_buffer(ssl, 1)) {
+ if (!ssl_init_wbio_buffer(ssl)) {
ret = -1;
goto end;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 2c40b44..ce9175c 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1140,7 +1140,7 @@
* otherwise. */
int ssl_is_wbio_buffered(const SSL *ssl);
-int ssl_init_wbio_buffer(SSL *ssl, int push);
+int ssl_init_wbio_buffer(SSL *ssl);
void ssl_free_wbio_buffer(SSL *ssl);
int tls1_change_cipher_state(SSL *ssl, int which);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 6f381cf..76e2e93 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -209,13 +209,11 @@
buf = NULL;
}
- if (!ssl_init_wbio_buffer(ssl, 0)) {
+ if (!ssl_init_wbio_buffer(ssl)) {
ret = -1;
goto end;
}
- /* don't push the buffering BIO quite yet */
-
if (!ssl3_init_handshake_buffer(ssl)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
ret = -1;
@@ -233,14 +231,9 @@
if (ret <= 0) {
goto end;
}
- ssl->state = SSL3_ST_CR_SRVR_HELLO_A;
+ ssl->s3->tmp.next_state = SSL3_ST_CR_SRVR_HELLO_A;
+ ssl->state = SSL3_ST_CW_FLUSH;
ssl->init_num = 0;
-
- /* turn on buffering for the next lot of output */
- if (ssl->bbio != ssl->wbio) {
- ssl->wbio = BIO_push(ssl->bbio, ssl->wbio);
- }
-
break;
case SSL3_ST_CR_SRVR_HELLO_A:
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index f06ee56..587be03 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -219,7 +219,7 @@
/* Enable a write buffer. This groups handshake messages within a flight
* into a single write. */
- if (!ssl_init_wbio_buffer(ssl, 1)) {
+ if (!ssl_init_wbio_buffer(ssl)) {
ret = -1;
goto end;
}
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 70bd832..3135db6 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1869,7 +1869,7 @@
return ssl->bbio != NULL && ssl->wbio == ssl->bbio;
}
-int ssl_init_wbio_buffer(SSL *ssl, int push) {
+int ssl_init_wbio_buffer(SSL *ssl) {
BIO *bbio;
if (ssl->bbio == NULL) {
@@ -1891,14 +1891,8 @@
return 0;
}
- if (push) {
- if (ssl->wbio != bbio) {
- ssl->wbio = BIO_push(bbio, ssl->wbio);
- }
- } else {
- if (ssl->wbio == bbio) {
- ssl->wbio = BIO_pop(bbio);
- }
+ if (ssl->wbio != bbio) {
+ ssl->wbio = BIO_push(bbio, ssl->wbio);
}
return 1;
