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;
