Skip to content

Commit e7cd9d9

Browse files
committed
fix: use-after-free crash at process exit from per-instance cleanup hooks
Every VideoDecoderAsync/VideoEncoderAsync constructor registered an env cleanup hook whose argument pointed at a member of the instance, and the destructor never removed it. Any instance GC'd during the process's lifetime left a dangling hook; at exit, RunCleanup() executed it, writing through freed memory — which by then was often reused by node's own CleanupQueue hash table, corrupting it so the next ThreadSafeFunction destructor's RemoveEnvironmentCleanupHook crashed (confirmed via crash report: SIGSEGV in CleanupHookCallback hash find during RunCleanup). Replace the per-instance hooks with one addon-level flag registered once in Init. Also pin the JS wrapper (Ref/Unref) while jobs are in flight so queued job-done callbacks, which capture this, can never outlive the instance; Close() balances the pin. Repro (construct/encode/drop 120 encoders without close, GC, exit) crashed ~5% of runs on main; 0/60 after the fix. This is the likely root cause of the ubuntu CI flake since v1.2.0 and the FFmpeg-7.x static-build exit aborts. Claude-Session: https://claude.ai/code/session_01QCBuz9ekP3aosHS34QaAqK
1 parent f135c81 commit e7cd9d9

7 files changed

Lines changed: 45 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
# Changelog
22

3+
## [1.3.1] - 2026-07-18
4+
5+
### Fixed
6+
- Intermittent crash at process exit (SIGSEGV/SIGABRT during teardown). Each codec instance registered an env cleanup hook pointing into itself and never removed it; instances collected during the process's lifetime left dangling hooks that corrupted Node's cleanup queue when run at exit. One process-level flag now serves all instances, and codec objects are pinned while work is in flight so queued completion callbacks can never outlive them. Also the likely cause of the ubuntu CI flake since v1.2.0 and the FFmpeg-7.x static-build aborts.
7+
38
## [1.3.0] - 2026-07-15
49

510
### Added

native/async_decoder.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "async_decoder.h"
2+
#include "env_state.h"
23
#include "frame.h"
34

45
Napi::FunctionReference VideoDecoderAsync::constructor;
@@ -63,22 +64,22 @@ VideoDecoderAsync::VideoDecoderAsync(const Napi::CallbackInfo& info)
6364
1
6465
);
6566
tsfnJobDone_.Unref(env);
66-
67-
// At env teardown TSFNs are finalized before this wrapper's destructor
68-
// runs; flag it so the destructor skips Release on dead handles
69-
env.AddCleanupHook([](bool* flag) { *flag = true; }, &envTeardown_);
7067
}
7168

7269
// Hold the event loop open while jobs are in flight (JS thread only)
7370
void VideoDecoderAsync::JobSubmitted(Napi::Env env) {
7471
if (activeJobs_++ == 0) {
7572
tsfnOutput_.Ref(env);
73+
// pin the wrapper so queued job-done callbacks (which capture this)
74+
// can never outlive the instance
75+
Ref();
7676
}
7777
}
7878

7979
void VideoDecoderAsync::JobFinished(Napi::Env env) {
8080
if (activeJobs_ > 0 && --activeJobs_ == 0) {
8181
tsfnOutput_.Unref(env);
82+
Unref();
8283
}
8384
}
8485

@@ -98,7 +99,7 @@ VideoDecoderAsync::~VideoDecoderAsync() {
9899
}
99100

