From 4947ba950d938ae7c6e0b8a8422ba9f3284389ef Mon Sep 17 00:00:00 2001 From: Dmitri Tikhonov Date: Tue, 8 Oct 2019 15:54:01 -0400 Subject: [PATCH] Release 2.4.5 - [OPTIMIZATION]: flush encoder stream only when necessary. - [BUGFIX] Use ls-qpack v0.10.2 for new API -- and for a bug fix. - [BUGFIX] Typo in IETF conn SETTINGS writer. - Use latest BoringSSL. --- .cirrus.yml | 3 +- .travis.yml | 3 +- CHANGELOG | 7 +++ Dockerfile | 3 +- README.md | 12 +---- include/lsquic.h | 2 +- patches/boringssl-meds.patch | 73 --------------------------- src/liblsquic/ls-qpack | 2 +- src/liblsquic/lsquic_enc_sess_ietf.c | 24 +-------- src/liblsquic/lsquic_full_conn_ietf.c | 2 +- src/liblsquic/lsquic_hcso_writer.c | 2 +- src/liblsquic/lsquic_qenc_hdl.c | 10 ++-- src/liblsquic/lsquic_qenc_hdl.h | 3 +- src/liblsquic/lsquic_stream.c | 18 ++++--- src/liblsquic/lsquic_stream.h | 2 +- test/unittests/test_send_headers.c | 5 +- 16 files changed, 41 insertions(+), 130 deletions(-) delete mode 100644 patches/boringssl-meds.patch diff --git a/.cirrus.yml b/.cirrus.yml index ace967e..2d86c28 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -8,8 +8,7 @@ task: - cd boringssl # This is so that both GQUIC and IETF branches build. Just picking # a known good revision: - - git checkout 32e59d2d3264e4e104b355ef73663b8b79ac4093 - - patch -p1 -i ../patches/boringssl-meds.patch + - git checkout 49de1fc2910524c888866c7e2b0db1ba8af2a530 - cmake . - make - cd - diff --git a/.travis.yml b/.travis.yml index ab8e37e..e076fbc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,8 +23,7 @@ before_script: - cd boringssl # This is so that both GQUIC and IETF branches build. Just picking # a known good revision: - - git checkout 32e59d2d3264e4e104b355ef73663b8b79ac4093 - - patch -p1 -i ../patches/boringssl-meds.patch + - git checkout 49de1fc2910524c888866c7e2b0db1ba8af2a530 - cmake . - make - cd - diff --git a/CHANGELOG b/CHANGELOG index 6d4e586..3913a1a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,10 @@ +2019-10-08 + - 2.4.5 + - [OPTIMIZATION]: flush encoder stream only when necessary. + - [BUGFIX] Use ls-qpack v0.10.2 for new API -- and for a bug fix. + - [BUGFIX] Typo in IETF conn SETTINGS writer. + - Use latest BoringSSL. + 2019-10-08 - 2.4.4 - [API] Add lsquic_alpn2ver() to aid parsing Alt-Svc header. diff --git a/Dockerfile b/Dockerfile index 13d24e2..6a528ed 100644 --- a/Dockerfile +++ b/Dockerfile @@ -17,8 +17,7 @@ COPY ./ /src/lsquic/ RUN git clone https://boringssl.googlesource.com/boringssl && \ cd boringssl && \ - git checkout 32e59d2d3264e4e104b355ef73663b8b79ac4093 && \ - patch -p1 -i /src/lsquic/patches/boringssl-meds.patch && \ + git checkout 49de1fc2910524c888866c7e2b0db1ba8af2a530 && \ cmake . && \ make diff --git a/README.md b/README.md index 64ece71..9eb70f9 100644 --- a/README.md +++ b/README.md @@ -51,18 +51,10 @@ You may need to install pre-requisites like zlib and libevent. 2. Use specific BoringSSL version ``` -git checkout 32e59d2d3264e4e104b355ef73663b8b79ac4093 +git checkout 49de1fc2910524c888866c7e2b0db1ba8af2a530 ``` -3. Patch the library - -This patch is required for IETF QUIC support. - -``` -patch -p1 -i ../patches/boringssl-meds.patch -``` - -4. Compile the library +3. Compile the library ``` cmake . && make diff --git a/include/lsquic.h b/include/lsquic.h index cbbd2e0..4847b69 100644 --- a/include/lsquic.h +++ b/include/lsquic.h @@ -25,7 +25,7 @@ extern "C" { #define LSQUIC_MAJOR_VERSION 2 #define LSQUIC_MINOR_VERSION 4 -#define LSQUIC_PATCH_VERSION 4 +#define LSQUIC_PATCH_VERSION 5 /** * Engine flags: diff --git a/patches/boringssl-meds.patch b/patches/boringssl-meds.patch deleted file mode 100644 index cd651df..0000000 --- a/patches/boringssl-meds.patch +++ /dev/null @@ -1,73 +0,0 @@ -commit 46f967bfe44a80bb4bc0e7e9d4b03de3f91d03fb -Author: Dmitri Tikhonov -Date: Fri Feb 22 11:51:21 2019 -0500 - - Add support for QUIC's use of max_early_data_size - - The server MUST set this value to 0xFFFFFFFF in NewSessionTicket, - while the client should be able to examine it in order to verify - this requirement. - -diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h -index 59b9eac..2e6cefb 100644 ---- a/include/openssl/ssl.h -+++ b/include/openssl/ssl.h -@@ -1744,6 +1744,11 @@ OPENSSL_EXPORT int SSL_SESSION_set_ticket(SSL_SESSION *session, - OPENSSL_EXPORT uint32_t - SSL_SESSION_get_ticket_lifetime_hint(const SSL_SESSION *session); - -+// SSL_SESSION_get_max_early_data_size returns ticket max early data size of -+// |session| in bytes or zero if none was set. -+OPENSSL_EXPORT uint32_t -+SSL_SESSION_get_max_early_data_size(const SSL_SESSION *session); -+ - // SSL_SESSION_get0_cipher returns the cipher negotiated by the connection which - // established |session|. - // -diff --git a/ssl/internal.h b/ssl/internal.h -index 1116bad..98dcfa3 100644 ---- a/ssl/internal.h -+++ b/ssl/internal.h -@@ -2506,6 +2506,10 @@ struct SSL_CONFIG { - // kMaxEarlyDataSkipped in tls_record.c, which is measured in ciphertext. - static const size_t kMaxEarlyDataAccepted = 14336; - -+// kQUICMaxEarlyData is the value to which the max_early_data_size field -+// in a NewSessionTicket must be set when sent by a QUIC server. -+static const uint32_t kQUICMaxEarlyData = 0xffffffffu; -+ - UniquePtr ssl_cert_dup(CERT *cert); - void ssl_cert_clear_certs(CERT *cert); - bool ssl_set_cert(CERT *cert, UniquePtr buffer); -diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc -index 927dd1b..cd5b5bb 100644 ---- a/ssl/ssl_session.cc -+++ b/ssl/ssl_session.cc -@@ -1025,6 +1025,10 @@ uint32_t SSL_SESSION_get_ticket_lifetime_hint(const SSL_SESSION *session) { - return session->ticket_lifetime_hint; - } - -+uint32_t SSL_SESSION_get_max_early_data_size(const SSL_SESSION *session) { -+ return session->ticket_max_early_data; -+} -+ - const SSL_CIPHER *SSL_SESSION_get0_cipher(const SSL_SESSION *session) { - return session->cipher; - } -diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc -index 562fecb..4edd0ef 100644 ---- a/ssl/tls13_server.cc -+++ b/ssl/tls13_server.cc -@@ -179,7 +179,11 @@ static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) { - } - session->ticket_age_add_valid = true; - if (ssl->enable_early_data) { -- session->ticket_max_early_data = kMaxEarlyDataAccepted; -+ if (ssl->quic_method == nullptr) { -+ session->ticket_max_early_data = kMaxEarlyDataAccepted; -+ } else { -+ session->ticket_max_early_data = kQUICMaxEarlyData; -+ } - } - - static_assert(kNumTickets < 256, "Too many tickets"); diff --git a/src/liblsquic/ls-qpack b/src/liblsquic/ls-qpack index 6e2bdf4..1d472c0 160000 --- a/src/liblsquic/ls-qpack +++ b/src/liblsquic/ls-qpack @@ -1 +1 @@ -Subproject commit 6e2bdf46a214efa0f934d931f29058238ed32f46 +Subproject commit 1d472c0a51de5b569bd6d32a77251c15e910d734 diff --git a/src/liblsquic/lsquic_enc_sess_ietf.c b/src/liblsquic/lsquic_enc_sess_ietf.c index 4136547..c1962b9 100644 --- a/src/liblsquic/lsquic_enc_sess_ietf.c +++ b/src/liblsquic/lsquic_enc_sess_ietf.c @@ -1141,38 +1141,17 @@ static int iquic_new_session_cb (SSL *ssl, SSL_SESSION *session) { struct enc_sess_iquic *enc_sess; - uint32_t max_early_data_size, num; + uint32_t num; unsigned char *p, *buf; uint8_t *ticket_buf; size_t ticket_sz; lsquic_ver_tag_t tag; const uint8_t *trapa_buf; - SSL_CTX *ssl_ctx; size_t trapa_sz, buf_sz; enc_sess = SSL_get_ex_data(ssl, s_idx); assert(enc_sess->esi_enpub->enp_stream_if->on_zero_rtt_info); - max_early_data_size = SSL_SESSION_get_max_early_data_size(session); - if (max_early_data_size && 0xFFFFFFFFu != max_early_data_size) - { - /* XXX We do not catch the case when early_data extension is present - * and max_early_data_size is set to zero, which is an invalid value. - * This is because there is no way to check this using existing - * BoringSSL APIs. - */ - /* See [draft-ietf-quic-tls-23], Section 4.5 */ - LSQ_INFO("max_early_data_size=0x%X, protocol violation", - max_early_data_size); - enc_sess->esi_conn->cn_if->ci_abort_error(enc_sess->esi_conn, 0, - TEC_PROTOCOL_VIOLATION, "max_early_data_size is set to %u " - "instead of 0xFFFFFFFF as mandated by standard", - max_early_data_size); - ssl_ctx = SSL_get_SSL_CTX(ssl); - SSL_CTX_sess_set_new_cb(ssl_ctx, NULL); - return 0; - } - SSL_get_peer_quic_transport_params(enc_sess->esi_ssl, &trapa_buf, &trapa_sz); if (!(trapa_buf + trapa_sz)) @@ -1513,7 +1492,6 @@ iquic_esfi_handshake (struct enc_sess_iquic *enc_sess) case SSL_ERROR_WANT_WRITE: LSQ_DEBUG("retry write"); return IHS_WANT_WRITE; - case SSL_EARLY_DATA_REJECTED: /* XXX should this be included here? */ case SSL_ERROR_EARLY_DATA_REJECTED: LSQ_DEBUG("early data rejected"); hsk_status = LSQ_HSK_0RTT_FAIL; diff --git a/src/liblsquic/lsquic_full_conn_ietf.c b/src/liblsquic/lsquic_full_conn_ietf.c index 42de517..af3f626 100644 --- a/src/liblsquic/lsquic_full_conn_ietf.c +++ b/src/liblsquic/lsquic_full_conn_ietf.c @@ -3028,7 +3028,7 @@ ietf_full_conn_ci_push_stream (struct lsquic_conn *lconn, void *hset, } } prefix_sz = lsqpack_enc_end_header(&conn->ifc_qeh.qeh_encoder, - discard, sizeof(discard)); + discard, sizeof(discard), NULL); if (!(prefix_sz == 2 && discard[0] == 0 && discard[1] == 0)) { LSQ_WARN("stream push: unexpected prefix values %zd, %hhu, %hhu", diff --git a/src/liblsquic/lsquic_hcso_writer.c b/src/liblsquic/lsquic_hcso_writer.c index 34eea4e..8b7bd5a 100644 --- a/src/liblsquic/lsquic_hcso_writer.c +++ b/src/liblsquic/lsquic_hcso_writer.c @@ -174,7 +174,7 @@ lsquic_hcso_write_settings (struct hcso_writer *writer, bits = hcso_setting_type2bits(writer, HQSID_QPACK_BLOCKED_STREAMS); vint_write(p, HQSID_QPACK_BLOCKED_STREAMS, bits, 1 << bits); p += 1 << bits; - bits = vint_val2bits(settings->es_qpack_dec_max_size); + bits = vint_val2bits(settings->es_qpack_dec_max_blocked); vint_write(p, settings->es_qpack_dec_max_blocked, bits, 1 << bits); p += 1 << bits; } diff --git a/src/liblsquic/lsquic_qenc_hdl.c b/src/liblsquic/lsquic_qenc_hdl.c index 7235091..7dbe842 100644 --- a/src/liblsquic/lsquic_qenc_hdl.c +++ b/src/liblsquic/lsquic_qenc_hdl.c @@ -322,7 +322,7 @@ static enum qwh_status qeh_write_headers (struct qpack_enc_hdl *qeh, lsquic_stream_id_t stream_id, unsigned seqno, const struct lsquic_http_headers *headers, unsigned char *buf, size_t *prefix_sz, size_t *headers_sz, - uint64_t *completion_offset) + uint64_t *completion_offset, enum lsqpack_enc_header_flags *hflags) { unsigned char *p = buf; unsigned char *const end = buf + *headers_sz; @@ -410,7 +410,8 @@ qeh_write_headers (struct qpack_enc_hdl *qeh, lsquic_stream_id_t stream_id, } } - nw = lsqpack_enc_end_header(&qeh->qeh_encoder, buf - *prefix_sz, *prefix_sz); + nw = lsqpack_enc_end_header(&qeh->qeh_encoder, buf - *prefix_sz, + *prefix_sz, hflags); if (nw <= 0) { LSQ_WARN("could not end header: %zd", nw); @@ -450,11 +451,12 @@ enum qwh_status lsquic_qeh_write_headers (struct qpack_enc_hdl *qeh, lsquic_stream_id_t stream_id, unsigned seqno, const struct lsquic_http_headers *headers, unsigned char *buf, - size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset) + size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset, + enum lsqpack_enc_header_flags *hflags) { if (qeh->qeh_flags & QEH_INITIALIZED) return qeh_write_headers(qeh, stream_id, seqno, headers, buf, - prefix_sz, headers_sz, completion_offset); + prefix_sz, headers_sz, completion_offset, hflags); else return QWH_ERR; } diff --git a/src/liblsquic/lsquic_qenc_hdl.h b/src/liblsquic/lsquic_qenc_hdl.h index 96d7e74..d448dfc 100644 --- a/src/liblsquic/lsquic_qenc_hdl.h +++ b/src/liblsquic/lsquic_qenc_hdl.h @@ -58,7 +58,8 @@ enum qwh_status { enum qwh_status lsquic_qeh_write_headers (struct qpack_enc_hdl *, lsquic_stream_id_t stream_id, unsigned seqno, const struct lsquic_http_headers *, unsigned char *buf, - size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset); + size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset, + enum lsqpack_enc_header_flags *hflags); uint64_t lsquic_qeh_enc_off (struct qpack_enc_hdl *); diff --git a/src/liblsquic/lsquic_stream.c b/src/liblsquic/lsquic_stream.c index e7b16a1..019643c 100644 --- a/src/liblsquic/lsquic_stream.c +++ b/src/liblsquic/lsquic_stream.c @@ -2753,13 +2753,13 @@ stream_write_to_packet_std (struct frame_gen_ctx *fg_ctx, const size_t size) if ((stream->stream_flags & (STREAM_HEADERS_SENT|STREAM_HDRS_FLUSHED)) == STREAM_HEADERS_SENT) { - /* Optimization idea: the QPACK encoder stream needs only be flushed - * if the headers in this stream are dependent on the buffered encoder - * stream bytes. Knowing this would require changes to ls-qpack. For - * this reason, we don't perform this check and just flush it. - */ if (stream->sm_bflags & SMBF_IETF) - headers_stream = stream->conn_pub->u.ietf.qeh->qeh_enc_sm_out; + { + if (stream->stream_flags & STREAM_ENCODER_DEP) + headers_stream = stream->conn_pub->u.ietf.qeh->qeh_enc_sm_out; + else + headers_stream = NULL; + } else headers_stream = lsquic_headers_stream_get_stream(stream->conn_pub->u.gquic.hs); @@ -3326,6 +3326,7 @@ send_headers_ietf (struct lsquic_stream *stream, unsigned bits; ssize_t nw; unsigned char *header_block; + enum lsqpack_enc_header_flags hflags; unsigned char buf[max_push_size + max_prefix_size + MAX_HEADERS_SIZE]; stream->stream_flags &= ~STREAM_PUSHING; @@ -3338,7 +3339,7 @@ send_headers_ietf (struct lsquic_stream *stream, headers_sz = sizeof(buf) - max_prefix_size - max_push_size; qwh = lsquic_qeh_write_headers(stream->conn_pub->u.ietf.qeh, stream->id, 0, headers, buf + max_push_size + max_prefix_size, &prefix_sz, - &headers_sz, &stream->sm_hb_compl); + &headers_sz, &stream->sm_hb_compl, &hflags); if (!(qwh == QWH_FULL || qwh == QWH_PARTIAL)) { @@ -3349,6 +3350,9 @@ send_headers_ietf (struct lsquic_stream *stream, return -1; } + if (hflags & LSQECH_REF_NEW_ENTRIES) + stream->stream_flags |= STREAM_ENCODER_DEP; + if (stream->sm_promise) { assert(lsquic_stream_is_pushed(stream)); diff --git a/src/liblsquic/lsquic_stream.h b/src/liblsquic/lsquic_stream.h index 935c286..afcd35e 100644 --- a/src/liblsquic/lsquic_stream.h +++ b/src/liblsquic/lsquic_stream.h @@ -197,7 +197,7 @@ enum stream_flags { STREAM_UNUSED10 = 1 << 10, /* Unused */ STREAM_HEADERS_SENT = 1 << 11, STREAM_HAVE_UH = 1 << 12, /* Have uncompressed headers */ - STREAM_UNUSED13 = 1 << 13, /* Unused */ + STREAM_ENCODER_DEP = 1 << 13, /* Encoder dependency: flush (IETF only) */ STREAM_HEAD_IN_FIN = 1 << 14, /* Incoming headers has FIN bit set */ STREAM_FRAMES_ELIDED= 1 << 15, STREAM_FORCE_FINISH = 1 << 16, /* Replaces FIN sent and received */ diff --git a/test/unittests/test_send_headers.c b/test/unittests/test_send_headers.c index 1153989..d10e3df 100644 --- a/test/unittests/test_send_headers.c +++ b/test/unittests/test_send_headers.c @@ -226,12 +226,15 @@ enum qwh_status lsquic_qeh_write_headers (struct qpack_enc_hdl *qeh, lsquic_stream_id_t stream_id, unsigned seqno, const struct lsquic_http_headers *headers, unsigned char *buf, - size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset) + size_t *prefix_sz, size_t *headers_sz, uint64_t *completion_offset, + enum lsqpack_enc_header_flags *hflags) { memset(buf - *prefix_sz, 0xC5, *prefix_sz + *headers_sz); *prefix_sz = test_vals.prefix_sz; *headers_sz = test_vals.headers_sz; *completion_offset = test_vals.completion_offset; + if (hflags) + *hflags = 0; return test_vals.status; }