Switch finish_handshake to release_current_message. With the previous DTLS change, the dispatch layer only cares about the end of the handshake to know when to drop the current message. TLS 1.3 post-handshake messages will need a similar hook, so convert it to this lower-level one. BUG=83 Change-Id: I4c8c3ba55ba793afa065bf261a7bccac8816c348 Reviewed-on: https://boringssl-review.googlesource.com/8989 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/d1_both.c b/ssl/d1_both.c index 1d9cd75..712c63b 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c
@@ -262,17 +262,6 @@ return frag != NULL && frag->reassembly == NULL; } -/* dtls1_pop_message removes the current handshake message, which must be - * complete, and advances to the next one. */ -static void dtls1_pop_message(SSL *ssl) { - assert(dtls1_is_current_message_complete(ssl)); - - size_t index = ssl->d1->handshake_read_seq % SSL_MAX_HANDSHAKE_FLIGHT; - dtls1_hm_fragment_free(ssl->d1->incoming_messages[index]); - ssl->d1->incoming_messages[index] = NULL; - ssl->d1->handshake_read_seq++; -} - /* dtls1_get_incoming_message returns the incoming message corresponding to * |msg_hdr|. If none exists, it creates a new one and inserts it in the * queue. Otherwise, it checks |msg_hdr| is consistent with the existing one. It @@ -412,12 +401,12 @@ * ssl_dont_hash_message would have to have been applied to the previous * call. */ assert(hash_message == ssl_hash_message); - assert(dtls1_is_current_message_complete(ssl)); + assert(ssl->init_msg != NULL); ssl->s3->tmp.reuse_message = 0; hash_message = ssl_dont_hash_message; - } else if (dtls1_is_current_message_complete(ssl)) { - dtls1_pop_message(ssl); + } else { + dtls1_release_current_message(ssl, 0 /* don't free buffer */); } /* Process handshake records until the current message is ready. */ @@ -463,6 +452,21 @@ DTLS1_HM_HEADER_LENGTH + frag->msg_len); } +void dtls1_release_current_message(SSL *ssl, int free_buffer) { + if (ssl->init_msg == NULL) { + return; + } + + assert(dtls1_is_current_message_complete(ssl)); + size_t index = ssl->d1->handshake_read_seq % SSL_MAX_HANDSHAKE_FLIGHT; + dtls1_hm_fragment_free(ssl->d1->incoming_messages[index]); + ssl->d1->incoming_messages[index] = NULL; + ssl->d1->handshake_read_seq++; + + ssl->init_msg = NULL; + ssl->init_num = 0; +} + void dtls_clear_incoming_messages(SSL *ssl) { for (size_t i = 0; i < SSL_MAX_HANDSHAKE_FLIGHT; i++) { dtls1_hm_fragment_free(ssl->d1->incoming_messages[i]); @@ -471,13 +475,14 @@ } int dtls_has_incoming_messages(const SSL *ssl) { - /* This function may not be called if there is a pending |dtls1_get_message| - * operation. */ - assert(dtls1_is_current_message_complete(ssl)); - size_t current = ssl->d1->handshake_read_seq % SSL_MAX_HANDSHAKE_FLIGHT; for (size_t i = 0; i < SSL_MAX_HANDSHAKE_FLIGHT; i++) { - if (i != current && ssl->d1->incoming_messages[i] != NULL) { + /* Skip the current message. */ + if (ssl->init_msg != NULL && i == current) { + assert(dtls1_is_current_message_complete(ssl)); + continue; + } + if (ssl->d1->incoming_messages[i] != NULL) { return 1; } }
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index ceb6e17..ab9b911 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c
@@ -224,7 +224,7 @@ } if (msg_hdr.type == SSL3_MT_FINISHED && - msg_hdr.seq == ssl->d1->handshake_read_seq) { + msg_hdr.seq == ssl->d1->handshake_read_seq - 1) { if (msg_hdr.frag_off == 0) { /* Retransmit our last flight of messages. If the peer sends the second * Finished, they may not have received ours. Only do this for the
diff --git a/ssl/dtls_method.c b/ssl/dtls_method.c index 09bd455..e2e1726 100644 --- a/ssl/dtls_method.c +++ b/ssl/dtls_method.c
@@ -92,12 +92,6 @@ return ~(version - 0x0201); } -static void dtls1_finish_handshake(SSL *ssl) { - dtls_clear_incoming_messages(ssl); - ssl->init_msg = NULL; - ssl->init_num = 0; -} - static int dtls1_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) { /* Cipher changes are illegal when there are buffered incoming messages. */ if (dtls_has_incoming_messages(ssl)) { @@ -135,9 +129,9 @@ dtls1_version_to_wire, dtls1_new, dtls1_free, - dtls1_finish_handshake, dtls1_get_message, dtls1_hash_current_message, + dtls1_release_current_message, dtls1_read_app_data, dtls1_read_change_cipher_spec, dtls1_read_close_notify,
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index fc4318e..5fe706c 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c
@@ -502,10 +502,9 @@ break; case SSL_ST_OK: - /* clean a few things up */ + /* Clean a few things up. */ ssl3_cleanup_key_block(ssl); - - ssl->method->finish_handshake(ssl); + ssl->method->release_current_message(ssl, 1 /* free_buffer */); /* Remove write buffering now. */ ssl_free_wbio_buffer(ssl);
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index 4708dd0..9f4aae3 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c
@@ -471,10 +471,9 @@ break; case SSL_ST_OK: - /* clean a few things up */ + /* Clean a few things up. */ ssl3_cleanup_key_block(ssl); - - ssl->method->finish_handshake(ssl); + ssl->method->release_current_message(ssl, 1 /* free_buffer */); /* remove buffering on output */ ssl_free_wbio_buffer(ssl);
diff --git a/ssl/internal.h b/ssl/internal.h index 2fb1492..1b3f28a 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1039,8 +1039,6 @@ uint16_t (*version_to_wire)(uint16_t version); int (*ssl_new)(SSL *ssl); void (*ssl_free)(SSL *ssl); - /* finish_handshake is called when a handshake completes. */ - void (*finish_handshake)(SSL *ssl); /* ssl_get_message reads the next handshake message. If |msg_type| is not -1, * the message must have the specified type. On success, it returns one and * sets |ssl->s3->tmp.message_type|, |ssl->init_msg|, and |ssl->init_num|. @@ -1051,6 +1049,9 @@ * handshake hash. It returns one on success and zero on allocation * failure. */ int (*hash_current_message)(SSL *ssl); + /* release_current_message is called to release the current handshake message. + * If |free_buffer| is one, buffers will also be released. */ + void (*release_current_message)(SSL *ssl, int free_buffer); int (*read_app_data)(SSL *ssl, uint8_t *buf, int len, int peek); int (*read_change_cipher_spec)(SSL *ssl); void (*read_close_notify)(SSL *ssl); @@ -1242,6 +1243,7 @@ int ssl3_get_message(SSL *ssl, int msg_type, enum ssl_hash_message_t hash_message); int ssl3_hash_current_message(SSL *ssl); +void ssl3_release_current_message(SSL *ssl, int free_buffer); /* ssl3_cert_verify_hash writes the SSL 3.0 CertificateVerify hash into the * bytes pointed to by |out| and writes the number of bytes to |*out_len|. |out| @@ -1321,6 +1323,7 @@ int dtls1_get_message(SSL *ssl, int mt, enum ssl_hash_message_t hash_message); int dtls1_hash_current_message(SSL *ssl); +void dtls1_release_current_message(SSL *ssl, int free_buffer); int dtls1_dispatch_alert(SSL *ssl); /* ssl_is_wbio_buffered returns one if |ssl|'s write BIO is buffered and zero
diff --git a/ssl/s3_both.c b/ssl/s3_both.c index 11555dd..7608c34 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c
@@ -550,14 +550,8 @@ ssl->s3->tmp.reuse_message = 0; hash_message = ssl_dont_hash_message; - } else if (ssl->init_msg != NULL) { - /* |init_buf| never contains data beyond the current message. */ - assert(SSL3_HM_HEADER_LENGTH + ssl->init_num == ssl->init_buf->length); - - /* Clear the current message. */ - ssl->init_msg = NULL; - ssl->init_num = 0; - ssl->init_buf->length = 0; + } else { + ssl3_release_current_message(ssl, 0 /* don't free buffer */); } /* Read the message header, if we haven't yet. */ @@ -618,6 +612,23 @@ ssl->init_buf->length); } +void ssl3_release_current_message(SSL *ssl, int free_buffer) { + if (ssl->init_msg != NULL) { + /* |init_buf| never contains data beyond the current message. */ + assert(SSL3_HM_HEADER_LENGTH + ssl->init_num == ssl->init_buf->length); + + /* Clear the current message. */ + ssl->init_msg = NULL; + ssl->init_num = 0; + ssl->init_buf->length = 0; + } + + if (free_buffer) { + BUF_MEM_free(ssl->init_buf); + ssl->init_buf = NULL; + } +} + int ssl_verify_alarm_type(long type) { int al;
diff --git a/ssl/tls_method.c b/ssl/tls_method.c index c11c452..923541b 100644 --- a/ssl/tls_method.c +++ b/ssl/tls_method.c
@@ -69,14 +69,6 @@ static uint16_t ssl3_version_to_wire(uint16_t version) { return version; } -static void ssl3_finish_handshake(SSL *ssl) { - BUF_MEM_free(ssl->init_buf); - ssl->init_buf = NULL; - - ssl->init_msg = NULL; - ssl->init_num = 0; -} - static int ssl3_set_read_state(SSL *ssl, SSL_AEAD_CTX *aead_ctx) { if (ssl->s3->rrec.length != 0) { /* There may not be unprocessed record data at a cipher change. */ @@ -109,9 +101,9 @@ ssl3_version_to_wire, ssl3_new, ssl3_free, - ssl3_finish_handshake, ssl3_get_message, ssl3_hash_current_message, + ssl3_release_current_message, ssl3_read_app_data, ssl3_read_change_cipher_spec, ssl3_read_close_notify,