diff --git a/lib/nghttp2_int.h b/lib/nghttp2_int.h index c26c8e99..9ab8c0ab 100644 --- a/lib/nghttp2_int.h +++ b/lib/nghttp2_int.h @@ -52,7 +52,8 @@ typedef enum { * Invalid HTTP header field was received but it can be treated as * if it was not received because of compatibility reasons. */ - NGHTTP2_ERR_IGN_HTTP_HEADER = -105 + NGHTTP2_ERR_IGN_HTTP_HEADER = -105, + NGHTTP2_ERR_FLOODING_DETECTED = -106 } nghttp2_internal_error; #endif /* NGHTTP2_INT_H */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d8335880..5501ea77 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -141,6 +141,15 @@ static int session_detect_idle_stream(nghttp2_session *session, return 0; } +static void session_on_flooding_detected(nghttp2_session *session) { + /* If we found flooding, we might not send GOAWAY since peer might + not read at all. So we just set these flags to pretend that + GOAWAY is sent, so that nghttp2_session_want_read() and + nghttp2_session_want_write() return 0. */ + session->goaway_flags |= NGHTTP2_GOAWAY_TERM_ON_SEND | + NGHTTP2_GOAWAY_TERM_SENT | NGHTTP2_GOAWAY_SENT; +} + static int session_terminate_session(nghttp2_session *session, int32_t last_stream_id, uint32_t error_code, const char *reason) { @@ -1853,6 +1862,11 @@ static int session_prep_frame(nghttp2_session *session, &frame->rst_stream); break; case NGHTTP2_SETTINGS: { + if (frame->hd.flags & NGHTTP2_FLAG_ACK) { + assert(session->obq_flood_counter_ > 0); + --session->obq_flood_counter_; + } + rv = nghttp2_frame_pack_settings(&session->aob.framebufs, &frame->settings); if (rv != 0) { @@ -1914,6 +1928,11 @@ static int session_prep_frame(nghttp2_session *session, break; } case NGHTTP2_PING: + if (frame->hd.flags & NGHTTP2_FLAG_ACK) { + assert(session->obq_flood_counter_ > 0); + --session->obq_flood_counter_; + } + if (session_is_closing(session)) { return NGHTTP2_ERR_SESSION_CLOSING; } @@ -5908,6 +5927,13 @@ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, nghttp2_mem *mem; mem = &session->mem; + + if ((flags & NGHTTP2_FLAG_ACK) && + session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + session_on_flooding_detected(session); + return NGHTTP2_ERR_FLOODING_DETECTED; + } + item = nghttp2_mem_malloc(mem, sizeof(nghttp2_outbound_item)); if (item == NULL) { return NGHTTP2_ERR_NOMEM; @@ -5926,6 +5952,11 @@ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, nghttp2_mem_free(mem, item); return rv; } + + if (flags & NGHTTP2_FLAG_ACK) { + ++session->obq_flood_counter_; + } + return 0; } @@ -6044,8 +6075,15 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, mem = &session->mem; - if ((flags & NGHTTP2_FLAG_ACK) && niv != 0) { - return NGHTTP2_ERR_INVALID_ARGUMENT; + if (flags & NGHTTP2_FLAG_ACK) { + if (niv != 0) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + + if (session->obq_flood_counter_ >= NGHTTP2_MAX_OBQ_FLOOD_ITEM) { + session_on_flooding_detected(session); + return NGHTTP2_ERR_FLOODING_DETECTED; + } } if (!nghttp2_iv_check(iv, niv)) { @@ -6095,6 +6133,10 @@ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, return rv; } + if (flags & NGHTTP2_FLAG_ACK) { + ++session->obq_flood_counter_; + } + session_append_inflight_settings(session, inflight_settings); /* Extract NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS and ENABLE_PUSH diff --git a/lib/nghttp2_session.h b/lib/nghttp2_session.h index 4594c57e..e8e92ee9 100644 --- a/lib/nghttp2_session.h +++ b/lib/nghttp2_session.h @@ -74,6 +74,16 @@ typedef struct { /* The default maximum number of incoming reserved streams */ #define NGHTTP2_MAX_INCOMING_RESERVED_STREAMS 200 +/* The maximum number of items in outbound queue, which is considered + as flooding caused by peer. All frames are not considered here. + We only consider PING + ACK and SETTINGS + ACK. This is because + they both are response to the frame initiated by peer and peer can + send as many of them as they want. If peer does not read network, + response frames are stacked up, which leads to memory exhaustion. + The value selected here is arbitrary, but safe value and if we have + these frames in this number, it is considered suspicious. */ +#define NGHTTP2_MAX_OBQ_FLOOD_ITEM 1000 + /* Internal state when receiving incoming frame */ typedef enum { /* Receiving frame header */ @@ -227,6 +237,8 @@ struct nghttp2_session { size_t num_idle_streams; /* The number of bytes allocated for nvbuf */ size_t nvbuflen; + /* Counter for detecting flooding in outbound queue */ + size_t obq_flood_counter_; /* Next Stream ID. Made unsigned int to detect >= (1 << 31). */ uint32_t next_stream_id; /* The largest stream ID received so far */ @@ -355,6 +367,9 @@ int nghttp2_session_add_rst_stream(nghttp2_session *session, int32_t stream_id, * * NGHTTP2_ERR_NOMEM * Out of memory. + * NGHTTP2_ERR_FLOODING_DETECTED + * There are too many items in outbound queue; this only happens + * if NGHTTP2_FLAG_ACK is set in |flags| */ int nghttp2_session_add_ping(nghttp2_session *session, uint8_t flags, const uint8_t *opaque_data); @@ -402,6 +417,9 @@ int nghttp2_session_add_window_update(nghttp2_session *session, uint8_t flags, * * NGHTTP2_ERR_NOMEM * Out of memory. + * NGHTTP2_ERR_FLOODING_DETECTED + * There are too many items in outbound queue; this only happens + * if NGHTTP2_FLAG_ACK is set in |flags| */ int nghttp2_session_add_settings(nghttp2_session *session, uint8_t flags, const nghttp2_settings_entry *iv, size_t niv); diff --git a/tests/main.c b/tests/main.c index 5ee0e176..748b578b 100644 --- a/tests/main.c +++ b/tests/main.c @@ -277,6 +277,7 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_defer_then_close) || !CU_add_test(pSuite, "session_detach_item_from_closed_stream", test_nghttp2_session_detach_item_from_closed_stream) || + !CU_add_test(pSuite, "session_flooding", test_nghttp2_session_flooding) || !CU_add_test(pSuite, "http_mandatory_headers", test_nghttp2_http_mandatory_headers) || !CU_add_test(pSuite, "http_content_length", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 31423d1c..c4429b59 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -8090,6 +8090,76 @@ void test_nghttp2_session_detach_item_from_closed_stream(void) { nghttp2_session_del(session); } +void test_nghttp2_session_flooding(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + nghttp2_bufs bufs; + nghttp2_buf *buf; + nghttp2_frame frame; + nghttp2_mem *mem; + + mem = nghttp2_mem_default(); + + frame_pack_bufs_init(&bufs); + + memset(&callbacks, 0, sizeof(callbacks)); + + /* PING ACK */ + nghttp2_session_server_new(&session, &callbacks, NULL); + + nghttp2_frame_ping_init(&frame.ping, NGHTTP2_FLAG_NONE, NULL); + nghttp2_frame_pack_ping(&bufs, &frame.ping); + nghttp2_frame_ping_free(&frame.ping); + + buf = &bufs.head->buf; + + for (int i = 0; i < NGHTTP2_MAX_OBQ_FLOOD_ITEM; ++i) { + CU_ASSERT( + (ssize_t)nghttp2_buf_len(buf) == + nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf))); + } + + CU_ASSERT(1 == nghttp2_session_want_read(session)); + CU_ASSERT(1 == nghttp2_session_want_write(session)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(buf) == + nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf))); + + CU_ASSERT(0 == nghttp2_session_want_read(session)); + CU_ASSERT(0 == nghttp2_session_want_write(session)); + + nghttp2_session_del(session); + + /* SETTINGS ACK */ + nghttp2_bufs_reset(&bufs); + + nghttp2_session_server_new(&session, &callbacks, NULL); + + nghttp2_frame_settings_init(&frame.settings, NGHTTP2_FLAG_NONE, NULL, 0); + nghttp2_frame_pack_settings(&bufs, &frame.settings); + nghttp2_frame_settings_free(&frame.settings, mem); + + buf = &bufs.head->buf; + + for (int i = 0; i < NGHTTP2_MAX_OBQ_FLOOD_ITEM; ++i) { + CU_ASSERT( + (ssize_t)nghttp2_buf_len(buf) == + nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf))); + } + + CU_ASSERT(1 == nghttp2_session_want_read(session)); + CU_ASSERT(1 == nghttp2_session_want_write(session)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(buf) == + nghttp2_session_mem_recv(session, buf->pos, nghttp2_buf_len(buf))); + + CU_ASSERT(0 == nghttp2_session_want_read(session)); + CU_ASSERT(0 == nghttp2_session_want_write(session)); + + nghttp2_session_del(session); + nghttp2_bufs_free(&bufs); +} + static void check_nghttp2_http_recv_headers_fail( nghttp2_session *session, nghttp2_hd_deflater *deflater, int32_t stream_id, int stream_state, const nghttp2_nv *nva, size_t nvlen) { diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 20774695..ce08b5f9 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -131,6 +131,7 @@ void test_nghttp2_session_send_data_callback(void); void test_nghttp2_session_on_begin_headers_temporal_failure(void); void test_nghttp2_session_defer_then_close(void); void test_nghttp2_session_detach_item_from_closed_stream(void); +void test_nghttp2_session_flooding(void); void test_nghttp2_http_mandatory_headers(void); void test_nghttp2_http_content_length(void); void test_nghttp2_http_content_length_mismatch(void);