From f7389ff2e6258dbc7e81f482be05ff34f11a11cb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 18 Oct 2013 19:27:15 +0900 Subject: [PATCH 1/4] Revert 622b05aa3131d646038522e762e90b1b4ea3b917 It turns out that 0-bit prefix is wrong, and the author now clearly stated that the intention is 8-bit prefix for 8+ fields. --- lib/nghttp2_hd.c | 88 ++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index 1b919f9d..e82874e0 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -359,15 +359,13 @@ static int ensure_write_buffer(uint8_t **buf_ptr, size_t *buflen_ptr, static size_t count_encoded_length(size_t n, int prefix) { + size_t k = (1 << prefix) - 1; size_t len = 0; - if(prefix > 0) { - size_t k = (1 << prefix) - 1; - if(n >= k) { - n -= k; - ++len; - } else { - return 1; - } + if(n >= k) { + n -= k; + ++len; + } else { + return 1; } do { ++len; @@ -382,17 +380,15 @@ static size_t count_encoded_length(size_t n, int prefix) static size_t encode_length(uint8_t *buf, size_t n, int prefix) { + size_t k = (1 << prefix) - 1; size_t len = 0; - if(prefix > 0) { - size_t k = (1 << prefix) - 1; - if(n >= k) { - *buf++ = k; - n -= k; - ++len; - } else { - *buf++ = n; - return 1; - } + if(n >= k) { + *buf++ = k; + n -= k; + ++len; + } else { + *buf++ = n; + return 1; } do { ++len; @@ -425,17 +421,13 @@ static uint8_t* decode_length(ssize_t *res, uint8_t *in, uint8_t *last, *res = -1; return in; } - if(prefix > 0) { - if((*in & k) == k) { - *res = k; - } else { - *res = (*in) & k; - return in + 1; - } - ++in; + if((*in & k) == k) { + *res = k; } else { - *res = 0; + *res = (*in) & k; + return in + 1; } + ++in; for(r = 0; in != last; ++in, r += 7) { *res += (*in & 0x7f) << r; if(*res >= (1 << 16)) { @@ -479,14 +471,14 @@ static int emit_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr, int rv; uint8_t *bufp; size_t blocklen = count_encoded_length(index + 1, 5) + - count_encoded_length(valuelen, 0) + valuelen; + count_encoded_length(valuelen, 8) + valuelen; rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen); if(rv != 0) { return rv; } bufp = *buf_ptr + *offset_ptr; bufp += encode_length(bufp, index + 1, 5); - bufp += encode_length(bufp, valuelen, 0); + bufp += encode_length(bufp, valuelen, 8); memcpy(bufp, value, valuelen); (*buf_ptr)[*offset_ptr] |= inc_indexing ? 0x40u : 0x60u; assert(bufp+valuelen - (*buf_ptr + *offset_ptr) == (ssize_t)blocklen); @@ -500,18 +492,18 @@ static int emit_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr, { int rv; uint8_t *bufp; - size_t blocklen = 1 + count_encoded_length(nv->namelen, 0) + nv->namelen + - count_encoded_length(nv->valuelen, 0) + nv->valuelen; + size_t blocklen = 1 + count_encoded_length(nv->namelen, 8) + nv->namelen + + count_encoded_length(nv->valuelen, 8) + nv->valuelen; rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen); if(rv != 0) { return rv; } bufp = *buf_ptr + *offset_ptr; *bufp++ = inc_indexing ? 0x40u : 0x60u; - bufp += encode_length(bufp, nv->namelen, 0); + bufp += encode_length(bufp, nv->namelen, 8); memcpy(bufp, nv->name, nv->namelen); bufp += nv->namelen; - bufp += encode_length(bufp, nv->valuelen, 0); + bufp += encode_length(bufp, nv->valuelen, 8); memcpy(bufp, nv->value, nv->valuelen); *offset_ptr += blocklen; return 0; @@ -525,16 +517,16 @@ static int emit_subst_indname_block(uint8_t **buf_ptr, size_t *buflen_ptr, int rv; uint8_t *bufp; size_t blocklen = count_encoded_length(index + 1, 6) + - count_encoded_length(subindex, 0) + - count_encoded_length(valuelen, 0) + valuelen; + count_encoded_length(subindex, 8) + + count_encoded_length(valuelen, 8) + valuelen; rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen); if(rv != 0) { return rv; } bufp = *buf_ptr + *offset_ptr; bufp += encode_length(bufp, index + 1, 6); - bufp += encode_length(bufp, subindex, 0); - bufp += encode_length(bufp, valuelen, 0); + bufp += encode_length(bufp, subindex, 8); + bufp += encode_length(bufp, valuelen, 8); memcpy(bufp, value, valuelen); *offset_ptr += blocklen; return 0; @@ -546,20 +538,20 @@ static int emit_subst_newname_block(uint8_t **buf_ptr, size_t *buflen_ptr, { int rv; uint8_t *bufp; - size_t blocklen = 1 + count_encoded_length(nv->namelen, 0) + nv->namelen + - count_encoded_length(subindex, 0) + - count_encoded_length(nv->valuelen, 0) + nv->valuelen; + size_t blocklen = 1 + count_encoded_length(nv->namelen, 8) + nv->namelen + + count_encoded_length(subindex, 8) + + count_encoded_length(nv->valuelen, 8) + nv->valuelen; rv = ensure_write_buffer(buf_ptr, buflen_ptr, *offset_ptr, blocklen); if(rv != 0) { return rv; } bufp = *buf_ptr + *offset_ptr; *bufp++ = 0; - bufp += encode_length(bufp, nv->namelen, 0); + bufp += encode_length(bufp, nv->namelen, 8); memcpy(bufp, nv->name, nv->namelen); bufp += nv->namelen; - bufp += encode_length(bufp, subindex, 0); - bufp += encode_length(bufp, nv->valuelen, 0); + bufp += encode_length(bufp, subindex, 8); + bufp += encode_length(bufp, nv->valuelen, 8); memcpy(bufp, nv->value, nv->valuelen); *offset_ptr += blocklen; return 0; @@ -918,7 +910,7 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, rv = NGHTTP2_ERR_HEADER_COMP; goto fail; } - in = decode_length(&namelen, in, last, 0); + in = decode_length(&namelen, in, last, 8); if(namelen < 0 || in + namelen > last) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; @@ -930,13 +922,13 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, nv.name = in; in += namelen; if(c == 0) { - in = decode_length(&subindex, in, last, 0); + in = decode_length(&subindex, in, last, 8); if(subindex < 0) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; } } - in = decode_length(&valuelen, in, last, 0); + in = decode_length(&valuelen, in, last, 8); if(valuelen < 0 || in + valuelen > last) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; @@ -981,13 +973,13 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, } ent = inflater->hd_table[index]; if((c & 0x40u) == 0) { - in = decode_length(&subindex, in, last, 0); + in = decode_length(&subindex, in, last, 8); if(subindex < 0) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; } } - in = decode_length(&valuelen, in , last, 0); + in = decode_length(&valuelen, in , last, 8); if(valuelen < 0 || in + valuelen > last) { rv = NGHTTP2_ERR_HEADER_COMP; goto fail; From 7bd145fd2340600fbac8bf7cf7fbfffbf07704c8 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 18 Oct 2013 19:30:04 +0900 Subject: [PATCH 2/4] Fix out of bound array access in decode_length --- lib/nghttp2_hd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index e82874e0..0d4a1221 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -438,7 +438,7 @@ static uint8_t* decode_length(ssize_t *res, uint8_t *in, uint8_t *last, break; } } - if(*in & (1 << 7)) { + if(in == last || *in & (1 << 7)) { *res = -1; return NULL; } else { From 0efa6e657f55b074a11bbc28d4ea9adcf812ee82 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 18 Oct 2013 19:43:59 +0900 Subject: [PATCH 3/4] Fix outbound flow control count We wrongly added the whole payload length even if we sent part of it. --- lib/nghttp2_session.c | 12 +++++++++--- tests/nghttp2_session_test.c | 15 ++++++++++++++- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 7eb6b642..048b0a2a 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1597,11 +1597,16 @@ int nghttp2_session_send(nghttp2_session *session) return NGHTTP2_ERR_CALLBACK_FAILURE; } } else { - session->aob.framebufoff += sentlen; - if(session->aob.item->frame_cat == NGHTTP2_CAT_DATA) { + if(session->aob.item->frame_cat == NGHTTP2_CAT_DATA && + session->aob.framebufoff + sentlen > NGHTTP2_FRAME_HEAD_LENGTH) { nghttp2_data *frame; nghttp2_stream *stream; - uint16_t len = nghttp2_get_uint16(&session->aob.framebuf[0]); + uint16_t len; + if(session->aob.framebufoff < NGHTTP2_FRAME_HEAD_LENGTH) { + len = session->aob.framebufoff + sentlen - NGHTTP2_FRAME_HEAD_LENGTH; + } else { + len = sentlen; + } frame = nghttp2_outbound_item_get_data_frame(session->aob.item); stream = nghttp2_session_get_stream(session, frame->hd.stream_id); if(stream && stream->remote_flow_control) { @@ -1611,6 +1616,7 @@ int nghttp2_session_send(nghttp2_session *session) session->remote_window_size -= len; } } + session->aob.framebufoff += sentlen; if(session->aob.framebufoff == session->aob.framebuflen) { /* Frame has completely sent */ r = nghttp2_session_after_frame_sent(session); diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 8dd3be2f..71699112 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -68,6 +68,7 @@ typedef struct { int data_chunk_recv_cb_called; int data_recv_cb_called; const nghttp2_frame *frame; + size_t fixed_sendlen; } my_user_data; static void scripted_data_feed_init(scripted_data_feed *df, @@ -94,6 +95,15 @@ static ssize_t fail_send_callback(nghttp2_session *session, return NGHTTP2_ERR_CALLBACK_FAILURE; } +static ssize_t fixed_bytes_send_callback(nghttp2_session *session, + const uint8_t *data, size_t len, + int flags, void *user_data) +{ + size_t fixed_sendlen = ((my_user_data*)user_data)->fixed_sendlen; + return fixed_sendlen < len ? fixed_sendlen : len; +} + + static ssize_t scripted_recv_callback(nghttp2_session *session, uint8_t* data, size_t len, int flags, void *user_data) @@ -2949,12 +2959,15 @@ void test_nghttp2_session_flow_control(void) nghttp2_frame settings_frame; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); - callbacks.send_callback = null_send_callback; + callbacks.send_callback = fixed_bytes_send_callback; callbacks.on_frame_send_callback = on_frame_send_callback; data_prd.read_callback = fixed_length_data_source_read_callback; ud.frame_send_cb_called = 0; ud.data_source_length = 128*1024; + /* Use smaller emission count so that we can check outbound flow + control window calculation is correct. */ + ud.fixed_sendlen = 2*1024; /* Initial window size to 64KiB - 1*/ nghttp2_session_client_new(&session, &callbacks, &ud); From 7b87d71121d7950d3fbc2f5483693a3ad55dfaa3 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 18 Oct 2013 19:48:15 +0900 Subject: [PATCH 4/4] nghttp2_hd: Fix missing return value handling --- lib/nghttp2_hd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index 0d4a1221..eb36a163 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -857,6 +857,9 @@ ssize_t nghttp2_hd_deflate_hd(nghttp2_hd_context *deflater, be removed. */ ent->flags ^= NGHTTP2_HD_FLAG_REFSET; rv = emit_indexed_block(buf_ptr, buflen_ptr, &offset, ent->index); + if(rv != 0) { + goto fail; + } } ent->flags &= ~(NGHTTP2_HD_FLAG_EMIT | NGHTTP2_HD_FLAG_IMPLICIT_EMIT); }