100101
// Release thread-safe functions unless env teardown already finalized them
101-
if (!envTeardown_) {
102+
if (!nwc_env_teardown.load()) {
102103
if (tsfnOutput_) tsfnOutput_.Release();
103104
if (tsfnError_) tsfnError_.Release();
104105
if (tsfnFlush_) tsfnFlush_.Release();
@@ -490,7 +491,10 @@ void VideoDecoderAsync::Close(const Napi::CallbackInfo& info) {
490491
if (tsfnError_) { tsfnError_.Release(); tsfnError_ = Napi::ThreadSafeFunction(); }
491492
if (tsfnFlush_) { tsfnFlush_.Release(); tsfnFlush_ = Napi::ThreadSafeFunction(); }
492493
if (tsfnJobDone_) { tsfnJobDone_.Release(); tsfnJobDone_ = Napi::ThreadSafeFunction(); }
493-
activeJobs_ = 0;
494+
if (activeJobs_ > 0) {
495+
activeJobs_ = 0;
496+
Unref(); // balance the in-flight pin; queued JobFinished sees 0 and skips
497+
}
494498

495499
configured_ = false;
496500
}

native/async_decoder.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ class VideoDecoderAsync : public Napi::ObjectWrap<VideoDecoderAsync> {
6363

6464
// Worker thread
6565
std::thread workerThread_;
66-
bool envTeardown_ = false;
66+
6767
Napi::ThreadSafeFunction tsfnJobDone_;
6868
int activeJobs_ = 0;
6969
void JobSubmitted(Napi::Env env);

native/async_encoder.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include "async_encoder.h"
2+
#include "env_state.h"
23
#include "frame.h"
34
#include "color.h"
45
#include "svc.h"
@@ -80,22 +81,22 @@ VideoEncoderAsync::VideoEncoderAsync(const Napi::CallbackInfo& info)
8081
1
8182
);
8283
tsfnJobDone_.Unref(env);
83-
84-
// At env teardown TSFNs are finalized before this wrapper's destructor
85-
// runs; flag it so the destructor skips Release on dead handles
86-
env.AddCleanupHook([](bool* flag) { *flag = true; }, &envTeardown_);
8784
}
8885

8986
// Hold the event loop open while jobs are in flight (JS thread only)
9087
void VideoEncoderAsync::JobSubmitted(Napi::Env env) {
9188
if (activeJobs_++ == 0) {
9289
tsfnOutput_.Ref(env);
90+
// pin the wrapper so queued job-done callbacks (which capture this)
91+
// can never outlive the instance
92+
Ref();
9393
}
9494
}
9595

9696
void VideoEncoderAsync::JobFinished(Napi::Env env) {
9797
if (activeJobs_ > 0 && --activeJobs_ == 0) {
9898
tsfnOutput_.Unref(env);
99+
Unref();
99100
}
100101
}
101102

@@ -124,7 +125,7 @@ VideoEncoderAsync::~VideoEncoderAsync() {
124125
}
125126

126127
// Release thread-safe functions unless env teardown already finalized them
127-
if (!envTeardown_) {
128+
if (!nwc_env_teardown.load()) {
128129
if (tsfnOutput_) tsfnOutput_.Release();
129130
if (tsfnError_) tsfnError_.Release();
130131
if (tsfnFlush_) tsfnFlush_.Release();
@@ -969,7 +970,10 @@ void VideoEncoderAsync::Close(const Napi::CallbackInfo& info) {
969970
if (tsfnError_) { tsfnError_.Release(); tsfnError_ = Napi::ThreadSafeFunction(); }
970971
if (tsfnFlush_) { tsfnFlush_.Release(); tsfnFlush_ = Napi::ThreadSafeFunction(); }
971972
if (tsfnJobDone_) { tsfnJobDone_.Release(); tsfnJobDone_ = Napi::ThreadSafeFunction(); }
972-
activeJobs_ = 0;
973+
if (activeJobs_ > 0) {
974+
activeJobs_ = 0;
975+
Unref(); // balance the in-flight pin; queued JobFinished sees 0 and skips
976+
}
973977

974978
configured_ = false;
975979
}

native/async_encoder.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class VideoEncoderAsync : public Napi::ObjectWrap<VideoEncoderAsync> {
7171

7272
// Worker thread
7373
std::thread workerThread_;
74-
bool envTeardown_ = false;
74+
7575
Napi::ThreadSafeFunction tsfnJobDone_;
7676
int activeJobs_ = 0;
7777
void JobSubmitted(Napi::Env env);

native/binding.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include <napi.h>
2+
#include "env_state.h"
23
#include "frame.h"
34
#include "audio.h"
45
#include "encoder.h"
@@ -11,7 +12,14 @@
1112
// Forward declaration
1213
void InitUtil(Napi::Env env, Napi::Object exports);
1314

15+
std::atomic<bool> nwc_env_teardown{false};
16+
1417
Napi::Object Init(Napi::Env env, Napi::Object exports) {
18+
// One hook with static-lifetime state; per-instance hooks dangled after
19+
// GC and corrupted node's cleanup queue at exit
20+
env.AddCleanupHook([](std::atomic<bool>* flag) { flag->store(true); },
21+
&nwc_env_teardown);
22+
1523
// Initialize frame classes
1624
VideoFrameNative::Init(env, exports);
1725

native/env_state.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#pragma once
2+
#include <atomic>
3+
4+
// Set once at env teardown (registered in binding.cpp Init). Object
5+
// destructors consult it to skip releasing thread-safe functions the runtime
6+
// has already finalized. Process-wide: with worker_threads a torn-down env
7+
// can make another env's destructors skip a release (a leak, never a crash),
8+
// which beats the alternative — per-instance cleanup hooks dangled after GC
9+
// and corrupted node's cleanup queue (crash at exit).
10+
extern std::atomic<bool> nwc_env_teardown;

0 commit comments

Comments
 (0)