From 5c59f91427c6e14036070d4e1426360393458b25 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Thu, 18 Apr 2024 23:24:34 +0200 Subject: [PATCH] http2 + ngtcp2: pass CURLcode errors from callbacks - errors returned by Curl_xfer_write_resp() and the header variant are not errors in the protocol. The result needs to be returned on the next recv() from the protocol filter. - make xfer write errors for response data cause the stream to be cancelled - added pytest test_02_14 and test_02_15 to verify that also for parallel processing Reported-by: Laramie Leavitt Fixes #13411 Closes #13424 --- lib/http2.c | 64 +++++++++++++++++++++-------- lib/vquic/curl_ngtcp2.c | 73 +++++++++++++++++++++++----------- tests/http/test_02_download.py | 28 +++++++++++++ 3 files changed, 125 insertions(+), 40 deletions(-) diff --git a/lib/http2.c b/lib/http2.c index 1ee57a4320..50dd878bb0 100644 --- a/lib/http2.c +++ b/lib/http2.c @@ -193,6 +193,7 @@ struct h2_stream_ctx { int status_code; /* HTTP response status code */ uint32_t error; /* stream error code */ + CURLcode xfer_result; /* Result of writing out response */ uint32_t local_window_size; /* the local recv window size */ int32_t id; /* HTTP/2 protocol identifier for stream */ BIT(resp_hds_complete); /* we have a complete, final response */ @@ -975,6 +976,41 @@ fail: return rv; } +static void h2_xfer_write_resp_hd(struct Curl_cfilter *cf, + struct Curl_easy *data, + struct h2_stream_ctx *stream, + const char *buf, size_t blen, bool eos) +{ + + /* If we already encountered an error, skip further writes */ + if(!stream->xfer_result) { + stream->xfer_result = Curl_xfer_write_resp_hd(data, buf, blen, eos); + if(stream->xfer_result) + CURL_TRC_CF(data, cf, "[%d] error %d writing %zu bytes of headers", + stream->id, stream->xfer_result, blen); + } +} + +static void h2_xfer_write_resp(struct Curl_cfilter *cf, + struct Curl_easy *data, + struct h2_stream_ctx *stream, + const char *buf, size_t blen, bool eos) +{ + + /* If we already encountered an error, skip further writes */ + if(!stream->xfer_result) + stream->xfer_result = Curl_xfer_write_resp(data, buf, blen, eos); + /* If the transfer write is errored, we do not want any more data */ + if(stream->xfer_result) { + struct cf_h2_ctx *ctx = cf->ctx; + CURL_TRC_CF(data, cf, "[%d] error %d writing %zu bytes of data, " + "RST-ing stream", + stream->id, stream->xfer_result, blen); + nghttp2_submit_rst_stream(ctx->h2, 0, stream->id, + NGHTTP2_ERR_CALLBACK_FAILURE); + } +} + static CURLcode on_stream_frame(struct Curl_cfilter *cf, struct Curl_easy *data, const nghttp2_frame *frame) @@ -982,7 +1018,6 @@ static CURLcode on_stream_frame(struct Curl_cfilter *cf, struct cf_h2_ctx *ctx = cf->ctx; struct h2_stream_ctx *stream = H2_STREAM_CTX(ctx, data); int32_t stream_id = frame->hd.stream_id; - CURLcode result; int rv; if(!stream) { @@ -1030,9 +1065,7 @@ static CURLcode on_stream_frame(struct Curl_cfilter *cf, stream->status_code = -1; } - result = Curl_xfer_write_resp_hd(data, STRCONST("\r\n"), stream->closed); - if(result) - return result; + h2_xfer_write_resp_hd(cf, data, stream, STRCONST("\r\n"), stream->closed); if(stream->status_code / 100 != 1) { stream->resp_hds_complete = TRUE; @@ -1251,7 +1284,6 @@ static int on_data_chunk_recv(nghttp2_session *session, uint8_t flags, struct cf_h2_ctx *ctx = cf->ctx; struct h2_stream_ctx *stream; struct Curl_easy *data_s; - CURLcode result; (void)flags; DEBUGASSERT(stream_id); /* should never be a zero stream ID here */ @@ -1274,12 +1306,7 @@ static int on_data_chunk_recv(nghttp2_session *session, uint8_t flags, if(!stream) return NGHTTP2_ERR_CALLBACK_FAILURE; - result = Curl_xfer_write_resp(data_s, (char *)mem, len, FALSE); - if(result && result != CURLE_AGAIN) { - nghttp2_submit_rst_stream(ctx->h2, 0, stream->id, - NGHTTP2_ERR_CALLBACK_FAILURE); - return 0; - } + h2_xfer_write_resp(cf, data_s, stream, (char *)mem, len, FALSE); nghttp2_session_consume(ctx->h2, stream_id, len); stream->nrcvd_data += (curl_off_t)len; @@ -1500,8 +1527,8 @@ static int on_header(nghttp2_session *session, const nghttp2_frame *frame, if(!result) result = Curl_dyn_addn(&ctx->scratch, STRCONST(" \r\n")); if(!result) - result = Curl_xfer_write_resp_hd(data_s, Curl_dyn_ptr(&ctx->scratch), - Curl_dyn_len(&ctx->scratch), FALSE); + h2_xfer_write_resp_hd(cf, data_s, stream, Curl_dyn_ptr(&ctx->scratch), + Curl_dyn_len(&ctx->scratch), FALSE); if(result) return NGHTTP2_ERR_CALLBACK_FAILURE; /* if we receive data for another handle, wake that up */ @@ -1525,8 +1552,8 @@ static int on_header(nghttp2_session *session, const nghttp2_frame *frame, if(!result) result = Curl_dyn_addn(&ctx->scratch, STRCONST("\r\n")); if(!result) - result = Curl_xfer_write_resp_hd(data_s, Curl_dyn_ptr(&ctx->scratch), - Curl_dyn_len(&ctx->scratch), FALSE); + h2_xfer_write_resp_hd(cf, data_s, stream, Curl_dyn_ptr(&ctx->scratch), + Curl_dyn_len(&ctx->scratch), FALSE); if(result) return NGHTTP2_ERR_CALLBACK_FAILURE; /* if we receive data for another handle, wake that up */ @@ -1831,7 +1858,12 @@ static ssize_t stream_recv(struct Curl_cfilter *cf, struct Curl_easy *data, (void)buf; *err = CURLE_AGAIN; - if(stream->closed) { + if(stream->xfer_result) { + CURL_TRC_CF(data, cf, "[%d] xfer write failed", stream->id); + *err = stream->xfer_result; + nread = -1; + } + else if(stream->closed) { CURL_TRC_CF(data, cf, "[%d] returning CLOSE", stream->id); nread = http2_handle_stream_close(cf, data, stream, err); } diff --git a/lib/vquic/curl_ngtcp2.c b/lib/vquic/curl_ngtcp2.c index 50fe7af701..5171125f2f 100644 --- a/lib/vquic/curl_ngtcp2.c +++ b/lib/vquic/curl_ngtcp2.c @@ -155,6 +155,7 @@ struct h3_stream_ctx { curl_uint64_t error3; /* HTTP/3 stream error code */ curl_off_t upload_left; /* number of request bytes left to upload */ int status_code; /* HTTP status code */ + CURLcode xfer_result; /* result from xfer_resp_write(_hd) */ bool resp_hds_complete; /* we have a complete, final response */ bool closed; /* TRUE on stream close */ bool reset; /* TRUE on stream reset */ @@ -795,6 +796,41 @@ static int cb_h3_stream_close(nghttp3_conn *conn, int64_t sid, return 0; } +static void h3_xfer_write_resp_hd(struct Curl_cfilter *cf, + struct Curl_easy *data, + struct h3_stream_ctx *stream, + const char *buf, size_t blen, bool eos) +{ + + /* If we already encountered an error, skip further writes */ + if(!stream->xfer_result) { + stream->xfer_result = Curl_xfer_write_resp_hd(data, buf, blen, eos); + if(stream->xfer_result) + CURL_TRC_CF(data, cf, "[%"CURL_PRId64"] error %d writing %zu " + "bytes of headers", stream->id, stream->xfer_result, blen); + } +} + +static void h3_xfer_write_resp(struct Curl_cfilter *cf, + struct Curl_easy *data, + struct h3_stream_ctx *stream, + const char *buf, size_t blen, bool eos) +{ + + /* If we already encountered an error, skip further writes */ + if(!stream->xfer_result) + stream->xfer_result = Curl_xfer_write_resp(data, buf, blen, eos); + /* If the transfer write is errored, we do not want any more data */ + if(stream->xfer_result) { + struct cf_ngtcp2_ctx *ctx = cf->ctx; + CURL_TRC_CF(data, cf, "[%"CURL_PRId64"] error %d writing %zu bytes " + "of data, cancelling stream", + stream->id, stream->xfer_result, blen); + nghttp3_conn_close_stream(ctx->h3conn, stream->id, + NGHTTP3_H3_REQUEST_CANCELLED); + } +} + static int cb_h3_recv_data(nghttp3_conn *conn, int64_t stream3_id, const uint8_t *buf, size_t blen, void *user_data, void *stream_user_data) @@ -803,7 +839,6 @@ static int cb_h3_recv_data(nghttp3_conn *conn, int64_t stream3_id, struct cf_ngtcp2_ctx *ctx = cf->ctx; struct Curl_easy *data = stream_user_data; struct h3_stream_ctx *stream = H3_STREAM_CTX(ctx, data); - CURLcode result; (void)conn; (void)stream3_id; @@ -811,16 +846,7 @@ static int cb_h3_recv_data(nghttp3_conn *conn, int64_t stream3_id, if(!stream) return NGHTTP3_ERR_CALLBACK_FAILURE; - result = Curl_xfer_write_resp(data, (char *)buf, blen, FALSE); - if(result) { - CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] DATA len=%zu, ERROR %d", - stream->id, blen, result); - nghttp3_conn_close_stream(ctx->h3conn, stream->id, - NGHTTP3_H3_REQUEST_CANCELLED); - ngtcp2_conn_extend_max_stream_offset(ctx->qconn, stream->id, blen); - ngtcp2_conn_extend_max_offset(ctx->qconn, blen); - return 0; - } + h3_xfer_write_resp(cf, data, stream, (char *)buf, blen, FALSE); if(blen) { CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] ACK %zu bytes of DATA", stream->id, blen); @@ -855,7 +881,6 @@ static int cb_h3_end_headers(nghttp3_conn *conn, int64_t sid, struct Curl_easy *data = stream_user_data; curl_int64_t stream_id = (curl_int64_t)sid; struct h3_stream_ctx *stream = H3_STREAM_CTX(ctx, data); - CURLcode result = CURLE_OK; (void)conn; (void)stream_id; (void)fin; @@ -864,10 +889,7 @@ static int cb_h3_end_headers(nghttp3_conn *conn, int64_t sid, if(!stream) return 0; /* add a CRLF only if we've received some headers */ - result = Curl_xfer_write_resp_hd(data, STRCONST("\r\n"), stream->closed); - if(result) { - return -1; - } + h3_xfer_write_resp_hd(cf, data, stream, STRCONST("\r\n"), stream->closed); CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] end_headers, status=%d", stream_id, stream->status_code); @@ -915,8 +937,8 @@ static int cb_h3_recv_header(nghttp3_conn *conn, int64_t sid, if(!result) result = Curl_dyn_addn(&ctx->scratch, STRCONST(" \r\n")); if(!result) - result = Curl_xfer_write_resp_hd(data, Curl_dyn_ptr(&ctx->scratch), - Curl_dyn_len(&ctx->scratch), FALSE); + h3_xfer_write_resp_hd(cf, data, stream, Curl_dyn_ptr(&ctx->scratch), + Curl_dyn_len(&ctx->scratch), FALSE); CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] status: %s", stream_id, Curl_dyn_ptr(&ctx->scratch)); if(result) { @@ -939,11 +961,8 @@ static int cb_h3_recv_header(nghttp3_conn *conn, int64_t sid, if(!result) result = Curl_dyn_addn(&ctx->scratch, STRCONST("\r\n")); if(!result) - result = Curl_xfer_write_resp_hd(data, Curl_dyn_ptr(&ctx->scratch), - Curl_dyn_len(&ctx->scratch), FALSE); - if(result) { - return -1; - } + h3_xfer_write_resp_hd(cf, data, stream, Curl_dyn_ptr(&ctx->scratch), + Curl_dyn_len(&ctx->scratch), FALSE); } return 0; } @@ -1128,7 +1147,13 @@ static ssize_t cf_ngtcp2_recv(struct Curl_cfilter *cf, struct Curl_easy *data, goto out; } - if(stream->closed) { + if(stream->xfer_result) { + CURL_TRC_CF(data, cf, "[%" CURL_PRId64 "] xfer write failed", stream->id); + *err = stream->xfer_result; + nread = -1; + goto out; + } + else if(stream->closed) { nread = recv_closed_stream(cf, data, stream, err); goto out; } diff --git a/tests/http/test_02_download.py b/tests/http/test_02_download.py index 95a30e114b..74f2e1e20f 100644 --- a/tests/http/test_02_download.py +++ b/tests/http/test_02_download.py @@ -257,6 +257,34 @@ class TestDownload: ]) r.check_response(count=count, http_status=200) + @pytest.mark.parametrize("proto", ['h2', 'h3']) + def test_02_14_not_found(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") + count = 10 + urln = f'https://{env.authority_for(env.domain1, proto)}/not-found?[0-{count-1}]' + curl = CurlClient(env=env) + r = curl.http_download(urls=[urln], alpn_proto=proto, extra_args=[ + '--parallel' + ]) + r.check_stats(count=count, http_status=404, exitcode=0) + + @pytest.mark.parametrize("proto", ['h2', 'h3']) + def test_02_15_fail_not_found(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") + count = 10 + urln = f'https://{env.authority_for(env.domain1, proto)}/not-found?[0-{count-1}]' + curl = CurlClient(env=env) + r = curl.http_download(urls=[urln], alpn_proto=proto, extra_args=[ + '--fail' + ]) + r.check_stats(count=count, http_status=404, exitcode=22) + @pytest.mark.skipif(condition=Env().slow_network, reason="not suitable for slow network tests") @pytest.mark.skipif(condition=Env().ci_run, reason="not suitable for CI runs") def test_02_20_h2_small_frames(self, env: Env, httpd, repeat): -- 2.47.2