Remove buffered_app_data as well.
This conceivably has a use, but NSS doesn't do this buffer either and it still
suffers from the same problems as the other uses of record_pqueue. This removes
the last use of record_pqueue. It also opens the door to removing pqueue
altogether as it isn't the right data structure for either of the remaining
uses either. (It's not clear it was right for record_pqueue either, but I don't
feel like digging into this code.)
Change-Id: If8a43e7332b3cd11a78a516f3e8ebf828052316f
Reviewed-on: https://boringssl-review.googlesource.com/4239
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h
index 5d60519..b2ea94c 100644
--- a/include/openssl/dtls1.h
+++ b/include/openssl/dtls1.h
@@ -105,11 +105,6 @@
uint16_t epoch;
};
-typedef struct record_pqueue_st {
- uint16_t epoch;
- pqueue q;
-} record_pqueue;
-
/* TODO(davidben): This structure is used for both incoming messages and
* outgoing messages. |fragment| and |reassembly| are only used in the former
* and should be moved elsewhere. */
@@ -157,10 +152,6 @@
* TODO(davidben): This data structure may as well be a STACK_OF(T). */
pqueue sent_messages;
- /* Buffered application records. Only for records between CCS and Finished to
- * prevent either protocol violation or unnecessary message loss. */
- record_pqueue buffered_app_data;
-
unsigned int mtu; /* max DTLS packet size */
struct hm_header_st w_msg_hdr;
@@ -180,13 +171,6 @@
unsigned int change_cipher_spec_ok;
} DTLS1_STATE;
-typedef struct dtls1_record_data_st {
- uint8_t *packet;
- unsigned int packet_length;
- SSL3_BUFFER rbuf;
- SSL3_RECORD rrec;
-} DTLS1_RECORD_DATA;
-
#ifdef __cplusplus
} /* extern C */
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 61a862d..fc59e33 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -98,19 +98,14 @@
d1->buffered_messages = pqueue_new();
d1->sent_messages = pqueue_new();
- d1->buffered_app_data.q = pqueue_new();
- if (!d1->buffered_messages || !d1->sent_messages ||
- !d1->buffered_app_data.q) {
+ if (!d1->buffered_messages || !d1->sent_messages) {
if (d1->buffered_messages) {
pqueue_free(d1->buffered_messages);
}
if (d1->sent_messages) {
pqueue_free(d1->sent_messages);
}
- if (d1->buffered_app_data.q) {
- pqueue_free(d1->buffered_app_data.q);
- }
OPENSSL_free(d1);
ssl3_free(s);
return 0;
@@ -130,7 +125,6 @@
static void dtls1_clear_queues(SSL *s) {
pitem *item = NULL;
hm_fragment *frag = NULL;
- DTLS1_RECORD_DATA *rdata;
while ((item = pqueue_pop(s->d1->buffered_messages)) != NULL) {
frag = (hm_fragment *)item->data;
@@ -143,15 +137,6 @@
dtls1_hm_fragment_free(frag);
pitem_free(item);
}
-
- while ((item = pqueue_pop(s->d1->buffered_app_data.q)) != NULL) {
- rdata = (DTLS1_RECORD_DATA *)item->data;
- if (rdata->rbuf.buf) {
- OPENSSL_free(rdata->rbuf.buf);
- }
- OPENSSL_free(item->data);
- pitem_free(item);
- }
}
void dtls1_free(SSL *s) {
@@ -165,7 +150,6 @@
pqueue_free(s->d1->buffered_messages);
pqueue_free(s->d1->sent_messages);
- pqueue_free(s->d1->buffered_app_data.q);
OPENSSL_free(s->d1);
s->d1 = NULL;
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c
index 56bdae0..7b2cab1 100644
--- a/ssl/d1_pkt.c
+++ b/ssl/d1_pkt.c
@@ -183,90 +183,10 @@
static int dtls1_record_replay_check(SSL *s, DTLS1_BITMAP *bitmap);
static void dtls1_record_bitmap_update(SSL *s, DTLS1_BITMAP *bitmap);
-static int dtls1_buffer_record(SSL *s, record_pqueue *q,
- uint8_t *priority);
static int dtls1_process_record(SSL *s);
static int do_dtls1_write(SSL *s, int type, const uint8_t *buf,
unsigned int len);
-/* copy buffered record into SSL structure */
-static int dtls1_copy_record(SSL *s, pitem *item) {
- DTLS1_RECORD_DATA *rdata;
-
- rdata = (DTLS1_RECORD_DATA *)item->data;
-
- if (s->s3->rbuf.buf != NULL) {
- OPENSSL_free(s->s3->rbuf.buf);
- }
-
- s->packet = rdata->packet;
- s->packet_length = rdata->packet_length;
- memcpy(&(s->s3->rbuf), &(rdata->rbuf), sizeof(SSL3_BUFFER));
- memcpy(&(s->s3->rrec), &(rdata->rrec), sizeof(SSL3_RECORD));
-
- /* Set proper sequence number for mac calculation */
- memcpy(&(s->s3->read_sequence[2]), &(rdata->packet[5]), 6);
-
- return 1;
-}
-
-static int dtls1_buffer_record(SSL *s, record_pqueue *queue,
- uint8_t *priority) {
- DTLS1_RECORD_DATA *rdata;
- pitem *item;
-
- /* Limit the size of the queue to prevent DOS attacks */
- if (pqueue_size(queue->q) >= 100) {
- return 0;
- }
-
- rdata = OPENSSL_malloc(sizeof(DTLS1_RECORD_DATA));
- item = pitem_new(priority, rdata);
- if (rdata == NULL || item == NULL) {
- if (rdata != NULL) {
- OPENSSL_free(rdata);
- }
- if (item != NULL) {
- pitem_free(item);
- }
-
- OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
- return -1;
- }
-
- rdata->packet = s->packet;
- rdata->packet_length = s->packet_length;
- memcpy(&(rdata->rbuf), &(s->s3->rbuf), sizeof(SSL3_BUFFER));
- memcpy(&(rdata->rrec), &(s->s3->rrec), sizeof(SSL3_RECORD));
-
- item->data = rdata;
-
- s->packet = NULL;
- s->packet_length = 0;
- memset(&(s->s3->rbuf), 0, sizeof(SSL3_BUFFER));
- memset(&(s->s3->rrec), 0, sizeof(SSL3_RECORD));
-
- if (!ssl3_setup_buffers(s)) {
- goto internal_error;
- }
-
- /* insert should not fail, since duplicates are dropped */
- if (pqueue_insert(queue->q, item) == NULL) {
- goto internal_error;
- }
-
- return 1;
-
-internal_error:
- OPENSSL_PUT_ERROR(SSL, dtls1_buffer_record, ERR_R_INTERNAL_ERROR);
- if (rdata->rbuf.buf != NULL) {
- OPENSSL_free(rdata->rbuf.buf);
- }
- OPENSSL_free(rdata);
- pitem_free(item);
- return -1;
-}
-
static int dtls1_process_record(SSL *s) {
int al;
SSL3_RECORD *rr;
@@ -540,21 +460,6 @@
* s->s3->rrec.length - number of bytes. */
rr = &s->s3->rrec;
- /* We are not handshaking and have no data yet,
- * so process data buffered during the last handshake
- * in advance, if any.
- */
- if (s->state == SSL_ST_OK && rr->length == 0) {
- pitem *item;
- item = pqueue_pop(s->d1->buffered_app_data.q);
- if (item) {
- dtls1_copy_record(s, item);
-
- OPENSSL_free(item->data);
- pitem_free(item);
- }
- }
-
/* Check for timeout */
if (dtls1_handle_timeout(s) > 0) {
goto start;
@@ -581,12 +486,8 @@
if (s->s3->change_cipher_spec && rr->type != SSL3_RT_HANDSHAKE &&
rr->type != SSL3_RT_ALERT) {
/* We now have an unexpected record between CCS and Finished. Most likely
- * the packets were reordered on their way, so buffer the application data
- * for later processing rather than dropping the connection. */
- if (dtls1_buffer_record(s, &(s->d1->buffered_app_data), rr->seq_num) < 0) {
- OPENSSL_PUT_ERROR(SSL, dtls1_read_bytes, ERR_R_INTERNAL_ERROR);
- return -1;
- }
+ * the packets were reordered on their way. DTLS is unreliable, so drop the
+ * packet and expect the peer to retransmit. */
rr->length = 0;
goto start;
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 084ae46..6e80f94 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -695,6 +695,8 @@
AppDataAfterChangeCipherSpec: []byte("TEST MESSAGE"),
},
},
+ // BoringSSL's DTLS implementation will drop the out-of-order
+ // application data.
},
{
name: "AlertAfterChangeCipherSpec",
@@ -1174,21 +1176,14 @@
return err
}
- var testMessage []byte
- if config.Bugs.AppDataAfterChangeCipherSpec != nil {
- // We've already sent a message. Expect the shim to echo it
- // back.
- testMessage = config.Bugs.AppDataAfterChangeCipherSpec
- } else {
- if messageLen == 0 {
- messageLen = 32
- }
- testMessage = make([]byte, messageLen)
- for i := range testMessage {
- testMessage[i] = 0x42
- }
- tlsConn.Write(testMessage)
+ if messageLen == 0 {
+ messageLen = 32
}
+ testMessage := make([]byte, messageLen)
+ for i := range testMessage {
+ testMessage[i] = 0x42
+ }
+ tlsConn.Write(testMessage)
buf := make([]byte, len(testMessage))
if test.protocol == dtls {