Possible `ExitEvent` leak when `BlockingCall()` returns `napi_closing`
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 76/100
- Issue type
- Bug
- Clarity
- Clearly specified
- Activity status
- Quiet
- Domain
- operating-systems
Research direction
Start in SetupExitCallback in src/unix/pty.cc and src/win/conpty.cc, tracing ownership around BlockingCall and the napi_closing branch. Apply the issue's suggested cleanup in both implementations, then verify that the ExitEvent is released when the callback is not queued and remains callback-owned otherwise.
Written by the indexing model from the issue text.
Description
Possible ExitEvent leak when BlockingCall() returns napi_closing
I found a possible native heap leak in the pty exit watcher when the thread-safe function is closing.
Files: src/unix/pty.cc, src/win/conpty.cc
Function: SetupExitCallback
Relevant Unix code:
auto callback = [](Napi::Env env, Napi::Function cb, ExitEvent *exit_event) {
cb.Call({Napi::Number::New(env, exit_event->exit_code),
Napi::Number::New(env, exit_event->signal_code)});
delete exit_event;
};
// ...
ExitEvent *exit_event = new ExitEvent;
// fill exit_event
auto status = tsfn.BlockingCall(exit_event, callback);
switch (status) {
case napi_closing:
break;
case napi_queue_full:
Napi::Error::Fatal("SetupExitCallback", "Queue was full");
case napi_ok:
if (tsfn.Release() != napi_ok) {
Napi::Error::Fatal("SetupExitCallback", "ThreadSafeFunction.Release() failed");
}
break;
}
The Windows implementation has the same ownership pattern.
The ExitEvent is deleted only by the JS-thread callback. If BlockingCall()
returns napi_closing, the item was not queued and that callback will not run.
The case napi_closing: branch then drops the only pointer to exit_event.
This is a small teardown-race leak: one ExitEvent per pty whose exit races
Node-API environment shutdown.
Suggested fix: delete exit_event in the napi_closing branch, where ownership
was not transferred to the callback queue.
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 337
- Avg merge
- 21h 58m
- Merged PRs (30d)
- 3
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from microsoft/node-pty
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 82/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
All issues in microsoft/node-pty
Similar issues
-
calcite-components needs triage refactor
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
Esri/calcite-design-system#15203 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 91/100
-
community first-timers-only good first issue hacktoberfest help wanted low hanging fruit up-for-grabs
Difficulty 1/5 Under an hour Newbie friendliness 95/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Automattic/studio#4908 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 90/100