BUG/MEDIUM: mux-h2: don't return a freed tasklet from h2_deferred_shut()

When h2_deferred_shut() completes the deferred shutdown of an orphaned
stream (the stream connector already left), it destroys the stream with
h2s_destroy(), which also frees the stream's shut_tl tasklet, which is
precisely the one being run. However the function then returns <t>,
which is that freed tasklet, unless the connection was also released.
The scheduler then clears TASK_RUNNING in the freed tasklet's state,
which is a use-after-free write. In default builds the area is generally
still in the thread's pool cache, so the bit being cleared is harmless,
but it may corrupt memory if the area was reused, and it crashes with
-dMuaf.

The situation happens when an RST_STREAM (or an ES) could not be sent
when the stream was shut because the mux buffer was full, and the stream
detaches before the deferred shutdown is performed, e.g. an H2 client
not reading its responses fast enough while a POST request is denied.
It was reproduced with a debug patch simulating a full mux buffer for
the first RST_STREAM, with a POST request blocked by "http-request deny"
and -dMuaf: the worker dies with SIGSEGV in run_tasks_from_lists(), and
works fine with the fix.

The fix simply consists in always returning NULL once the stream was
destroyed. This was introduced in 2.4 by commit 741631414 ("CLEANUP: task:
make sure tasklet handlers always indicate their statuses"), so the fix
must be backported to all supported versions. Note that mux-fcgi's
fcgi_deferred_shut() has the same issue.

diff --git a/src/mux_h2.c b/src/mux_h2.c
index d181454d24a955475125c390c575fcbed76aa34d..7455b3238f44b5c6989740030d4c6b1b59ec23d2 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -6076,11 +6076,11 @@ struct task *h2_deferred_shut(struct task *t, void *ctx, unsigned int state)
                h2_remove_from_list(h2s);
 
                if (!h2s_sc(h2s)) {
+                       /* note: this frees the tasklet as well */
                        h2s_destroy(h2s);
-                       if (h2c_is_dead(h2c)) {
+                       t = NULL;
+                       if (h2c_is_dead(h2c))
                                h2_release(h2c);
-                               t = NULL;
-                       }
                }
        }
  end: