From 269a10008177d359e978d6a256d00dedb8837b52 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 7 Nov 2015 11:30:15 +0900 Subject: [PATCH] Add nghttp2_session_upgrade2(), deprecate nghttp2_session_upgrade() To validate actual response body length against the value declared in content-length response header field, we first check request method. If request method is HEAD, respose body must be 0 regardless of the value in content-length. nghttp2_session_upgrade() has no parameter to indicate the request method is HEAD, so we failed to validate response body if HEAD is used with HTTP Upgrade. New nghttp2_session_upgrade2() accepts new parameter to indicate that request method is HEAD or not to fix this issue. Although, this issue affects client side only, we deprecate nghttp2_session_upgrade() in favor of nghttp2_session_upgrade2() for both client and server side. --- doc/Makefile.am | 1 + lib/includes/nghttp2/nghttp2.h | 56 ++++++++++++++++++++++++++++++++ lib/nghttp2_session.c | 58 +++++++++++++++++++++++++++++----- tests/main.c | 2 +- tests/nghttp2_session_test.c | 26 +++++++-------- tests/nghttp2_session_test.h | 2 +- 6 files changed, 122 insertions(+), 23 deletions(-) diff --git a/doc/Makefile.am b/doc/Makefile.am index d3055c38..7b6caaee 100644 --- a/doc/Makefile.am +++ b/doc/Makefile.am @@ -116,6 +116,7 @@ APIDOCS= \ nghttp2_session_terminate_session.rst \ nghttp2_session_terminate_session2.rst \ nghttp2_session_upgrade.rst \ + nghttp2_session_upgrade2.rst \ nghttp2_session_want_read.rst \ nghttp2_session_want_write.rst \ nghttp2_stream_get_first_child.rst \ diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index b4d79429..82749969 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -2858,6 +2858,17 @@ NGHTTP2_EXTERN int nghttp2_session_consume_stream(nghttp2_session *session, * be called from both client and server, but the behavior is very * different in each other. * + * .. warning:: + * + * This function is deprecated in favor of + * `nghttp2_session_upgrade2()`, because this function lacks the + * parameter to tell the library the request method used in the + * original HTTP request. This information is required for client + * to validate actual response body length against content-length + * header field (see `nghttp2_option_set_no_http_messaging()`). If + * HEAD is used in request, the length of response body must be 0 + * regardless of value included in content-length header field. + * * If called from client side, the |settings_payload| must be the * value sent in ``HTTP2-Settings`` header field and must be decoded * by base64url decoder. The |settings_payloadlen| is the length of @@ -2892,6 +2903,51 @@ NGHTTP2_EXTERN int nghttp2_session_upgrade(nghttp2_session *session, size_t settings_payloadlen, void *stream_user_data); +/** + * @function + * + * Performs post-process of HTTP Upgrade request. This function can + * be called from both client and server, but the behavior is very + * different in each other. + * + * If called from client side, the |settings_payload| must be the + * value sent in ``HTTP2-Settings`` header field and must be decoded + * by base64url decoder. The |settings_payloadlen| is the length of + * |settings_payload|. The |settings_payload| is unpacked and its + * setting values will be submitted using `nghttp2_submit_settings()`. + * This means that the client application code does not need to submit + * SETTINGS by itself. The stream with stream ID=1 is opened and the + * |stream_user_data| is used for its stream_user_data. The opened + * stream becomes half-closed (local) state. + * + * If called from server side, the |settings_payload| must be the + * value received in ``HTTP2-Settings`` header field and must be + * decoded by base64url decoder. The |settings_payloadlen| is the + * length of |settings_payload|. It is treated as if the SETTINGS + * frame with that payload is received. Thus, callback functions for + * the reception of SETTINGS frame will be invoked. The stream with + * stream ID=1 is opened. The |stream_user_data| is ignored. The + * opened stream becomes half-closed (remote). + * + * If the request method is HEAD, pass nonzero value to + * |head_request|. Otherwise, pass 0. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * :enum:`NGHTTP2_ERR_NOMEM` + * Out of memory. + * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` + * The |settings_payload| is badly formed. + * :enum:`NGHTTP2_ERR_PROTO` + * The stream ID 1 is already used or closed; or is not available. + */ +NGHTTP2_EXTERN int nghttp2_session_upgrade2(nghttp2_session *session, + const uint8_t *settings_payload, + size_t settings_payloadlen, + int head_request, + void *stream_user_data); + /** * @function * diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 07ba9608..dfb29a0d 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -6502,10 +6502,10 @@ uint32_t nghttp2_session_get_remote_settings(nghttp2_session *session, assert(0); } -int nghttp2_session_upgrade(nghttp2_session *session, - const uint8_t *settings_payload, - size_t settings_payloadlen, - void *stream_user_data) { +static int nghttp2_session_upgrade_internal(nghttp2_session *session, + const uint8_t *settings_payload, + size_t settings_payloadlen, + void *stream_user_data) { nghttp2_stream *stream; nghttp2_frame frame; nghttp2_settings_entry *iv; @@ -6559,19 +6559,61 @@ int nghttp2_session_upgrade(nghttp2_session *session, nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); session->next_stream_id += 2; } + return 0; +} + +int nghttp2_session_upgrade(nghttp2_session *session, + const uint8_t *settings_payload, + size_t settings_payloadlen, + void *stream_user_data) { + int rv; + nghttp2_stream *stream; + + rv = nghttp2_session_upgrade_internal(session, settings_payload, + settings_payloadlen, stream_user_data); + if (rv != 0) { + return rv; + } + + stream = nghttp2_session_get_stream(session, 1); + assert(stream); + /* We have no information about request header fields when Upgrade was happened. So we don't know the request method here. If request method is HEAD, we have a trouble because we may have nonzero content-length header field in response headers, and we will going to check it against the actual DATA frames, but we may - get mismatch because HEAD response body must be empty. We will - add new version of nghttp2_session_upgrade with the parameter to - pass the request method information so that we can handle this - situation properly. */ + get mismatch because HEAD response body must be empty. Because + of this reason, nghttp2_session_upgrade() was deprecated in favor + of nghttp2_session_upgrade2(), which has |head_request| parameter + to indicate that request method is HEAD or not. */ stream->http_flags |= NGHTTP2_HTTP_FLAG_METH_UPGRADE_WORKAROUND; return 0; } +int nghttp2_session_upgrade2(nghttp2_session *session, + const uint8_t *settings_payload, + size_t settings_payloadlen, int head_request, + void *stream_user_data) { + int rv; + nghttp2_stream *stream; + + rv = nghttp2_session_upgrade_internal(session, settings_payload, + settings_payloadlen, stream_user_data); + if (rv != 0) { + return rv; + } + + stream = nghttp2_session_get_stream(session, 1); + assert(stream); + + if (head_request) { + stream->http_flags |= NGHTTP2_HTTP_FLAG_METH_HEAD; + } + + return 0; +} + int nghttp2_session_get_stream_local_close(nghttp2_session *session, int32_t stream_id) { nghttp2_stream *stream; diff --git a/tests/main.c b/tests/main.c index 2887d071..cc3896c0 100644 --- a/tests/main.c +++ b/tests/main.c @@ -138,7 +138,7 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_session_send_push_promise) || !CU_add_test(pSuite, "session_is_my_stream_id", test_nghttp2_session_is_my_stream_id) || - !CU_add_test(pSuite, "session_upgrade", test_nghttp2_session_upgrade) || + !CU_add_test(pSuite, "session_upgrade2", test_nghttp2_session_upgrade2) || !CU_add_test(pSuite, "session_reprioritize_stream", test_nghttp2_session_reprioritize_stream) || !CU_add_test( diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 632c606a..758ada16 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3331,7 +3331,7 @@ void test_nghttp2_session_is_my_stream_id(void) { nghttp2_session_del(session); } -void test_nghttp2_session_upgrade(void) { +void test_nghttp2_session_upgrade2(void) { nghttp2_session *session; nghttp2_session_callbacks callbacks; uint8_t settings_payload[128]; @@ -3351,8 +3351,8 @@ void test_nghttp2_session_upgrade(void) { /* Check client side */ nghttp2_session_client_new(&session, &callbacks, NULL); - CU_ASSERT(0 == nghttp2_session_upgrade(session, settings_payload, - settings_payloadlen, &callbacks)); + CU_ASSERT(0 == nghttp2_session_upgrade2(session, settings_payload, + settings_payloadlen, 0, &callbacks)); stream = nghttp2_session_get_stream(session, 1); CU_ASSERT(stream != NULL); CU_ASSERT(&callbacks == stream->stream_user_data); @@ -3367,16 +3367,16 @@ void test_nghttp2_session_upgrade(void) { item->frame.settings.iv[1].settings_id); CU_ASSERT(4095 == item->frame.settings.iv[1].value); - /* Call nghttp2_session_upgrade() again is error */ + /* Call nghttp2_session_upgrade2() again is error */ CU_ASSERT(NGHTTP2_ERR_PROTO == - nghttp2_session_upgrade(session, settings_payload, - settings_payloadlen, &callbacks)); + nghttp2_session_upgrade2(session, settings_payload, + settings_payloadlen, 0, &callbacks)); nghttp2_session_del(session); /* Check server side */ nghttp2_session_server_new(&session, &callbacks, NULL); - CU_ASSERT(0 == nghttp2_session_upgrade(session, settings_payload, - settings_payloadlen, &callbacks)); + CU_ASSERT(0 == nghttp2_session_upgrade2(session, settings_payload, + settings_payloadlen, 0, &callbacks)); stream = nghttp2_session_get_stream(session, 1); CU_ASSERT(stream != NULL); CU_ASSERT(NULL == stream->stream_user_data); @@ -3384,10 +3384,10 @@ void test_nghttp2_session_upgrade(void) { CU_ASSERT(NULL == nghttp2_session_get_next_ob_item(session)); CU_ASSERT(1 == session->remote_settings.max_concurrent_streams); CU_ASSERT(4095 == session->remote_settings.initial_window_size); - /* Call nghttp2_session_upgrade() again is error */ + /* Call nghttp2_session_upgrade2() again is error */ CU_ASSERT(NGHTTP2_ERR_PROTO == - nghttp2_session_upgrade(session, settings_payload, - settings_payloadlen, &callbacks)); + nghttp2_session_upgrade2(session, settings_payload, + settings_payloadlen, 0, &callbacks)); nghttp2_session_del(session); /* Empty SETTINGS is OK */ @@ -3395,8 +3395,8 @@ void test_nghttp2_session_upgrade(void) { settings_payload, sizeof(settings_payload), NULL, 0); nghttp2_session_client_new(&session, &callbacks, NULL); - CU_ASSERT(0 == nghttp2_session_upgrade(session, settings_payload, - settings_payloadlen, NULL)); + CU_ASSERT(0 == nghttp2_session_upgrade2(session, settings_payload, + settings_payloadlen, 0, NULL)); nghttp2_session_del(session); } diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index 979dbc46..80aa8edd 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -59,7 +59,7 @@ void test_nghttp2_session_send_headers_push_reply(void); void test_nghttp2_session_send_rst_stream(void); void test_nghttp2_session_send_push_promise(void); void test_nghttp2_session_is_my_stream_id(void); -void test_nghttp2_session_upgrade(void); +void test_nghttp2_session_upgrade2(void); void test_nghttp2_session_reprioritize_stream(void); void test_nghttp2_session_reprioritize_stream_with_idle_stream_dep(void); void test_nghttp2_submit_data(void);