From: Stefan Eissing Date: Tue, 19 Dec 2023 11:57:40 +0000 (+0100) Subject: http2: improved on_stream_close/data_done handling X-Git-Tag: curl-8_6_0~178 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=35380273b9311cf0741e386284310fa7ca4d005e;p=thirdparty%2Fcurl.git http2: improved on_stream_close/data_done handling - there seems to be a code path that cleans up easy handles without triggering DONE or DETACH events to the connection filters. This would explain wh nghttp2 still holds stream user data - add GOOD check to easy handle used in on_close_callback to prevent crashes, ASSERTs in debug builds. - NULL the stream user data early before submitting RST - add checks in on_stream_close() to identify UNGOOD easy handles Reported-by: Hans-Christian Egtvedt Fixes #10936 Closes #12562 --- diff --git a/lib/http2.c b/lib/http2.c index 59903cfa72..dcc24ea102 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -283,13 +283,20 @@ static void http2_data_done(struct Curl_cfilter *cf, return; if(ctx->h2) { + bool flush_egress = FALSE; + /* returns error if stream not known, which is fine here */ + (void)nghttp2_session_set_stream_user_data(ctx->h2, stream->id, NULL); + if(!stream->closed && stream->id > 0) { /* RST_STREAM */ CURL_TRC_CF(data, cf, "[%d] premature DATA_DONE, RST stream", stream->id); - if(!nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, - stream->id, NGHTTP2_STREAM_CLOSED)) - (void)nghttp2_session_send(ctx->h2); + stream->closed = TRUE; + stream->reset = TRUE; + stream->send_closed = TRUE; + nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE, + stream->id, NGHTTP2_STREAM_CLOSED); + flush_egress = TRUE; } if(!Curl_bufq_is_empty(&stream->recvbuf)) { /* Anything in the recvbuf is still being counted @@ -299,19 +306,11 @@ static void http2_data_done(struct Curl_cfilter *cf, nghttp2_session_consume(ctx->h2, stream->id, Curl_bufq_len(&stream->recvbuf)); /* give WINDOW_UPATE a chance to be sent, but ignore any error */ - (void)h2_progress_egress(cf, data); + flush_egress = TRUE; } - /* -1 means unassigned and 0 means cleared */ - if(nghttp2_session_get_stream_user_data(ctx->h2, stream->id)) { - int rv = nghttp2_session_set_stream_user_data(ctx->h2, - stream->id, 0); - if(rv) { - infof(data, "http/2: failed to clear user_data for stream %u", - stream->id); - DEBUGASSERT(0); - } - } + if(flush_egress) + nghttp2_session_send(ctx->h2); } Curl_bufq_free(&stream->sendbuf); @@ -1316,26 +1315,43 @@ static int on_stream_close(nghttp2_session *session, int32_t stream_id, uint32_t error_code, void *userp) { struct Curl_cfilter *cf = userp; - struct Curl_easy *data_s; + struct Curl_easy *data_s, *call_data = CF_DATA_CURRENT(cf); struct stream_ctx *stream; int rv; (void)session; + DEBUGASSERT(call_data); /* get the stream from the hash based on Stream ID, stream ID zero is for connection-oriented stuff */ data_s = stream_id? nghttp2_session_get_stream_user_data(session, stream_id) : NULL; if(!data_s) { + CURL_TRC_CF(call_data, cf, + "[%d] on_stream_close, no easy set on stream", stream_id); return 0; } + if(!GOOD_EASY_HANDLE(data_s)) { + /* nghttp2 still has an easy registered for the stream which has + * been freed be libcurl. This points to a code path that does not + * trigger DONE or DETACH events as it must. */ + CURL_TRC_CF(call_data, cf, + "[%d] on_stream_close, not a GOOD easy on stream", stream_id); + (void)nghttp2_session_set_stream_user_data(session, stream_id, 0); + return NGHTTP2_ERR_CALLBACK_FAILURE; + } stream = H2_STREAM_CTX(data_s); - if(!stream) + if(!stream) { + CURL_TRC_CF(data_s, cf, + "[%d] on_stream_close, GOOD easy but no stream", stream_id); return NGHTTP2_ERR_CALLBACK_FAILURE; + } stream->closed = TRUE; stream->error = error_code; - if(stream->error) + if(stream->error) { stream->reset = TRUE; + stream->send_closed = TRUE; + } if(stream->error) CURL_TRC_CF(data_s, cf, "[%d] RESET: %s (err %d)", diff --git a/tests/http/test_07_upload.py b/tests/http/test_07_upload.py index 018a56c6e5..2115b57713 100644 --- a/tests/http/test_07_upload.py +++ b/tests/http/test_07_upload.py @@ -189,6 +189,23 @@ class TestUpload: r.check_response(count=count, http_status=200) self.check_download(count, fdata, curl) + # upload large data parallel to a URL that denies uploads + @pytest.mark.parametrize("proto", ['h2', 'h3']) + def test_07_22_upload_parallel_fail(self, env: Env, httpd, nghttpx, repeat, proto): + if proto == 'h3' and not env.have_h3(): + pytest.skip("h3 not supported") + if proto == 'h3' and env.curl_uses_lib('msh3'): + pytest.skip("msh3 stalls here") + fdata = os.path.join(env.gen_dir, 'data-10m') + count = 100 + curl = CurlClient(env=env) + url = f'https://{env.authority_for(env.domain1, proto)}'\ + f'/curltest/tweak?status=400&delay=5ms&chunks=1&body_error=reset&id=[0-{count-1}]' + r = curl.http_upload(urls=[url], data=f'@{fdata}', alpn_proto=proto, + extra_args=['--parallel']) + exp_exit = 92 if proto == 'h2' else 95 + r.check_stats(count=count, exitcode=exp_exit) + # PUT 100k @pytest.mark.parametrize("proto", ['http/1.1', 'h2', 'h3']) def test_07_30_put_100k(self, env: Env, httpd, nghttpx, repeat, proto): diff --git a/tests/http/testenv/curl.py b/tests/http/testenv/curl.py index bc5b4881c7..f8aaac46a5 100644 --- a/tests/http/testenv/curl.py +++ b/tests/http/testenv/curl.py @@ -247,7 +247,7 @@ class ExecResult: if exitcode is not None: for idx, x in enumerate(self.stats): if 'exitcode' in x: - assert x['exitcode'] == 0, \ + assert x['exitcode'] == exitcode, \ f'status #{idx} exitcode: expected {exitcode}, '\ f'got {x["exitcode"]}\n{self.dump_stat(x)}'