From b5717cd2888360e48f1685d47a128f3b866baaf6 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 11 Jun 2015 23:34:30 +0900 Subject: [PATCH] Fix bug that data are not consumed for connection in race condition When we know that stream is closed at time we read DATA frame header, we use NGHTTP2_IB_IGN_DATA, and consume data for connection if nghttp2_option_set_no_auto_window_update() is used. However, if stream is closed while we are in NGHTTP2_IB_READ_DATA, those bytes are not consumed for connection, nor notified to application via callback, so it eventually fills up connection window and connection will freeze. This commit fixes this issue by consuming these data for connection when stream is closed or does not exist. --- lib/nghttp2_session.c | 83 ++++++++++++++++++++---------------- tests/main.c | 2 + tests/nghttp2_session_test.c | 56 ++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 4 files changed, 105 insertions(+), 37 deletions(-) diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 4f5c3b61..6c96f4bf 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -5760,51 +5760,60 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } - } - data_readlen = inbound_frame_effective_readlen( - iframe, iframe->payloadleft, readlen); + data_readlen = inbound_frame_effective_readlen( + iframe, iframe->payloadleft, readlen); - padlen = readlen - data_readlen; + padlen = readlen - data_readlen; - if (padlen > 0) { - /* Padding is considered as "consumed" immediately */ - rv = nghttp2_session_consume(session, iframe->frame.hd.stream_id, - padlen); + if (padlen > 0) { + /* Padding is considered as "consumed" immediately */ + rv = nghttp2_session_consume(session, iframe->frame.hd.stream_id, + padlen); + + if (nghttp2_is_fatal(rv)) { + return rv; + } + } + + DEBUGF(fprintf(stderr, "recv: data_readlen=%zd\n", data_readlen)); + + if (data_readlen > 0) { + if (session_enforce_http_messaging(session)) { + if (nghttp2_http_on_data_chunk(stream, data_readlen) != 0) { + rv = nghttp2_session_add_rst_stream(session, + iframe->frame.hd.stream_id, + NGHTTP2_PROTOCOL_ERROR); + if (nghttp2_is_fatal(rv)) { + return rv; + } + busy = 1; + iframe->state = NGHTTP2_IB_IGN_DATA; + break; + } + } + if (session->callbacks.on_data_chunk_recv_callback) { + rv = session->callbacks.on_data_chunk_recv_callback( + session, iframe->frame.hd.flags, iframe->frame.hd.stream_id, + in - readlen, data_readlen, session->user_data); + if (rv == NGHTTP2_ERR_PAUSE) { + return in - first; + } + + if (nghttp2_is_fatal(rv)) { + return NGHTTP2_ERR_CALLBACK_FAILURE; + } + } + } + } else if (session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE) { + /* stream was closed or does not exist. Consume all data + for connection immediately here */ + rv = session_update_connection_consumed_size(session, readlen); if (nghttp2_is_fatal(rv)) { return rv; } } - - DEBUGF(fprintf(stderr, "recv: data_readlen=%zd\n", data_readlen)); - - if (stream && data_readlen > 0) { - if (session_enforce_http_messaging(session)) { - if (nghttp2_http_on_data_chunk(stream, data_readlen) != 0) { - rv = nghttp2_session_add_rst_stream( - session, iframe->frame.hd.stream_id, NGHTTP2_PROTOCOL_ERROR); - if (nghttp2_is_fatal(rv)) { - return rv; - } - busy = 1; - iframe->state = NGHTTP2_IB_IGN_DATA; - break; - } - } - if (session->callbacks.on_data_chunk_recv_callback) { - rv = session->callbacks.on_data_chunk_recv_callback( - session, iframe->frame.hd.flags, iframe->frame.hd.stream_id, - in - readlen, data_readlen, session->user_data); - if (rv == NGHTTP2_ERR_PAUSE) { - return in - first; - } - - if (nghttp2_is_fatal(rv)) { - return NGHTTP2_ERR_CALLBACK_FAILURE; - } - } - } } if (iframe->payloadleft) { diff --git a/tests/main.c b/tests/main.c index 1d187f70..d15e3c13 100644 --- a/tests/main.c +++ b/tests/main.c @@ -80,6 +80,8 @@ int main(int argc _U_, char *argv[] _U_) { !CU_add_test(pSuite, "session_recv_eof", test_nghttp2_session_recv_eof) || !CU_add_test(pSuite, "session_recv_data", test_nghttp2_session_recv_data) || + !CU_add_test(pSuite, "session_recv_data_no_auto_flow_control", + test_nghttp2_session_recv_data_no_auto_flow_control) || !CU_add_test(pSuite, "session_recv_continuation", test_nghttp2_session_recv_continuation) || !CU_add_test(pSuite, "session_recv_headers_with_priority", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index fa2791f2..d1471f59 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -800,6 +800,62 @@ void test_nghttp2_session_recv_data(void) { nghttp2_session_del(session); } +void test_nghttp2_session_recv_data_no_auto_flow_control(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + my_user_data ud; + nghttp2_option *option; + nghttp2_frame_hd hd; + size_t padlen; + uint8_t data[8192]; + ssize_t rv; + size_t sendlen; + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.send_callback = null_send_callback; + + nghttp2_option_new(&option); + nghttp2_option_set_no_auto_window_update(option, 1); + + nghttp2_session_server_new2(&session, &callbacks, &ud, option); + + /* Create DATA frame with length 4KiB + 11 bytes padding*/ + padlen = 11; + memset(data, 0, sizeof(data)); + hd.length = 4096 + 1 + padlen; + hd.type = NGHTTP2_DATA; + hd.flags = NGHTTP2_FLAG_PADDED; + hd.stream_id = 1; + nghttp2_frame_pack_frame_hd(data, &hd); + data[NGHTTP2_FRAME_HDLEN] = padlen; + + /* First create stream 1, then close it. Check that data is + consumed for connection in this situation */ + open_stream(session, 1); + + /* Receive first 100 bytes */ + sendlen = 100; + rv = nghttp2_session_mem_recv(session, data, sendlen); + CU_ASSERT((ssize_t)sendlen == rv); + + /* close stream here */ + nghttp2_submit_rst_stream(session, NGHTTP2_FLAG_NONE, 1, NGHTTP2_NO_ERROR); + nghttp2_session_send(session); + + /* stream 1 has been closed, and we disabled auto flow-control, so + data must be immediately consumed for connection. */ + rv = nghttp2_session_mem_recv(session, data + sendlen, + NGHTTP2_FRAME_HDLEN + hd.length - sendlen); + CU_ASSERT((ssize_t)(NGHTTP2_FRAME_HDLEN + hd.length - sendlen) == rv); + + /* We already consumed pad length field (1 byte), so do +1 here */ + CU_ASSERT((int32_t)(NGHTTP2_FRAME_HDLEN + hd.length - sendlen + 1) == + session->consumed_size); + + nghttp2_session_del(session); + nghttp2_option_del(option); +} + void test_nghttp2_session_recv_continuation(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index e879c783..3b4b2b5d 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -30,6 +30,7 @@ void test_nghttp2_session_recv_invalid_stream_id(void); void test_nghttp2_session_recv_invalid_frame(void); void test_nghttp2_session_recv_eof(void); void test_nghttp2_session_recv_data(void); +void test_nghttp2_session_recv_data_no_auto_flow_control(void); void test_nghttp2_session_recv_continuation(void); void test_nghttp2_session_recv_headers_with_priority(void); void test_nghttp2_session_recv_premature_headers(void);