From 29f33b3400431e23afc85d6ec23427864104d730 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Mon, 26 Jun 2023 09:03:47 +0200 Subject: [PATCH] http2: fix crash in handling stream weights - Delay the priority handling until the stream has been opened. - Add test2404 to reproduce and verify. Weights may change "on the run", which is why there are checks in general egress handling. These must not trigger when the stream has not been opened yet. Reported-by: jbgoog@users.noreply.github.com Fixes https://github.com/curl/curl/issues/11379 Closes https://github.com/curl/curl/pull/11384 --- lib/http2.c | 7 +- tests/data/Makefile.inc | 2 +- tests/data/test2404 | 109 ++++++++++++++++++++++++++++ tests/libtest/Makefile.inc | 5 +- tests/libtest/lib2404.c | 144 +++++++++++++++++++++++++++++++++++++ 5 files changed, 262 insertions(+), 5 deletions(-) create mode 100644 tests/data/test2404 create mode 100644 tests/libtest/lib2404.c diff --git a/lib/http2.c b/lib/http2.c index 52ae8ce3df..c6275459a9 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -1686,9 +1686,10 @@ static CURLcode h2_progress_egress(struct Curl_cfilter *cf, struct stream_ctx *stream = H2_STREAM_CTX(data); int rv = 0; - if((sweight_wanted(data) != sweight_in_effect(data)) || - (data->set.priority.exclusive != data->state.priority.exclusive) || - (data->set.priority.parent != data->state.priority.parent) ) { + if(stream && stream->id > 0 && + ((sweight_wanted(data) != sweight_in_effect(data)) || + (data->set.priority.exclusive != data->state.priority.exclusive) || + (data->set.priority.parent != data->state.priority.parent)) ) { /* send new weight and/or dependency */ nghttp2_priority_spec pri_spec; diff --git a/tests/data/Makefile.inc b/tests/data/Makefile.inc index 2b003d950b..ca5faf9b58 100644 --- a/tests/data/Makefile.inc +++ b/tests/data/Makefile.inc @@ -247,7 +247,7 @@ test2200 test2201 test2202 test2203 test2204 test2205 \ \ test2300 test2301 test2302 test2303 test2304 test2305 test2306 \ \ -test2400 test2401 test2402 test2403 \ +test2400 test2401 test2402 test2403 test2404 \ \ test2500 test2501 test2502 test2503 \ \ diff --git a/tests/data/test2404 b/tests/data/test2404 new file mode 100644 index 0000000000..0e22bfe5f5 --- /dev/null +++ b/tests/data/test2404 @@ -0,0 +1,109 @@ + + + +HTTP +HTTP/2 +multi +verbose logs + + + +# Server-side + + +HTTP/1.1 200 OK +Date: Tue, 09 Nov 2010 14:49:00 GMT +Server: server.example.com +Content-Length: 47 + +file contents should appear once for each file + + +HTTP/1.1 200 OK +Date: Tue, 09 Nov 2010 14:49:00 GMT +Server: server.example.com +Content-Length: 47 + +file contents should appear once for each file + + +HTTP/1.1 200 OK +Date: Tue, 09 Nov 2010 14:49:00 GMT +Server: server.example.com +Content-Length: 47 + +file contents should appear once for each file + + +HTTP/1.1 200 OK +Date: Tue, 09 Nov 2010 14:49:00 GMT +Server: server.example.com +Content-Length: 47 + +file contents should appear once for each file + + + +# Client-side + + +h2c +SSL + + +http +http/2 + + +lib%TESTNUMBER + + +HTTP/2 using STREAM_WEIGHTs + + +https://%HOSTIP:%HTTP2TLSPORT/path/%TESTNUMBER %HOSTIP %HTTP2TLSPORT + + + +# Verify data after the test has been "shot" + + +GET /path/%TESTNUMBER0001 HTTP/1.1 +Host: %HOSTIP:%HTTPPORT +Accept: */* +X-Forwarded-Proto: https +Via: 2 nghttpx + +GET /path/%TESTNUMBER0002 HTTP/1.1 +Host: %HOSTIP:%HTTPPORT +Accept: */* +X-Forwarded-Proto: https +Via: 2 nghttpx + +GET /path/%TESTNUMBER0003 HTTP/1.1 +Host: %HOSTIP:%HTTPPORT +Accept: */* +X-Forwarded-Proto: https +Via: 2 nghttpx + +GET /path/%TESTNUMBER0004 HTTP/1.1 +Host: %HOSTIP:%HTTPPORT +Accept: */* +X-Forwarded-Proto: https +Via: 2 nghttpx + + + +^Host:.* + + +* Connection #0 to host localhost left intact +* Connection #0 to host localhost left intact +* Connection #0 to host localhost left intact +* Connection #0 to host localhost left intact + + +$_ = '' if (($_ !~ /left intact/) && ($_ !~ /Closing connection/)) + + + diff --git a/tests/libtest/Makefile.inc b/tests/libtest/Makefile.inc index c3b2a9957c..648536eb0a 100644 --- a/tests/libtest/Makefile.inc +++ b/tests/libtest/Makefile.inc @@ -72,7 +72,7 @@ noinst_PROGRAMS = chkhostname libauthretry libntlmconnect libprereq \ lib1960 \ lib1970 lib1971 lib1972 lib1973 lib1974 lib1975 \ lib2301 lib2302 lib2304 lib2305 lib2306 \ - lib2402 \ + lib2402 lib2404 \ lib2502 \ lib3010 lib3025 lib3026 lib3027 \ lib3100 lib3101 @@ -660,6 +660,9 @@ lib2306_LDADD = $(TESTUTIL_LIBS) lib2402_SOURCES = lib2402.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) lib2402_LDADD = $(TESTUTIL_LIBS) +lib2404_SOURCES = lib2404.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) +lib2404_LDADD = $(TESTUTIL_LIBS) + lib2502_SOURCES = lib2502.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS) lib2502_LDADD = $(TESTUTIL_LIBS) diff --git a/tests/libtest/lib2404.c b/tests/libtest/lib2404.c new file mode 100644 index 0000000000..1a282ffe2a --- /dev/null +++ b/tests/libtest/lib2404.c @@ -0,0 +1,144 @@ +/*************************************************************************** + * _ _ ____ _ + * Project ___| | | | _ \| | + * / __| | | | |_) | | + * | (__| |_| | _ <| |___ + * \___|\___/|_| \_\_____| + * + * Copyright (C) Linus Nielsen Feltzing + * + * This software is licensed as described in the file COPYING, which + * you should have received as part of this distribution. The terms + * are also available at https://curl.se/docs/copyright.html. + * + * You may opt to use, copy, modify, merge, publish, distribute and/or sell + * copies of the Software, and permit persons to whom the Software is + * furnished to do so, under the terms of the COPYING file. + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY + * KIND, either express or implied. + * + * SPDX-License-Identifier: curl + * + ***************************************************************************/ +#include "test.h" + +#include "testutil.h" +#include "warnless.h" +#include "memdebug.h" + +#define TEST_HANG_TIMEOUT 60 * 1000 + +#define NUM_HANDLES 4 + +int test(char *URL) +{ + int res = 0; + CURL *curl[NUM_HANDLES] = {0}; + int running; + CURLM *m = NULL; + int i; + char target_url[256]; + char dnsentry[256]; + struct curl_slist *slist = NULL; + char *port = libtest_arg3; + char *address = libtest_arg2; + + (void)URL; + + msnprintf(dnsentry, sizeof(dnsentry), "localhost:%s:%s", + port, address); + printf("%s\n", dnsentry); + slist = curl_slist_append(slist, dnsentry); + if(!slist) { + fprintf(stderr, "curl_slist_append() failed\n"); + goto test_cleanup; + } + + start_test_timing(); + + global_init(CURL_GLOBAL_ALL); + + multi_init(m); + + multi_setopt(m, CURLMOPT_MAXCONNECTS, 1L); + + /* get NUM_HANDLES easy handles */ + for(i = 0; i < NUM_HANDLES; i++) { + /* get an easy handle */ + easy_init(curl[i]); + /* specify target */ + msnprintf(target_url, sizeof(target_url), + "https://localhost:%s/path/2404%04i", + port, i + 1); + target_url[sizeof(target_url) - 1] = '\0'; + easy_setopt(curl[i], CURLOPT_URL, target_url); + /* go http2 */ + easy_setopt(curl[i], CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0); + /* no peer verify */ + easy_setopt(curl[i], CURLOPT_SSL_VERIFYPEER, 0L); + easy_setopt(curl[i], CURLOPT_SSL_VERIFYHOST, 0L); + /* wait for first connection established to see if we can share it */ + easy_setopt(curl[i], CURLOPT_PIPEWAIT, 1L); + /* go verbose */ + easy_setopt(curl[i], CURLOPT_VERBOSE, 1L); + /* include headers */ + easy_setopt(curl[i], CURLOPT_HEADER, 1L); + + easy_setopt(curl[i], CURLOPT_RESOLVE, slist); + + easy_setopt(curl[i], CURLOPT_STREAM_WEIGHT, (long)128 + i); + } + + fprintf(stderr, "Start at URL 0\n"); + + for(i = 0; i < NUM_HANDLES; i++) { + /* add handle to multi */ + multi_add_handle(m, curl[i]); + + for(;;) { + struct timeval interval; + fd_set rd, wr, exc; + int maxfd = -99; + + interval.tv_sec = 1; + interval.tv_usec = 0; + + multi_perform(m, &running); + + abort_on_test_timeout(); + + if(!running) + break; /* done */ + + FD_ZERO(&rd); + FD_ZERO(&wr); + FD_ZERO(&exc); + + multi_fdset(m, &rd, &wr, &exc, &maxfd); + + /* At this point, maxfd is guaranteed to be greater or equal than -1. */ + + select_test(maxfd + 1, &rd, &wr, &exc, &interval); + + abort_on_test_timeout(); + } + wait_ms(1); /* to ensure different end times */ + } + +test_cleanup: + + /* proper cleanup sequence - type PB */ + + for(i = 0; i < NUM_HANDLES; i++) { + curl_multi_remove_handle(m, curl[i]); + curl_easy_cleanup(curl[i]); + } + + curl_slist_free_all(slist); + + curl_multi_cleanup(m); + curl_global_cleanup(); + + return res; +} -- 2.47.2