From 3f3f258cd63c59d9c8f179e307c6e1d9fd7fc44a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 15 Feb 2014 16:30:43 +0900 Subject: [PATCH] Add padding to PUSH_PROMISE --- lib/nghttp2_frame.c | 38 ++++--- lib/nghttp2_frame.h | 9 ++ lib/nghttp2_session.c | 192 +++++++++++++++++++++++------------ tests/nghttp2_frame_test.c | 15 ++- tests/nghttp2_session_test.c | 22 +++- 5 files changed, 186 insertions(+), 90 deletions(-) diff --git a/lib/nghttp2_frame.c b/lib/nghttp2_frame.c index c6318c1e..680a37b1 100644 --- a/lib/nghttp2_frame.c +++ b/lib/nghttp2_frame.c @@ -266,7 +266,7 @@ ssize_t nghttp2_frame_pack_headers(uint8_t **buf_ptr, if(frame->hd.flags & NGHTTP2_FLAG_PRIORITY) { nghttp2_put_uint32be(&(*buf_ptr)[payloadoff], frame->pri); } - return frame->hd.length + NGHTTP2_FRAME_HEAD_LENGTH + *bufoff_ptr; + return *bufoff_ptr + NGHTTP2_FRAME_HEAD_LENGTH + frame->hd.length; } int nghttp2_frame_unpack_headers_payload(nghttp2_headers *frame, @@ -396,35 +396,45 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr, ssize_t nghttp2_frame_pack_push_promise(uint8_t **buf_ptr, size_t *buflen_ptr, + size_t *bufoff_ptr, nghttp2_push_promise *frame, nghttp2_hd_deflater *deflater) { - ssize_t framelen; - size_t nv_offset = NGHTTP2_FRAME_HEAD_LENGTH + 4; + size_t payloadoff = NGHTTP2_FRAME_HEAD_LENGTH + 2; + size_t nv_offset = payloadoff + 4; ssize_t rv; + size_t payloadlen; + rv = nghttp2_hd_deflate_hd(deflater, buf_ptr, buflen_ptr, nv_offset, frame->nva, frame->nvlen); if(rv < 0) { return rv; } - framelen = rv + nv_offset; - if(NGHTTP2_FRAME_HEAD_LENGTH + NGHTTP2_MAX_FRAME_LENGTH < rv + nv_offset) { - frame->hd.length = NGHTTP2_MAX_FRAME_LENGTH; - frame->hd.flags &= ~NGHTTP2_FLAG_END_HEADERS; - } else { - frame->hd.length = framelen - NGHTTP2_FRAME_HEAD_LENGTH; - } + + payloadlen = 4 + rv; + + *bufoff_ptr = 2; + frame->padlen = 0; + frame->hd.length = payloadlen; /* If frame->nvlen == 0, *buflen_ptr may be smaller than nv_offset */ rv = nghttp2_reserve_buffer(buf_ptr, buflen_ptr, nv_offset); if(rv < 0) { return rv; } - memset(*buf_ptr, 0, nv_offset); + memset(*buf_ptr + *bufoff_ptr, 0, NGHTTP2_FRAME_HEAD_LENGTH); /* pack ctrl header after length is determined */ - nghttp2_frame_pack_frame_hd(*buf_ptr, &frame->hd); - nghttp2_put_uint32be(&(*buf_ptr)[8], frame->promised_stream_id); - return framelen; + if(NGHTTP2_MAX_FRAME_LENGTH < payloadlen) { + /* Needs CONTINUATION */ + nghttp2_frame_hd hd = frame->hd; + hd.flags &= ~NGHTTP2_FLAG_END_HEADERS; + hd.length = NGHTTP2_MAX_FRAME_LENGTH; + nghttp2_frame_pack_frame_hd(*buf_ptr + *bufoff_ptr, &hd); + } else { + nghttp2_frame_pack_frame_hd(*buf_ptr + *bufoff_ptr, &frame->hd); + } + nghttp2_put_uint32be(&(*buf_ptr)[payloadoff], frame->promised_stream_id); + return *bufoff_ptr + NGHTTP2_FRAME_HEAD_LENGTH + frame->hd.length; } int nghttp2_frame_unpack_push_promise_payload(nghttp2_push_promise *frame, diff --git a/lib/nghttp2_frame.h b/lib/nghttp2_frame.h index ee0e0b84..16754d09 100644 --- a/lib/nghttp2_frame.h +++ b/lib/nghttp2_frame.h @@ -256,12 +256,20 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr, * expansion occurred, memory previously pointed by |*buf_ptr| may * change. |*buf_ptr| and |*buflen_ptr| are updated accordingly. * + * The first byte the frame is serialized is returned in the + * |*bufoff_ptr|. Currently, it is always 2 to account for possible + * PAD_HIGH and PAD_LOW. + * * frame->hd.length is assigned after length is determined during * packing process. If payload length is strictly larger than * NGHTTP2_MAX_FRAME_LENGTH, payload data is still serialized as is, * but frame->hd.length is set to NGHTTP2_MAX_FRAME_LENGTH and * NGHTTP2_FLAG_END_HEADERS flag is cleared from frame->hd.flags. * + * This function returns the size of packed frame (which includes + * |*bufoff_ptr| bytes) if it succeeds, or returns one of the + * following negative error codes: + * * This function returns the size of packed frame if it succeeds, or * returns one of the following negative error codes: * @@ -274,6 +282,7 @@ int nghttp2_frame_unpack_settings_payload2(nghttp2_settings_entry **iv_ptr, */ ssize_t nghttp2_frame_pack_push_promise(uint8_t **buf_ptr, size_t *buflen_ptr, + size_t *bufoff_ptr, nghttp2_push_promise *frame, nghttp2_hd_deflater *deflater); diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 7fab91f8..8a29304b 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1095,6 +1095,76 @@ static ssize_t session_call_select_padding(nghttp2_session *session, return frame->hd.length; } +/* Add padding to HEADERS or PUSH_PROMISE. We use + frame->headers.padlen in this function to use the fact that + frame->push_promise has also padlen in the same position. */ +static ssize_t session_headers_add_pad(nghttp2_session *session, + nghttp2_frame *frame) +{ + int rv; + ssize_t padded_payloadlen; + + padded_payloadlen = session_call_select_padding(session, frame, + frame->hd.length + 1024); + if(nghttp2_is_fatal(padded_payloadlen)) { + return padded_payloadlen; + } + + frame->headers.padlen = padded_payloadlen - frame->hd.length; + frame->hd.length = padded_payloadlen; + + DEBUGF(fprintf(stderr, "payloadlen=%zu, padlen=%zu\n", + frame->hd.length, frame->headers.padlen)); + + if(frame->hd.length > NGHTTP2_MAX_FRAME_LENGTH) { + nghttp2_frame_hd hd = frame->hd; + hd.flags &= ~NGHTTP2_FLAG_END_HEADERS; + hd.length = NGHTTP2_MAX_FRAME_LENGTH; + + if(NGHTTP2_MAX_FRAME_LENGTH > + frame->hd.length - frame->headers.padlen) { + size_t padlen = NGHTTP2_MAX_FRAME_LENGTH - + (frame->hd.length - frame->headers.padlen); + + DEBUGF(fprintf(stderr, "padding across 2 frames\n")); + DEBUGF(fprintf(stderr, "first HEADERS/PUSH_PROMISE " + "payloadlen=%zu, padlen=%zu\n", hd.length, padlen)); + + rv = nghttp2_frame_add_pad(&session->aob.framebuf, + &session->aob.framebufmax, + &session->aob.framebufoff, + &hd.flags, + hd.length - padlen, + padlen); + if(nghttp2_is_fatal(rv)) { + return rv; + } + } else { + /* PAD_HIGH and PAD_LOW will be added in + nghttp2_session_after_frame_sent(). */ + } + nghttp2_frame_pack_frame_hd + (session->aob.framebuf + session->aob.framebufoff, &hd); + /* At this point, framebuflen > session->aob.framebufmax. But + before we access the missing part, we will allocate it in + nghttp2_session_after_frame_sent(). */ + } else if(frame->headers.padlen > 0) { + rv = nghttp2_frame_add_pad(&session->aob.framebuf, + &session->aob.framebufmax, + &session->aob.framebufoff, + &frame->hd.flags, + frame->hd.length - frame->headers.padlen, + frame->headers.padlen); + if(nghttp2_is_fatal(rv)) { + return rv; + } + nghttp2_frame_pack_frame_hd + (session->aob.framebuf + session->aob.framebufoff, &frame->hd); + } + return session->aob.framebufoff + NGHTTP2_FRAME_HEAD_LENGTH + + frame->hd.length; +} + static ssize_t nghttp2_session_prep_frame(nghttp2_session *session, nghttp2_outbound_item *item) { @@ -1106,7 +1176,6 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session, case NGHTTP2_HEADERS: { int r; nghttp2_headers_aux_data *aux_data; - ssize_t padded_payloadlen; aux_data = (nghttp2_headers_aux_data*)item->aux_data; if(frame->hd.stream_id == -1) { /* initial HEADERS, which opens stream */ @@ -1140,71 +1209,9 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session, if(framebuflen < 0) { return framebuflen; } - padded_payloadlen = session_call_select_padding - (session, frame, frame->hd.length + 1024); - if(nghttp2_is_fatal(padded_payloadlen)) { - return padded_payloadlen; - } - - frame->headers.padlen = padded_payloadlen - frame->hd.length; - frame->hd.length = padded_payloadlen; - - DEBUGF(fprintf(stderr, "payloadlen=%zu, padlen=%zu\n", - frame->hd.length, frame->headers.padlen)); - - if(frame->hd.length > NGHTTP2_MAX_FRAME_LENGTH) { - if(NGHTTP2_MAX_FRAME_LENGTH > - frame->hd.length - frame->headers.padlen) { - size_t padlen = NGHTTP2_MAX_FRAME_LENGTH - - (frame->hd.length - frame->headers.padlen); - nghttp2_frame_hd hd = frame->hd; - - DEBUGF(fprintf(stderr, "padding across 2 frames\n")); - - hd.flags &= ~NGHTTP2_FLAG_END_HEADERS; - hd.length = NGHTTP2_MAX_FRAME_LENGTH; - - DEBUGF(fprintf(stderr, "first HEADERS payloadlen=%zu, padlen=%zu\n", - hd.length, padlen)); - - r = nghttp2_frame_add_pad(&session->aob.framebuf, - &session->aob.framebufmax, - &session->aob.framebufoff, - &hd.flags, - hd.length - padlen, - padlen); - if(nghttp2_is_fatal(r)) { - return r; - } - framebuflen = session->aob.framebufoff + frame->hd.length - + NGHTTP2_FRAME_HEAD_LENGTH; - - nghttp2_frame_pack_frame_hd - (session->aob.framebuf + session->aob.framebufoff, &hd); - } else { - /* PAD_HIGH and PAD_LOW will be added in - nghttp2_session_after_frame_sent(). */ - framebuflen += frame->headers.padlen; - } - /* At this point, framebuflen > session->aob.framebufmax. But - before we access the missing part, we will allocate it in - nghttp2_session_after_frame_sent(). */ - } else if(frame->hd.length <= NGHTTP2_MAX_FRAME_LENGTH && - frame->headers.padlen > 0) { - r = nghttp2_frame_add_pad(&session->aob.framebuf, - &session->aob.framebufmax, - &session->aob.framebufoff, - &frame->hd.flags, - frame->hd.length - frame->headers.padlen, - frame->headers.padlen); - if(nghttp2_is_fatal(r)) { - return r; - } - framebuflen = session->aob.framebufoff + frame->hd.length - + NGHTTP2_FRAME_HEAD_LENGTH; - - nghttp2_frame_pack_frame_hd - (session->aob.framebuf + session->aob.framebufoff, &frame->hd); + framebuflen = session_headers_add_pad(session, frame); + if(framebuflen < 0) { + return framebuflen; } switch(frame->headers.cat) { @@ -1281,11 +1288,17 @@ static ssize_t nghttp2_session_prep_frame(nghttp2_session *session, session->next_stream_id += 2; framebuflen = nghttp2_frame_pack_push_promise(&session->aob.framebuf, &session->aob.framebufmax, + &session->aob.framebufoff, &frame->push_promise, &session->hd_deflater); if(framebuflen < 0) { return framebuflen; } + framebuflen = session_headers_add_pad(session, frame); + if(framebuflen < 0) { + return framebuflen; + } + stream = nghttp2_session_get_stream(session, frame->hd.stream_id); assert(stream); if(nghttp2_session_open_stream @@ -1538,6 +1551,9 @@ static int nghttp2_session_after_frame_sent(nghttp2_session *session) if(cont_hd.length < frame->headers.padlen) { padlen = cont_hd.length; } else { + /* We use frame->headers.padlen for PUSH_PROMISE too. This + is possible because padlen is located in the same + position. */ padlen = frame->headers.padlen; } DEBUGF(fprintf(stderr, @@ -3454,10 +3470,16 @@ static int inbound_frame_handle_pad(nghttp2_inbound_frame *iframe, if((hd->flags & NGHTTP2_FLAG_PAD_LOW) == 0) { return -1; } + if(hd->length < 2) { + return -1; + } inbound_frame_reset_left(iframe, 2); return 1; } if(hd->flags & NGHTTP2_FLAG_PAD_LOW) { + if(hd->length < 1) { + return -1; + } inbound_frame_reset_left(iframe, 1); return 1; } @@ -3657,6 +3679,21 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, break; case NGHTTP2_PUSH_PROMISE: DEBUGF(fprintf(stderr, "PUSH_PROMISE\n")); + rv = inbound_frame_handle_pad(iframe, &iframe->frame.hd); + if(rv < 0) { + busy = 1; + iframe->state = NGHTTP2_IB_IGN_PAYLOAD; + rv = nghttp2_session_terminate_session(session, + NGHTTP2_PROTOCOL_ERROR); + if(nghttp2_is_fatal(rv)) { + return rv; + } + break; + } + if(rv == 1) { + iframe->state = NGHTTP2_IB_READ_NBYTE; + break; + } if(iframe->payloadleft < 4) { busy = 1; iframe->state = NGHTTP2_IB_FRAME_SIZE_ERROR; @@ -3762,6 +3799,29 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, nghttp2_inbound_frame_reset(session); break; case NGHTTP2_PUSH_PROMISE: + if(iframe->padlen == 0 && + iframe->frame.hd.flags & NGHTTP2_FLAG_PAD_LOW) { + rv = inbound_frame_compute_pad(iframe); + if(rv < 0) { + busy = 1; + rv = nghttp2_session_terminate_session(session, + NGHTTP2_PROTOCOL_ERROR); + if(nghttp2_is_fatal(rv)) { + return rv; + } + iframe->state = NGHTTP2_IB_IGN_PAYLOAD; + break; + } + iframe->frame.push_promise.padlen = rv; + if(iframe->payloadleft < 4) { + busy = 1; + iframe->state = NGHTTP2_IB_FRAME_SIZE_ERROR; + break; + } + iframe->state = NGHTTP2_IB_READ_NBYTE; + inbound_frame_reset_left(iframe, 4); + break; + } rv = session_process_push_promise_frame(session); if(nghttp2_is_fatal(rv)) { return rv; diff --git a/tests/nghttp2_frame_test.c b/tests/nghttp2_frame_test.c index 9fe5cbbb..5580e1a7 100644 --- a/tests/nghttp2_frame_test.c +++ b/tests/nghttp2_frame_test.c @@ -263,10 +263,12 @@ void test_nghttp2_frame_pack_push_promise() nghttp2_push_promise frame, oframe; uint8_t *buf = NULL; size_t buflen = 0; + size_t bufoff; ssize_t framelen; nghttp2_nv *nva; ssize_t nvlen; nva_out out; + ssize_t nv_offset; nva_out_init(&out); nghttp2_hd_deflate_init(&deflater); @@ -276,16 +278,19 @@ void test_nghttp2_frame_pack_push_promise() nvlen = HEADERS_LENGTH; nghttp2_frame_push_promise_init(&frame, NGHTTP2_FLAG_END_PUSH_PROMISE, 1000000007, (1U << 31) - 1, nva, nvlen); - framelen = nghttp2_frame_pack_push_promise(&buf, &buflen, &frame, &deflater); + framelen = nghttp2_frame_pack_push_promise(&buf, &buflen, &bufoff, &frame, + &deflater); - CU_ASSERT(0 == unpack_frame((nghttp2_frame*)&oframe, buf, framelen)); - check_frame_header(framelen - NGHTTP2_FRAME_HEAD_LENGTH, + CU_ASSERT(0 == unpack_frame((nghttp2_frame*)&oframe, + buf + bufoff, framelen - bufoff)); + check_frame_header(framelen - bufoff - NGHTTP2_FRAME_HEAD_LENGTH, NGHTTP2_PUSH_PROMISE, NGHTTP2_FLAG_END_PUSH_PROMISE, 1000000007, &oframe.hd); CU_ASSERT((1U << 31) - 1 == oframe.promised_stream_id); - CU_ASSERT(framelen - 12 == - inflate_hd(&inflater, &out, buf + 12, framelen - 12)); + nv_offset = bufoff + NGHTTP2_FRAME_HEAD_LENGTH + 4; + CU_ASSERT(framelen - nv_offset == + inflate_hd(&inflater, &out, buf + nv_offset, framelen - nv_offset)); CU_ASSERT(7 == out.nvlen); CU_ASSERT(nvnameeq("method", &out.nva[0])); diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index dca5ba86..fdb2c524 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -4044,7 +4044,7 @@ void test_nghttp2_session_pack_headers_with_padding(void) nghttp2_session *session, *sv_session; accumulator acc; my_user_data ud; - nghttp2_session_callbacks callbacks, sv_callbacks; + nghttp2_session_callbacks callbacks; nghttp2_nv nva[8190]; size_t i; @@ -4059,15 +4059,13 @@ void test_nghttp2_session_pack_headers_with_padding(void) callbacks.send_callback = accumulator_send_callback; callbacks.on_frame_send_callback = on_frame_send_callback; callbacks.select_padding_callback = select_padding_callback; + callbacks.on_frame_recv_callback = on_frame_recv_callback; acc.length = 0; ud.acc = &acc; - memset(&sv_callbacks, 0, sizeof(sv_callbacks)); - sv_callbacks.on_frame_recv_callback = on_frame_recv_callback; - nghttp2_session_client_new(&session, &callbacks, &ud); - nghttp2_session_server_new(&sv_session, &sv_callbacks, &ud); + nghttp2_session_server_new(&sv_session, &callbacks, &ud); ud.padding_boundary = 16385; @@ -4083,6 +4081,20 @@ void test_nghttp2_session_pack_headers_with_padding(void) CU_ASSERT(1 == ud.frame_recv_cb_called); CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(sv_session)); + /* Check PUSH_PROMISE */ + CU_ASSERT(0 == + nghttp2_submit_push_promise(sv_session, NGHTTP2_FLAG_NONE, 1, + nva, ARRLEN(nva))); + acc.length = 0; + CU_ASSERT(0 == nghttp2_session_send(sv_session)); + + CU_ASSERT(acc.length > NGHTTP2_MAX_FRAME_LENGTH); + ud.frame_recv_cb_called = 0; + CU_ASSERT((ssize_t)acc.length == + nghttp2_session_mem_recv(session, acc.buf, acc.length)); + CU_ASSERT(1 == ud.frame_recv_cb_called); + CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session)); + nghttp2_session_del(sv_session); nghttp2_session_del(session); }