1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
|
From 35380273b9311cf0741e386284310fa7ca4d005e Mon Sep 17 00:00:00 2001
From: Stefan Eissing <stefan@eissing.org>
Date: Tue, 19 Dec 2023 12:57:40 +0100
Subject: [PATCH] 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
---
lib/http2.c | 50 ++++++++++++++++++++++++------------
tests/http/test_07_upload.py | 17 ++++++++++++
tests/http/testenv/curl.py | 2 +-
3 files changed, 51 insertions(+), 18 deletions(-)
diff --git a/lib/http2.c b/lib/http2.c
index 59903cfa72d250..dcc24ea102302c 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)",
|
