Remove redundant setup buffer calls. Nothing should call ssl3_setup_read_buffer or ssl3_setup_write_buffer unless it intends to write into the buffer. This way buffer management can later be an implementation detail of the record layer. Change-Id: Idb0effba00e77c6169764843793f40ec37868b61 Reviewed-on: https://boringssl-review.googlesource.com/4687 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c index c063247..1827a67 100644 --- a/ssl/d1_clnt.c +++ b/ssl/d1_clnt.c
@@ -176,8 +176,7 @@ buf = NULL; } - if (!ssl3_setup_buffers(s) || - !ssl_init_wbio_buffer(s, 0)) { + if (!ssl_init_wbio_buffer(s, 0)) { ret = -1; goto end; }
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index c169cb1..9e056ac 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c
@@ -446,11 +446,6 @@ } } - if (s->s3->rbuf.buf == NULL && !ssl3_setup_buffers(s)) { - /* TODO(davidben): Is this redundant with the calls in the handshake? */ - return -1; - } - start: s->rwstate = SSL_NOTHING;
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index 3415f98..e314910 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c
@@ -179,11 +179,6 @@ buf = NULL; } - if (!ssl3_setup_buffers(s)) { - ret = -1; - goto end; - } - s->init_num = 0; if (s->state != SSL_ST_RENEGOTIATE) {
diff --git a/ssl/internal.h b/ssl/internal.h index 430b323..3bd749d 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -861,7 +861,6 @@ const SSL_CIPHER *ssl3_choose_cipher( SSL *ssl, STACK_OF(SSL_CIPHER) *clnt, struct ssl_cipher_preference_list_st *srvr); -int ssl3_setup_buffers(SSL *s); int ssl3_setup_read_buffer(SSL *s); int ssl3_setup_write_buffer(SSL *s); int ssl3_release_read_buffer(SSL *s);
diff --git a/ssl/s3_both.c b/ssl/s3_both.c index a9b20ca..b78f6d3 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c
@@ -654,15 +654,6 @@ return 0; } - -int ssl3_setup_buffers(SSL *s) { - if (!ssl3_setup_read_buffer(s) || - !ssl3_setup_write_buffer(s)) { - return 0; - } - return 1; -} - int ssl3_release_write_buffer(SSL *s) { OPENSSL_free(s->s3->wbuf.buf); s->s3->wbuf.buf = NULL;
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index f42a9ad..d01acae 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c
@@ -224,8 +224,7 @@ buf = NULL; } - if (!ssl3_setup_buffers(s) || - !ssl_init_wbio_buffer(s, 0)) { + if (!ssl_init_wbio_buffer(s, 0)) { ret = -1; goto end; }
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index 913c3ba..c42d000 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c
@@ -272,19 +272,6 @@ rr = &s->s3->rrec; - if (s->options & SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER) { - extra = SSL3_RT_MAX_EXTRA; - } else { - extra = 0; - } - - if (extra && !s->s3->init_extra) { - /* An application error: SSL_OP_MICROSOFT_BIG_SSLV3_BUFFER set after - * ssl3_setup_buffers() was done */ - OPENSSL_PUT_ERROR(SSL, ssl3_get_record, ERR_R_INTERNAL_ERROR); - return -1; - } - again: /* check if we have the header */ if (s->rstate != SSL_ST_READ_BODY || @@ -295,6 +282,11 @@ } s->rstate = SSL_ST_READ_BODY; + /* Some bytes were read, so the read buffer must be existant and + * |s->s3->init_extra| is defined. */ + assert(s->s3->rbuf.buf != NULL); + extra = s->s3->init_extra ? SSL3_RT_MAX_EXTRA : 0; + p = s->packet; if (s->msg_callback) { s->msg_callback(0, 0, SSL3_RT_HEADER, p, 5, s, s->msg_callback_arg); @@ -325,6 +317,11 @@ } /* now s->rstate == SSL_ST_READ_BODY */ + } else { + /* |packet_length| is non-zero and |s->rstate| is |SSL_ST_READ_BODY|. The + * read buffer must be existant and |s->s3->init_extra| is defined. */ + assert(s->s3->rbuf.buf != NULL); + extra = s->s3->init_extra ? SSL3_RT_MAX_EXTRA : 0; } /* s->rstate == SSL_ST_READ_BODY, get and decode the data */ @@ -778,11 +775,6 @@ } } - if (s->s3->rbuf.buf == NULL && !ssl3_setup_read_buffer(s)) { - /* TODO(davidben): Is this redundant with the calls in the handshake? */ - return -1; - } - start: s->rwstate = SSL_NOTHING;
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 7728622..3cc3032 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c
@@ -227,11 +227,6 @@ } s->init_num = 0; - if (!ssl3_setup_buffers(s)) { - ret = -1; - goto end; - } - if (!s->s3->send_connection_binding && !(s->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) { /* Server attempting to renegotiate with client that doesn't support @@ -296,6 +291,13 @@ } s->init_num = 0; + /* Enable a write buffer. This groups handshake messages within a flight + * into a single write. */ + if (!ssl_init_wbio_buffer(s, 1)) { + ret = -1; + goto end; + } + if (!ssl3_init_finished_mac(s)) { OPENSSL_PUT_ERROR(SSL, ssl3_accept, ERR_R_INTERNAL_ERROR); ret = -1; @@ -303,19 +305,8 @@ } if (!s->s3->have_version) { - /* This is the initial handshake. The record layer has not been - * initialized yet. Sniff for a V2ClientHello before reading a - * ClientHello normally. */ - assert(s->s3->rbuf.buf == NULL); - assert(s->s3->wbuf.buf == NULL); s->state = SSL3_ST_SR_INITIAL_BYTES; } else { - /* Enable a write buffer. This groups handshake messages within a - * flight into a single write. */ - if (!ssl3_setup_buffers(s) || !ssl_init_wbio_buffer(s, 1)) { - ret = -1; - goto end; - } s->state = SSL3_ST_SR_CLNT_HELLO_A; } break; @@ -751,10 +742,12 @@ p[5] == SSL3_MT_CLIENT_HELLO) { /* This is a ClientHello. Initialize the record layer with the already * consumed data and continue the handshake. */ - if (!ssl3_setup_buffers(s) || !ssl_init_wbio_buffer(s, 1)) { + if (!ssl3_setup_read_buffer(s)) { return -1; } assert(s->rstate == SSL_ST_READ_HEADER); + /* There cannot have already been data in the record layer. */ + assert(s->s3->rbuf.left == 0); memcpy(s->s3->rbuf.buf, p, s->s3->sniff_buffer_len); s->s3->rbuf.offset = 0; s->s3->rbuf.left = s->s3->sniff_buffer_len; @@ -897,11 +890,6 @@ /* The handshake message header is 4 bytes. */ s->s3->tmp.message_size = len - 4; - /* Initialize the record layer. */ - if (!ssl3_setup_buffers(s) || !ssl_init_wbio_buffer(s, 1)) { - return -1; - } - /* Drop the sniff buffer. */ BUF_MEM_free(s->s3->sniff_buffer); s->s3->sniff_buffer = NULL;