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 59903cfa72d2501dd43392566183c567f379ea89..dcc24ea102302ce7497c2ef102862a544db66b85 100644 (file)
--- 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 018a56c6e50d4ee68a2d7fa7ad52467cfefd3800..2115b577134d99a3accfe6c690265ffc5a273008 100644 (file)
--- 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 bc5b4881c7320d817bf86ef386b34f6a4e2f8e24..f8aaac46a55451f0666cebdf307cee7ff29176d0 100644 (file)
--- 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)}'