From 5e7e479c6c5e8c903256adba0847b5c7872f52b7 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Sat, 7 Nov 2015 10:56:40 +0900 Subject: [PATCH] Workaround HTTP upgrade with HEAD request By default, we check the length of response body matches content-length. For HEAD request, this is not necessarily true, so we sniff request method, and if it is HEAD, make sure that response body length is 0. But this does not work for HTTP Upgrade, since nghttp2_session_upgrade() has no parameter to tell the request method was HEAD. This commit disables this response body length validation for the stream upgraded by HTTP Upgrade. 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. --- lib/nghttp2_http.c | 3 ++- lib/nghttp2_session.c | 10 +++++++++ lib/nghttp2_stream.h | 12 ++++++----- tests/main.c | 2 ++ tests/nghttp2_session_test.c | 42 ++++++++++++++++++++++++++++++++++++ tests/nghttp2_session_test.h | 1 + 6 files changed, 64 insertions(+), 6 deletions(-) diff --git a/lib/nghttp2_http.c b/lib/nghttp2_http.c index 5dfd44fc..2abcd50f 100644 --- a/lib/nghttp2_http.c +++ b/lib/nghttp2_http.c @@ -381,7 +381,8 @@ int nghttp2_http_on_response_headers(nghttp2_stream *stream) { if (!expect_response_body(stream)) { stream->content_length = 0; - } else if (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT) { + } else if (stream->http_flags & (NGHTTP2_HTTP_FLAG_METH_CONNECT | + NGHTTP2_HTTP_FLAG_METH_UPGRADE_WORKAROUND)) { stream->content_length = -1; } diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 4b5e6a22..07ba9608 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -6559,6 +6559,16 @@ int nghttp2_session_upgrade(nghttp2_session *session, nghttp2_stream_shutdown(stream, NGHTTP2_SHUT_WR); session->next_stream_id += 2; } + /* 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. */ + stream->http_flags |= NGHTTP2_HTTP_FLAG_METH_UPGRADE_WORKAROUND; return 0; } diff --git a/lib/nghttp2_stream.h b/lib/nghttp2_stream.h index c8aca99a..f73febbb 100644 --- a/lib/nghttp2_stream.h +++ b/lib/nghttp2_stream.h @@ -116,19 +116,21 @@ typedef enum { NGHTTP2_HTTP_FLAG_METH_CONNECT = 1 << 7, NGHTTP2_HTTP_FLAG_METH_HEAD = 1 << 8, NGHTTP2_HTTP_FLAG_METH_OPTIONS = 1 << 9, + NGHTTP2_HTTP_FLAG_METH_UPGRADE_WORKAROUND = 1 << 10, NGHTTP2_HTTP_FLAG_METH_ALL = NGHTTP2_HTTP_FLAG_METH_CONNECT | NGHTTP2_HTTP_FLAG_METH_HEAD | - NGHTTP2_HTTP_FLAG_METH_OPTIONS, + NGHTTP2_HTTP_FLAG_METH_OPTIONS | + NGHTTP2_HTTP_FLAG_METH_UPGRADE_WORKAROUND, /* :path category */ /* path starts with "/" */ - NGHTTP2_HTTP_FLAG_PATH_REGULAR = 1 << 10, + NGHTTP2_HTTP_FLAG_PATH_REGULAR = 1 << 11, /* path "*" */ - NGHTTP2_HTTP_FLAG_PATH_ASTERISK = 1 << 11, + NGHTTP2_HTTP_FLAG_PATH_ASTERISK = 1 << 12, /* scheme */ /* "http" or "https" scheme */ - NGHTTP2_HTTP_FLAG_SCHEME_HTTP = 1 << 12, + NGHTTP2_HTTP_FLAG_SCHEME_HTTP = 1 << 13, /* set if final response is expected */ - NGHTTP2_HTTP_FLAG_EXPECT_FINAL_RESPONSE = 1 << 13 + NGHTTP2_HTTP_FLAG_EXPECT_FINAL_RESPONSE = 1 << 14 } nghttp2_http_flag; struct nghttp2_stream { diff --git a/tests/main.c b/tests/main.c index 64710066..2887d071 100644 --- a/tests/main.c +++ b/tests/main.c @@ -298,6 +298,8 @@ int main(int argc _U_, char *argv[] _U_) { test_nghttp2_http_record_request_method) || !CU_add_test(pSuite, "http_push_promise", test_nghttp2_http_push_promise) || + !CU_add_test(pSuite, "http_head_method_upgrade_workaround", + test_nghttp2_http_head_method_upgrade_workaround) || !CU_add_test(pSuite, "frame_pack_headers", test_nghttp2_frame_pack_headers) || !CU_add_test(pSuite, "frame_pack_headers_frame_too_large", diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index b3441f5c..463f2e10 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -9246,3 +9246,45 @@ void test_nghttp2_http_push_promise(void) { nghttp2_session_del(session); nghttp2_bufs_free(&bufs); } + +void test_nghttp2_http_head_method_upgrade_workaround(void) { + nghttp2_session *session; + nghttp2_session_callbacks callbacks; + const nghttp2_nv resnv[] = {MAKE_NV(":status", "200"), + MAKE_NV("content-length", "1000000007")}; + nghttp2_bufs bufs; + nghttp2_hd_deflater deflater; + nghttp2_mem *mem; + ssize_t rv; + nghttp2_stream *stream; + + mem = nghttp2_mem_default(); + frame_pack_bufs_init(&bufs); + + memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); + callbacks.send_callback = null_send_callback; + + nghttp2_session_client_new(&session, &callbacks, NULL); + + nghttp2_hd_deflate_init(&deflater, mem); + + nghttp2_session_upgrade(session, NULL, 0, NULL); + + rv = pack_headers(&bufs, &deflater, 1, NGHTTP2_FLAG_END_HEADERS, resnv, + ARRLEN(resnv), mem); + + CU_ASSERT(0 == rv); + + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, + nghttp2_buf_len(&bufs.head->buf)); + + CU_ASSERT((ssize_t)nghttp2_buf_len(&bufs.head->buf) == rv); + + stream = nghttp2_session_get_stream(session, 1); + + CU_ASSERT(-1 == stream->content_length); + + nghttp2_hd_deflate_free(&deflater); + nghttp2_session_del(session); + nghttp2_bufs_free(&bufs); +} diff --git a/tests/nghttp2_session_test.h b/tests/nghttp2_session_test.h index ce08b5f9..979dbc46 100644 --- a/tests/nghttp2_session_test.h +++ b/tests/nghttp2_session_test.h @@ -141,5 +141,6 @@ void test_nghttp2_http_ignore_regular_header(void); void test_nghttp2_http_ignore_content_length(void); void test_nghttp2_http_record_request_method(void); void test_nghttp2_http_push_promise(void); +void test_nghttp2_http_head_method_upgrade_workaround(void); #endif /* NGHTTP2_SESSION_TEST_H */