From 6a08da2cf8c4bb506ccc97a0b702788555348435 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 8 May 2015 22:58:12 -0400 Subject: [PATCH] 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 --- ssl/d1_clnt.c | 3 +-- ssl/d1_pkt.c | 5 ----- ssl/d1_srvr.c | 5 ----- ssl/internal.h | 1 - ssl/s3_both.c | 9 --------- ssl/s3_clnt.c | 3 +-- ssl/s3_pkt.c | 28 ++++++++++------------------ ssl/s3_srvr.c | 32 ++++++++++---------------------- 8 files changed, 22 insertions(+), 64 deletions(-) diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c index c063247c..1827a675 100644 --- a/ssl/d1_clnt.c +++ b/ssl/d1_clnt.c @@ -176,8 +176,7 @@ int dtls1_connect(SSL *s) { 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 c169cb11..9e056ac9 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -446,11 +446,6 @@ int dtls1_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) { } } - 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 3415f985..e314910f 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c @@ -179,11 +179,6 @@ int dtls1_accept(SSL *s) { 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 430b323c..3bd749db 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -861,7 +861,6 @@ int ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk); 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 a9b20ca1..b78f6d39 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -654,15 +654,6 @@ err: 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 f42a9ad3..d01acaed 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -224,8 +224,7 @@ int ssl3_connect(SSL *s) { 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 913c3ba5..c42d0009 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -272,19 +272,6 @@ static int ssl3_get_record(SSL *s) { 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 @@ again: } 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 @@ again: } /* 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 @@ int ssl3_read_bytes(SSL *s, int type, uint8_t *buf, int len, int peek) { } } - 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 77286229..3cc30328 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -227,11 +227,6 @@ int ssl3_accept(SSL *s) { } 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 @@ int ssl3_accept(SSL *s) { } 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 @@ int ssl3_accept(SSL *s) { } 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 @@ int ssl3_get_initial_bytes(SSL *s) { 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 @@ int ssl3_get_v2_client_hello(SSL *s) { /* 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;