Fix leaked QUIC blocking-section refs on SSL_poll abort path

[johnbuckman]

TL;DR

poll_translate() in ssl/rio/poll_immediate.c leaks one reactor blocking-section reference per already-ready QUIC poll item, on the ordinary SSL_poll() return path. The leaked references leave QUIC_REACTOR.cur_blocking_waiters permanently above zero, so rtor_notify_other_threads() eventually blocks forever and the whole QUIC reactor deadlocks — an SSL_poll() call that was given a finite timeout ends up parked in an untimed condition-variable wait. It reproduces in seconds under concurrent QUIC/HTTP-3 load and never for a single sequential poller. The fix is one statement: release the references on the abort path exactly as the existing failure path already does.

You can confirm this by reading ssl/rio/poll_immediate.c alone (next section) — no reproducer required, and no need to take the patch on faith. I've also included a runtime backtrace, a reproduction recipe, a scope note, and a suggested regression test. The fix was independently reproduced and confirmed on master by @gustafn (NaviServer maintainer).

The defect, by inspection

Two functions in the same file.

(1) Each translated item takes one blocking-section reference (poll_translate_ssl_quic(), ~line 130):

/* Tell QUIC domain we need to receive notifications. */
ossl_quic_enter_blocking_section(ssl, wctx);
...
if (revents != 0) {
    ossl_quic_leave_blocking_section(ssl, wctx);   /* releases THIS item's ref */
    *abort_blocking = 1;                            /* ...and asks to abort   */
    return 1;
}

On the already-ready outcome it releases its own reference and sets *abort_blocking. It does not touch the references taken for the items translated before it.

(2) poll_translate() handles its two exits asymmetrically. The failure path unwinds correctly:

out:
    if (!ok)
        postpoll_translation_cleanup(items, i, stride, wctx);   /* releases items 0..i-1 */

postpoll_translation_cleanup() walks items 0..i-1 calling ossl_quic_leave_blocking_section() on each. But the abort path (pre-patch) returns without it:

    if (*abort_blocking)
        return 1;              /* releases nothing for items 0..i-1 */

So on abort, items 0..i-1 each keep a blocking-section reference forever. (Item i already released its own in step 1 — which is exactly why the correct cleanup bound is i, identical to the failure path.)

Why this is the common case, not a corner case: *abort_blocking is set whenever any polled QUIC object is found already readable between the initial readout and arming the notifier. On a busy pollset that is the normal result of a poll, not an exception — so the leak accrues on essentially every poll that has ≥2 items and any readiness.

Why the leak deadlocks the reactor

ossl_quic_enter_/leave_blocking_section() balance the reactor's blocking-waiter accounting. In ssl/quic/quic_reactor.c, signalled_notifier is cleared only once cur_blocking_waiters returns to zero (the last waiter to leave clears it — see the block-until path, ~lines 608–628, and the design comment at ~544–576). A thread trying to notify does:

/* rtor_notify_other_threads(), ~line 488 */
while (rtor->signalled_notifier)
    ossl_crypto_condvar_wait(rtor->notifier_cv, rtor->mutex);

Leaked enters keep cur_blocking_waiters permanently > 0, so signalled_notifier is never cleared, so this loop never returns. The QUIC reactor is dead for the life of the process.

The only guard that would have caught the imbalance is an assert() (ossl_quic_reactor_wait_ctx_cleanup() asserts the count is zero), and release builds compile with -DNDEBUG — so it is silent in production and surfaces only as a mysterious hang.

How we found it (train of thought)

