pr12562-nghttp2-segfault-fix.patch 4.5 KB

123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120
  1. From 35380273b9311cf0741e386284310fa7ca4d005e Mon Sep 17 00:00:00 2001
  2. From: Stefan Eissing <stefan@eissing.org>
  3. Date: Tue, 19 Dec 2023 12:57:40 +0100
  4. Subject: [PATCH] http2: improved on_stream_close/data_done handling
  5. - there seems to be a code path that cleans up easy handles without
  6. triggering DONE or DETACH events to the connection filters. This
  7. would explain wh nghttp2 still holds stream user data
  8. - add GOOD check to easy handle used in on_close_callback to
  9. prevent crashes, ASSERTs in debug builds.
  10. - NULL the stream user data early before submitting RST
  11. - add checks in on_stream_close() to identify UNGOOD easy handles
  12. Reported-by: Hans-Christian Egtvedt
  13. Fixes #10936
  14. Closes #12562
  15. ---
  16. lib/http2.c | 50 ++++++++++++++++++++++++------------
  17. tests/http/test_07_upload.py | 17 ++++++++++++
  18. tests/http/testenv/curl.py | 2 +-
  19. 3 files changed, 51 insertions(+), 18 deletions(-)
  20. diff --git a/lib/http2.c b/lib/http2.c
  21. index 59903cfa72d250..dcc24ea102302c 100644
  22. --- a/lib/http2.c
  23. +++ b/lib/http2.c
  24. @@ -283,13 +283,20 @@ static void http2_data_done(struct Curl_cfilter *cf,
  25. return;
  26. if(ctx->h2) {
  27. + bool flush_egress = FALSE;
  28. + /* returns error if stream not known, which is fine here */
  29. + (void)nghttp2_session_set_stream_user_data(ctx->h2, stream->id, NULL);
  30. +
  31. if(!stream->closed && stream->id > 0) {
  32. /* RST_STREAM */
  33. CURL_TRC_CF(data, cf, "[%d] premature DATA_DONE, RST stream",
  34. stream->id);
  35. - if(!nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE,
  36. - stream->id, NGHTTP2_STREAM_CLOSED))
  37. - (void)nghttp2_session_send(ctx->h2);
  38. + stream->closed = TRUE;
  39. + stream->reset = TRUE;
  40. + stream->send_closed = TRUE;
  41. + nghttp2_submit_rst_stream(ctx->h2, NGHTTP2_FLAG_NONE,
  42. + stream->id, NGHTTP2_STREAM_CLOSED);
  43. + flush_egress = TRUE;
  44. }
  45. if(!Curl_bufq_is_empty(&stream->recvbuf)) {
  46. /* Anything in the recvbuf is still being counted
  47. @@ -299,19 +306,11 @@ static void http2_data_done(struct Curl_cfilter *cf,
  48. nghttp2_session_consume(ctx->h2, stream->id,
  49. Curl_bufq_len(&stream->recvbuf));
  50. /* give WINDOW_UPATE a chance to be sent, but ignore any error */
  51. - (void)h2_progress_egress(cf, data);
  52. + flush_egress = TRUE;
  53. }
  54. - /* -1 means unassigned and 0 means cleared */
  55. - if(nghttp2_session_get_stream_user_data(ctx->h2, stream->id)) {
  56. - int rv = nghttp2_session_set_stream_user_data(ctx->h2,
  57. - stream->id, 0);
  58. - if(rv) {
  59. - infof(data, "http/2: failed to clear user_data for stream %u",
  60. - stream->id);
  61. - DEBUGASSERT(0);
  62. - }
  63. - }
  64. + if(flush_egress)
  65. + nghttp2_session_send(ctx->h2);
  66. }
  67. Curl_bufq_free(&stream->sendbuf);
  68. @@ -1316,26 +1315,43 @@ static int on_stream_close(nghttp2_session *session, int32_t stream_id,
  69. uint32_t error_code, void *userp)
  70. {
  71. struct Curl_cfilter *cf = userp;
  72. - struct Curl_easy *data_s;
  73. + struct Curl_easy *data_s, *call_data = CF_DATA_CURRENT(cf);
  74. struct stream_ctx *stream;
  75. int rv;
  76. (void)session;
  77. + DEBUGASSERT(call_data);
  78. /* get the stream from the hash based on Stream ID, stream ID zero is for
  79. connection-oriented stuff */
  80. data_s = stream_id?
  81. nghttp2_session_get_stream_user_data(session, stream_id) : NULL;
  82. if(!data_s) {
  83. + CURL_TRC_CF(call_data, cf,
  84. + "[%d] on_stream_close, no easy set on stream", stream_id);
  85. return 0;
  86. }
  87. + if(!GOOD_EASY_HANDLE(data_s)) {
  88. + /* nghttp2 still has an easy registered for the stream which has
  89. + * been freed be libcurl. This points to a code path that does not
  90. + * trigger DONE or DETACH events as it must. */
  91. + CURL_TRC_CF(call_data, cf,
  92. + "[%d] on_stream_close, not a GOOD easy on stream", stream_id);
  93. + (void)nghttp2_session_set_stream_user_data(session, stream_id, 0);
  94. + return NGHTTP2_ERR_CALLBACK_FAILURE;
  95. + }
  96. stream = H2_STREAM_CTX(data_s);
  97. - if(!stream)
  98. + if(!stream) {
  99. + CURL_TRC_CF(data_s, cf,
  100. + "[%d] on_stream_close, GOOD easy but no stream", stream_id);
  101. return NGHTTP2_ERR_CALLBACK_FAILURE;
  102. + }
  103. stream->closed = TRUE;
  104. stream->error = error_code;
  105. - if(stream->error)
  106. + if(stream->error) {
  107. stream->reset = TRUE;
  108. + stream->send_closed = TRUE;
  109. + }
  110. if(stream->error)
  111. CURL_TRC_CF(data_s, cf, "[%d] RESET: %s (err %d)",