From 514558afc01dd58ad2167224a903950cb4075504 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 23 Apr 2015 23:43:30 +0900 Subject: [PATCH] Allow NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from nghttp2_on_begin_headers_callback Since application most likely allocates the stream object in nghttp2_on_begin_headers_callback, it is desirable to handle its failure as stream error. But previously it only signals success or fatal error. Submitting RST_STREAM does not prevent nghttp2_on_header_callback from being invoked. This commit improves this situation by allowing NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE from nghttp2_on_begin_headers_callback. If that value is returned, library submits RST_STREAM with error code INTERNAL_ERROR, and nghttp2_on_header_callback and nghttp2_on_frame_recv_callback for that frame are not invoked. Note that for PUSH_PROMISE frame, the stream to be reset is promised stream. --- lib/includes/nghttp2/nghttp2.h | 23 +++++++--- lib/nghttp2_session.c | 34 +++++++++++++++ tests/main.c | 2 + tests/nghttp2_session_test.c | 78 ++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 5 files changed, 133 insertions(+), 5 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 0c6122c2..9caafe2e 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -1502,11 +1502,24 @@ typedef int (*nghttp2_on_stream_close_callback)(nghttp2_session *session, * trailer headers also has ``frame->headers.cat == * NGHTTP2_HCAT_HEADERS`` which does not contain any status code. * - * The implementation of this function must return 0 if it succeeds or - * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE`. If nonzero value other than - * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` is returned, it is treated as - * if :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` is returned. If - * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` is returned, + * Returning :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE` will close + * the stream (promised stream if frame is PUSH_PROMISE) by issuing + * RST_STREAM with :enum:`NGHTTP2_INTERNAL_ERROR`. In this case, + * :type:`nghttp2_on_header_callback` and + * :type:`nghttp2_on_frame_recv_callback` will not be invoked. If a + * different error code is desirable, use + * `nghttp2_submit_rst_stream()` with a desired error code and then + * return :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE`. Again, use + * `frame->push_promise.promised_stream_id` as stream_id parameter in + * `nghttp2_submit_rst_stream()` if frame is PUSH_PROMISE. + * + * The implementation of this function must return 0 if it succeeds. + * It can return :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE` to + * reset the stream (promised stream if frame is PUSH_PROMISE). For + * critical errors, it must return + * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE`. If the other value is + * returned, it is treated as if :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` + * is returned. If :enum:`NGHTTP2_ERR_CALLBACK_FAILURE` is returned, * `nghttp2_session_mem_recv()` function will immediately return * :enum:`NGHTTP2_ERR_CALLBACK_FAILURE`. * diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 108a30c1..c4f61355 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3069,6 +3069,9 @@ static int session_call_on_begin_headers(nghttp2_session *session, if (session->callbacks.on_begin_headers_callback) { rv = session->callbacks.on_begin_headers_callback(session, frame, session->user_data); + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + return rv; + } if (rv != 0) { return NGHTTP2_ERR_CALLBACK_FAILURE; } @@ -5085,6 +5088,16 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + rv = nghttp2_session_add_rst_stream( + session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR); + if (nghttp2_is_fatal(rv)) { + return rv; + } + iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; + break; + } + if (rv == NGHTTP2_ERR_IGN_HEADER_BLOCK) { iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; break; @@ -5329,6 +5342,16 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + rv = nghttp2_session_add_rst_stream( + session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR); + if (nghttp2_is_fatal(rv)) { + return rv; + } + iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; + break; + } + if (rv == NGHTTP2_ERR_IGN_HEADER_BLOCK) { iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; break; @@ -5393,6 +5416,17 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + rv = nghttp2_session_add_rst_stream( + session, iframe->frame.push_promise.promised_stream_id, + NGHTTP2_INTERNAL_ERROR); + if (nghttp2_is_fatal(rv)) { + return rv; + } + iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; + break; + } + if (rv == NGHTTP2_ERR_IGN_HEADER_BLOCK) { iframe->state = NGHTTP2_IB_IGN_HEADER_BLOCK; break; diff --git a/tests/main.c b/tests/main.c index 09a92124..933da918 100644 --- a/tests/main.c +++ b/tests/main.c @@ -262,6 +262,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_reset_pending_headers) || !CU_add_test(pSuite, "session_send_data_callback", test_nghttp2_session_send_data_callback) || + !CU_add_test(pSuite, "session_on_begin_headers_temporal_failure", + test_nghttp2_session_on_begin_headers_temporal_failure) || !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 a685918b..3e83d2b4 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -405,6 +405,12 @@ static int on_begin_headers_callback(nghttp2_session *session _U_, return 0; } +static int temporal_failure_on_begin_headers_callback( + nghttp2_session *session, const nghttp2_frame *frame, void *user_data) { + on_begin_headers_callback(session, frame, user_data); + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; +} + static ssize_t defer_data_source_read_callback(nghttp2_session *session _U_, int32_t stream_id _U_, uint8_t *buf _U_, size_t len _U_, @@ -7033,6 +7039,78 @@ void test_nghttp2_session_send_data_callback(void) { nghttp2_session_del(session); } +void test_nghttp2_session_on_begin_headers_temporal_failure(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + my_user_data ud; + nghttp2_bufs bufs; + nghttp2_mem *mem; + ssize_t rv; + nghttp2_hd_deflater deflater; + nghttp2_outbound_item *item; + + mem = nghttp2_mem_default(); + frame_pack_bufs_init(&bufs); + nghttp2_hd_deflate_init(&deflater, mem); + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.on_begin_headers_callback = + temporal_failure_on_begin_headers_callback; + callbacks.on_header_callback = on_header_callback; + callbacks.on_frame_recv_callback = on_frame_recv_callback; + callbacks.send_callback = null_send_callback; + nghttp2_session_server_new(&session, &callbacks, &ud); + + rv = pack_headers(&bufs, &deflater, 1, NGHTTP2_FLAG_END_HEADERS, reqnv, + ARRLEN(reqnv), mem); + CU_ASSERT(0 == rv); + + ud.header_cb_called = 0; + ud.frame_recv_cb_called = 0; + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_bufs_len(&bufs)); + CU_ASSERT(nghttp2_bufs_len(&bufs) == rv); + CU_ASSERT(0 == ud.header_cb_called); + CU_ASSERT(0 == ud.frame_recv_cb_called); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + CU_ASSERT(1 == item->frame.hd.stream_id); + CU_ASSERT(NGHTTP2_INTERNAL_ERROR == item->frame.rst_stream.error_code); + + nghttp2_session_del(session); + nghttp2_hd_deflate_free(&deflater); + + nghttp2_bufs_reset(&bufs); + /* check for PUSH_PROMISE */ + nghttp2_hd_deflate_init(&deflater, mem); + nghttp2_session_client_new(&session, &callbacks, &ud); + + nghttp2_session_open_stream(session, 1, NGHTTP2_STREAM_FLAG_NONE, + &pri_spec_default, NGHTTP2_STREAM_OPENING, NULL); + + rv = pack_push_promise(&bufs, &deflater, 1, NGHTTP2_FLAG_END_HEADERS, 2, + reqnv, ARRLEN(reqnv), mem); + CU_ASSERT(0 == rv); + + ud.header_cb_called = 0; + ud.frame_recv_cb_called = 0; + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_bufs_len(&bufs)); + CU_ASSERT(nghttp2_bufs_len(&bufs) == rv); + CU_ASSERT(0 == ud.header_cb_called); + CU_ASSERT(0 == ud.frame_recv_cb_called); + + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + CU_ASSERT(2 == item->frame.hd.stream_id); + CU_ASSERT(NGHTTP2_INTERNAL_ERROR == item->frame.rst_stream.error_code); + + nghttp2_session_del(session); + nghttp2_hd_deflate_free(&deflater); + 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 b6fe967d..1b4d2f64 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -124,6 +124,7 @@ void test_nghttp2_session_open_idle_stream(void); void test_nghttp2_session_cancel_reserved_remote(void); void test_nghttp2_session_reset_pending_headers(void); void test_nghttp2_session_send_data_callback(void); +void test_nghttp2_session_on_begin_headers_temporal_failure(void); void test_nghttp2_http_mandatory_headers(void); void test_nghttp2_http_content_length(void); void test_nghttp2_http_content_length_mismatch(void);