1. On a server driving OpenSSL QUIC through SSL_poll(), HTTP/3 would wedge permanently under load while HTTP/1.1 and HTTP/2 in the same process kept answering 200 and the process never crashed. One client doing requests one-at-a-time never triggered it; ~6 concurrent h3 connections wedged it in under 2 seconds, every time.
2. Since only h3 died, and h3 runs on a single event-loop thread, the fault was that thread. gdb -p <pid> -batch -ex "thread apply all bt" on the wedged process:

   #5  __pthread_cond_wait_common (mutex=..., abstime=0x0)   <- untimed, indefinite
   #6  ___pthread_cond_wait ()
   #7  ossl_quic_reactor_tick ()      from libssl.so.4
   #8  ossl_quic_conn_poll_events ()  from libssl.so.4
   #9  SSL_poll ()                    from libssl.so.4        <- we passed a finite 3s timeout
   #10 <application QUIC event loop>
   

   An SSL_poll() given a finite timeout has no business parking in an untimed condvar wait. That located the bug inside libssl, not the caller.
3. Ruled out the two most likely caller mistakes first:
   • "We left the accepted connection in blocking mode." Logged the inherited value: blocking_mode=0 — non-blocking is inherited as documented. Not it.
   • "Our extra SSL_handle_events() preprocessing is at fault." OpenSSL's own demos/quic/poll-server/ never calls it. Removing ours just moved the same hang into SSL_poll() itself, via the same ossl_quic_reactor_tick path — two entry points, one broken reactor.
4. Reproduced identically on master (4.1.0-dev), so it was not already fixed.
5. Read the reactor: the wait is rtor_notify_other_threads() spinning on signalled_notifier, which clears only when cur_blocking_waiters reaches zero. So something increments that count without a matching decrement. The enter/leave blocking-section calls are that count — and poll_translate()'s abort path is the one place that returns without the paired leave. That is the leak; the fix is to mirror the failure path.

The fix

if (*abort_blocking) {
    postpoll_translation_cleanup(items, i, stride, wctx);
    return 1;
}

Fixed here rather than in wait_ctx_cleanup() because the enter/leave wrappers take the reactor lock while cleanup runs unlocked — moving it there would race.

Reproducing it

A. By inspection — the two functions above. No build required; this is the most direct check.

B. End-to-end, with a real browser. A stdlib-only Python script drives headless Chrome forced onto HTTP/3:

chrome --headless=new --enable-quic \
       --origin-to-force-quic-on=HOST:443 \
       --user-data-dir=<tmp> --remote-debugging-port=9333

then over the DevTools protocol it confirms the navigation actually used h3 (performance.getEntries()[0].nextHopProtocol === "h3") and fetch()es a small file plus a multi-MB file, concurrently, printing the instant h3 stops answering while a plain-HTTPS canary on the same server stays 200 (which distinguishes "h3 reactor wedged" from "server down"). Under ~6 concurrent streams it wedges in <2s. We also have an aioquic-based harness that does the same without a browser. Happy to share both scripts, or the exact server build, on request.

Scope note (re "other servers"): the client is server-agnostic, but this defect is in OpenSSL's QUIC SSL_poll() blocking path, so it only reproduces against a server that drives QUIC through SSL_poll() in a blocking event loop — i.e. something built on OpenSSL's native QUIC, of which demos/quic/poll-server/ is the reference. Servers on other QUIC stacks (ngtcp2, quiche, msquic, …) do not exercise this code and will not reproduce it. We hit it on the NaviServer OpenSSL-QUIC driver; @gustafn independently reproduced and confirmed the fix on master (with the patch, 20 alternating 5 MiB and range requests then completed in one process).

C. Suggested regression test. Poll two or more QUIC stream items in blocking mode with a finite timeout where at least one is already readable, then assert the reactor's blocking-waiter count is back to zero after SSL_poll() returns (pre-patch it is left at N−1, where N is the number of items before the ready one). Glad to help turn this into a proper test/ case if you'd like it in the PR.

Backport

The same code is on the 4.0 branch (ssl/rio/poll_immediate.c) — a backport is desirable.

[Sashan]

this seems to be a duplicate to PR submitted by @mattcaswell
#31743

I think we should close this one and get reviews done on Matt's PR which comes witht test too.

thanks for understanding.