AsyncSRT.dispose()/worker.terminate() causes native heap corruption (reproduces on v1.5.5 and v1.5.6)

[birme]

Summary

AsyncSRT.dispose() (which calls Worker.terminate() under the hood — see src/async.js) causes native heap corruption when called on a worker that has active or recently-closed SRT sockets. This reproduces on both v1.5.5 and v1.5.6 of the bundled libSRT — it is unrelated to the version-bump work in #89/#90.

This code path currently has no test coverage: dispose()/worker.terminate() is never called in spec/async_srt_await_spec.js (it's commented out) and is absent from every other spec file.

Reproduction

In a spec exercising AsyncSRT: create socket(s), close them via the async API, then call dispose() on the AsyncSRT instance (which calls Worker.terminate()) before the process exits.

Observed on Linux (node:20-bookworm, arm64, matching CI's toolchain):

free(): invalid pointer

and, on other runs of the same scenario:

free(): invalid next size (fast)

and one occurrence of a V8 fatal error during worker teardown:

Check failed: node->IsInUse().

All are consistent with a race between Worker.terminate() tearing down the worker thread/its Realm and the native addon's own in-flight close()/socket-teardown state still running inside that same worker thread — i.e. terminate() doesn't wait for the native binding's cleanup to finish before the thread (and its heap) is torn down.

Why this matters

Heap corruption (free(): invalid pointer / invalid next size) is a use-after-free/double-free class bug — exploitable in principle, not just a crash. Any application using AsyncSRT.dispose() for graceful shutdown (a reasonable, expected use of a public API method) can hit this today, independent of which libSRT version is bundled.

Suggested fix direction

Worker.terminate() is inherently abrupt — it doesn't let in-flight work finish. A safer dispose() likely needs to:

• signal the worker to finish any pending native close()/socket teardown and acknowledge completion (e.g. a message round-trip) before calling worker.terminate(), or
• avoid terminate() entirely in favor of a graceful shutdown message the worker acts on and then exits on its own.

Suggested test coverage

Un-comment and fix the existing dispose() call in spec/async_srt_await_spec.js, and add explicit close+dispose coverage to the other async spec files, so this path is exercised by CI going forward.

---

Found incidentally while investigating #89/#90 (libSRT v1.5.6 CVE fix); filed separately since it's unrelated to that work and affects the current release.

[birme]

Thank you for this thorough and precise report, @birme. The analysis is exactly right, and the code confirms it.

In src/async.js, dispose() calls Worker.terminate() — which is inherently fire-and-forget at the JS level. It sends a termination signal to the worker thread but does not wait for the native addon's own in-flight close()/socket teardown to complete before the thread and its associated heap are torn down. Any libSRT or N-API state that was still active inside that worker thread is then freed from underneath it, producing exactly the free(): invalid pointer / free(): invalid next size (fast) / Check failed: node->IsInUse() errors described here. These are classic use-after-free / double-free symptoms from a torn-down thread's heap.

The impact is real and security-class: this is reachable through a documented, public API (AsyncSRT.dispose()) that users are expected to call for graceful shutdown. It is not a corner case.

Fix directions (both valid):

1. Graceful-shutdown message round-trip — before calling worker.terminate(), send the worker a shutdown message and await a reply that signals all native cleanup is complete, then call terminate(). This aligns with how other N-API addons handle cooperative teardown.
2. Worker self-exit — have the worker drain its queue and exit on its own, avoiding terminate() entirely. Simpler in some designs.

Either path requires touching src/async.js and src/async-worker.js (and possibly the N-API binding layer). A fix PR should also un-comment and extend the dispose() test coverage in spec/async_srt_await_spec.js and add explicit close+dispose coverage to the other async spec files, so this path is exercised by CI going forward.

Because this involves the native addon's async/worker layer, any fix PR will be reviewed as Tier B and merged by a human maintainer.

Marking this ready-for-maintainer given the security-class severity and the native-layer scope. A fix PR against master with the test coverage described would be very welcome.

— node-srt maintenance bot