http2: assertion failure `onread->IsFunction()` when a session is destroyed from a 'stream' handler

[mcollina]

What steps will reproduce the bug?

node:http2 aborts on an internal assertion when an HTTP/2 session is destroyed from inside a 'stream' handler while the peer's DATA frames are still in the receive buffer:

node[...]: v8::MaybeLocal<v8::Value> node::StreamBase::CallJSOnreadMethod(ssize_t, v8::Local<v8::ArrayBuffer>, std::size_t, StreamBaseJSChecks) at ../src/stream_base.cc:473
Assertion failed: onread->IsFunction()

 1: node::Assert(node::AssertionInfo const&)
 2: node::StreamBase::CallJSOnreadMethod(...)
 3: node::http2::Http2StreamListener::OnStreamRead(long, uv_buf_t const&)
 4: node::http2::Http2Session::OnDataChunkReceived(nghttp2_session*, unsigned char, int, unsigned char const*, unsigned long, void*)
 5: nghttp2_session_mem_recv2
 6: node::http2::Http2Session::ConsumeHTTP2Data()
 7: node::http2::Http2Session::OnStreamRead(long, uv_buf_t const&)

handle.onread is assigned in exactly one place — Http2Stream[kInit] in lib/internal/http2/core.js — so nghttp2 is delivering a DATA chunk to a stream whose JS wrapper has already been torn down.

This is reachable from ordinary peer behaviour: the server simply destroys its own session from the 'stream' handler, which runs inside ConsumeHTTP2Data().

Minimal reproduction

No dependencies, no TLS. Aborts within a few hundred rounds, typically well under the 5s default window:

'use strict'

// Standalone reproduction — node:http2 only, no dependencies.
//
// An HTTP/2 server destroys its own session from inside the 'stream' handler,
// which nghttp2 dispatches from within Http2Session::ConsumeHTTP2Data. The
// client sends several concurrent POSTs whose HEADERS and DATA arrive in the
// same receive buffer, so nghttp2 keeps delivering DATA for the remaining
// streams after JS has torn the session down. Delivery then reaches a stream
// whose JS wrapper is gone:
//
//   node::StreamBase::CallJSOnreadMethod at ../src/stream_base.cc:473
//   Assertion failed: onread->IsFunction()
//     <- node::http2::Http2StreamListener::OnStreamRead
//     <- node::http2::Http2Session::OnDataChunkReceived
//     <- nghttp2_session_mem_recv2
//     <- node::http2::Http2Session::ConsumeHTTP2Data
//
// handle.onread is assigned in exactly one place, Http2Stream[kInit]
// (lib/internal/http2/core.js), so the C++ side is calling into a stream that
// was destroyed while the receive operation was still in flight.
//
// Usage:
//   node repro-h2-abort.js
//
// Exit status 134 (SIGABRT) means it reproduced; 0 means it survived the run.
// Reproduces over cleartext h2 (and equally over TLS), and does not depend on
// --expose-gc.

const http2 = require('node:http2')

const DURATION = Number(process.env.DURATION || 5000)
const STREAMS = Number(process.env.STREAMS || 8)
// Small enough that HEADERS and DATA coalesce into a single read.
const BODY = Buffer.alloc(Number(process.env.BODY || 2048), 'a')

const serverOptions = {
  settings: { maxConcurrentStreams: Number(process.env.MAX_CONCURRENT_STREAMS || 4) }
}

const server = http2.createServer(serverOptions)

server.on('session', (session) => session.on('error', () => {}))

// The whole trigger: tear the connection down from inside the stream handler,
// i.e. while nghttp2 is still walking the buffer that delivered this stream.
server.on('stream', (stream) => {
  stream.on('error', () => {})
  stream.session.destroy()
})

let rounds = 0

server.listen(0, '127.0.0.1', () => {
  const port = server.address().port
  const origin = `http://127.0.0.1:${port}`
  let stopped = false

  const round = () => {
    if (stopped) return
    rounds++

    const session = http2.connect(origin)
    session.on('error', () => {})
    session.on('close', () => setImmediate(round))

    session.on('connect', () => {
      for (let i = 0; i < STREAMS; i++) {
        const stream = session.request({ ':path': `/${i}`, ':method': 'POST' })
        stream.on('error', () => {})
        stream.resume()
        stream.end(BODY)
      }
    })
  }

  round()

  setTimeout(() => {
    stopped = true
    console.log(`survived ${rounds} rounds without aborting`)
    server.close()
    process.exit(0)
  }, DURATION)
})

$ node repro-h2-abort.js
Assertion failed: onread->IsFunction()
Aborted (core dumped)
$ echo $?
134

Does not depend on --expose-gc (4/4 with and without). Reproduces over TLS as well; the TLSWrap::ClearOut frame in the original stack was incidental.

Affected versions

build	result (3 runs each)
main @ 012ecf51d39, clean checkout	3/3 abort
v24.18.0 (release binary)	0/3 — ~6,000 rounds clean
v22.22.3 (release binary)	0/3 — ~6,000 rounds clean

Bisect

Introduced by 46de80d — http2: avoid uaf while receiving and sending rst_stream (#64166, fixing #64113):

46de80de88c    BAD  (rc=134)
46de80de88c~1  GOOD

That commit defers session close while nghttp2_session_mem_recv() is in progress:

if (is_receiving()) {
  set_close_pending();
  pending_close_code_ = code;
  pending_close_socket_closed_ = socket_closed;
  return;   // finished later via MaybeFinishPendingClose()
}

Our reproduction destroys the session from inside a 'stream' handler, i.e. while receiving. Before that change the close completed immediately and the torn-down streams stopped receiving data. With the close deferred, nghttp2 keeps walking the same receive buffer and delivers DATA to streams whose JS wrappers have already been destroyed, so CallJSOnreadMethod finds onread unset.

So the fix for one use-after-free in the mem_recv window appears to have opened a different lifetime hole in the same window. Note this is not addressed by ba6cb5c34c2 (http2: defer rst stream while in scope) — I cherry-picked that on top and it still aborts 5/5.

Discovery

Found via undici's HTTP/2 test suite, where it surfaced under borp as a bare 'test failed' with one subtest silently missing and no assertion text — worth knowing, since that signature is easy to mistake for flakiness. Running the test file directly is what exposes exit 134.

cc @Eusgor @mcollina @pimterry @RafaelGSS