Remove buffering out records from the next epoch.
It was only ever enabled for handshake and alert messages. The comments cite
renego as a use case though even then I'm not clear on why. The only use I see
is if, say, the Finished message and ClientKeyExchange came in out-of-order.
DTLS is unreliable so leaning on retransmit seems fine, and usually flights
will be packed into one packet where possible. NSS doesn't have any such
buffer and doesn't seem to have problems.
The buffering mechanism is also rather dubious. It stows away the entire packet
and read buffer---all 16K of it---and there may have been other records in that
packet.
Change-Id: Ic3b7bf817be380dc73102eec62c690ed093e6667
Reviewed-on: https://boringssl-review.googlesource.com/4238
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h
index ad49aed..5d60519 100644
--- a/include/openssl/dtls1.h
+++ b/include/openssl/dtls1.h
@@ -135,9 +135,6 @@
/* records being received in the current epoch */
DTLS1_BITMAP bitmap;
- /* renegotiation starts a new set of sequence numbers */
- DTLS1_BITMAP next_bitmap;
-
/* handshake message numbers */
uint16_t handshake_write_seq;
uint16_t next_handshake_write_seq;
@@ -147,10 +144,6 @@
/* save last sequence number for retransmissions */
uint8_t last_write_sequence[8];
- /* Received handshake records (processed and unprocessed) */
- record_pqueue unprocessed_rcds;
- record_pqueue processed_rcds;
-
/* buffered_messages is a priority queue of incoming handshake messages that
* have yet to be processed.
*
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 356a637..61a862d 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -96,21 +96,12 @@
}
memset(d1, 0, sizeof *d1);
- d1->unprocessed_rcds.q = pqueue_new();
- d1->processed_rcds.q = pqueue_new();
d1->buffered_messages = pqueue_new();
d1->sent_messages = pqueue_new();
d1->buffered_app_data.q = pqueue_new();
- if (!d1->unprocessed_rcds.q || !d1->processed_rcds.q ||
- !d1->buffered_messages || !d1->sent_messages ||
+ if (!d1->buffered_messages || !d1->sent_messages ||
!d1->buffered_app_data.q) {
- if (d1->unprocessed_rcds.q) {
- pqueue_free(d1->unprocessed_rcds.q);
- }
- if (d1->processed_rcds.q) {
- pqueue_free(d1->processed_rcds.q);
- }
if (d1->buffered_messages) {
pqueue_free(d1->buffered_messages);
}
@@ -141,24 +132,6 @@
hm_fragment *frag = NULL;
DTLS1_RECORD_DATA *rdata;
- while ((item = pqueue_pop(s->d1->unprocessed_rcds.q)) != NULL) {
- rdata = (DTLS1_RECORD_DATA *)item->data;
- if (rdata->rbuf.buf) {
- OPENSSL_free(rdata->rbuf.buf);
- }
- OPENSSL_free(item->data);
- pitem_free(item);
- }
-
- while ((item = pqueue_pop(s->d1->processed_rcds.q)) != NULL) {
- rdata = (DTLS1_RECORD_DATA *)item->data;
- if (rdata->rbuf.buf) {
- OPENSSL_free(rdata->rbuf.buf);
- }
- OPENSSL_free(item->data);
- pitem_free(item);
- }
-
while ((item = pqueue_pop(s->d1->buffered_messages)) != NULL) {
frag = (hm_fragment *)item->data;
dtls1_hm_fragment_free(frag);
@@ -190,8 +163,6 @@
dtls1_clear_queues(s);
- pqueue_free(s->d1->unprocessed_rcds.q);
- pqueue_free(s->d1->processed_rcds.q);
pqueue_free(s->d1->buffered_messages);
pqueue_free(s->d1->sent_messages);
pqueue_free(s->d1->buffered_app_data.q);
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 766f8b2..56bdae0 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -183,8 +183,6 @@
static int dtls1_record_replay_check(SSL *s, DTLS1_BITMAP *bitmap);
static void dtls1_record_bitmap_update(SSL *s, DTLS1_BITMAP *bitmap);
-static DTLS1_BITMAP *dtls1_get_bitmap(SSL *s, SSL3_RECORD *rr,
- unsigned int *is_next_epoch);
static int dtls1_buffer_record(SSL *s, record_pqueue *q,
uint8_t *priority);
static int dtls1_process_record(SSL *s);
@@ -269,62 +267,6 @@
return -1;
}
-static int dtls1_retrieve_buffered_record(SSL *s, record_pqueue *queue) {
- pitem *item;
-
- item = pqueue_pop(queue->q);
- if (item) {
- dtls1_copy_record(s, item);
-
- OPENSSL_free(item->data);
- pitem_free(item);
-
- return 1;
- }
-
- return 0;
-}
-
-/* retrieve a buffered record that belongs to the new epoch, i.e., not
- * processed yet */
-#define dtls1_get_unprocessed_record(s) \
- dtls1_retrieve_buffered_record((s), &((s)->d1->unprocessed_rcds))
-
-/* retrieve a buffered record that belongs to the current epoch, i.e.,
- * processed */
-#define dtls1_get_processed_record(s) \
- dtls1_retrieve_buffered_record((s), &((s)->d1->processed_rcds))
-
-static int dtls1_process_buffered_records(SSL *s) {
- pitem *item;
-
- item = pqueue_peek(s->d1->unprocessed_rcds.q);
- if (item) {
- /* Check if epoch is current. */
- if (s->d1->unprocessed_rcds.epoch != s->d1->r_epoch) {
- return 1; /* Nothing to do. */
- }
-
- /* Process all the records. */
- while (pqueue_peek(s->d1->unprocessed_rcds.q)) {
- dtls1_get_unprocessed_record(s);
- if (!dtls1_process_record(s)) {
- return 0;
- }
- if (dtls1_buffer_record(s, &(s->d1->processed_rcds),
- s->s3->rrec.seq_num) < 0) {
- return -1;
- }
- }
- }
-
- /* sync epoch numbers once all the unprocessed records have been processed */
- s->d1->processed_rcds.epoch = s->d1->r_epoch;
- s->d1->unprocessed_rcds.epoch = s->d1->r_epoch + 1;
-
- return 1;
-}
-
static int dtls1_process_record(SSL *s) {
int al;
SSL3_RECORD *rr;
@@ -403,22 +345,9 @@
SSL3_RECORD *rr;
unsigned char *p = NULL;
unsigned short version;
- DTLS1_BITMAP *bitmap;
- unsigned int is_next_epoch;
rr = &(s->s3->rrec);
- /* The epoch may have changed. If so, process all the pending records. This
- * is a non-blocking operation. */
- if (dtls1_process_buffered_records(s) < 0) {
- return -1;
- }
-
- /* If we're renegotiating, then there may be buffered records. */
- if (dtls1_get_processed_record(s)) {
- return 1;
- }
-
/* get something from the wire */
again:
/* check if we have the header */
@@ -513,16 +442,17 @@
}
s->rstate = SSL_ST_READ_HEADER; /* set state for later operations */
- /* match epochs. NULL means the packet is dropped on the floor */
- bitmap = dtls1_get_bitmap(s, rr, &is_next_epoch);
- if (bitmap == NULL) {
+ if (rr->epoch != s->d1->r_epoch) {
+ /* This record is from the wrong epoch. If it is the next epoch, it could be
+ * buffered. For simplicity, drop it and expect retransmit to handle it
+ * later; DTLS is supposed to handle packet loss. */
rr->length = 0;
- s->packet_length = 0; /* dump this record */
- goto again; /* get another record */
+ s->packet_length = 0;
+ goto again;
}
/* Check whether this is a repeat, or aged record. */
- if (!dtls1_record_replay_check(s, bitmap)) {
+ if (!dtls1_record_replay_check(s, &s->d1->bitmap)) {
rr->length = 0;
s->packet_length = 0; /* dump this record */
goto again; /* get another record */
@@ -533,28 +463,12 @@
goto again;
}
- /* If this record is from the next epoch (either HM or ALERT),
- * and a handshake is currently in progress, buffer it since it
- * cannot be processed at this time.
- */
- if (is_next_epoch) {
- if (SSL_in_init(s) || s->in_handshake) {
- if (dtls1_buffer_record(s, &(s->d1->unprocessed_rcds), rr->seq_num) < 0) {
- return -1;
- }
- dtls1_record_bitmap_update(s, bitmap); /* Mark receipt of record. */
- }
- rr->length = 0;
- s->packet_length = 0;
- goto again;
- }
-
if (!dtls1_process_record(s)) {
rr->length = 0;
s->packet_length = 0; /* dump this record */
goto again; /* get another record */
}
- dtls1_record_bitmap_update(s, bitmap); /* Mark receipt of record. */
+ dtls1_record_bitmap_update(s, &s->d1->bitmap); /* Mark receipt of record. */
return 1;
}
@@ -1095,23 +1009,6 @@
return i;
}
-static DTLS1_BITMAP *dtls1_get_bitmap(SSL *s, SSL3_RECORD *rr,
- unsigned int *is_next_epoch) {
- *is_next_epoch = 0;
-
- /* In current epoch, accept HM, CCS, DATA, & ALERT */
- if (rr->epoch == s->d1->r_epoch) {
- return &s->d1->bitmap;
- } else if (rr->epoch == (unsigned long)(s->d1->r_epoch + 1) &&
- (rr->type == SSL3_RT_HANDSHAKE || rr->type == SSL3_RT_ALERT)) {
- /* Only HM and ALERT messages can be from the next epoch */
- *is_next_epoch = 1;
- return &s->d1->next_bitmap;
- }
-
- return NULL;
-}
-
void dtls1_reset_seq_numbers(SSL *s, int rw) {
uint8_t *seq;
unsigned int seq_bytes = sizeof(s->s3->read_sequence);
@@ -1119,8 +1016,7 @@
if (rw & SSL3_CC_READ) {
seq = s->s3->read_sequence;
s->d1->r_epoch++;
- memcpy(&(s->d1->bitmap), &(s->d1->next_bitmap), sizeof(DTLS1_BITMAP));
- memset(&(s->d1->next_bitmap), 0x00, sizeof(DTLS1_BITMAP));
+ memset(&s->d1->bitmap, 0, sizeof(DTLS1_BITMAP));
} else {
seq = s->s3->write_sequence;
memcpy(s->d1->last_write_sequence, seq, sizeof(s->s3->write_sequence));
