fix(node_loader): plug 48-byte trampoline leak per metacall_await call (#578)#641
Draft
k5602 wants to merge 1 commit intometacall:developfrom
Draft
fix(node_loader): plug 48-byte trampoline leak per metacall_await call (#578)#641k5602 wants to merge 1 commit intometacall:developfrom
k5602 wants to merge 1 commit intometacall:developfrom
Conversation
Fix ownership of loader_impl_async_future_await_trampoline used by metacall_await handlers. Previously trampoline allocations could leak (regression metacall#578) after resolve/reject callbacks completed. Add a std::unique_ptr guard in both resolve and reject trampoline entry points to ensure the trampoline is deallocated once the callback executes. Also add AwaitTrampolineOwnershipRegression unit test to exercise resolve, reject and exception paths and assert expected callback counts.
a9e936b to
d977527
Compare
Member
|
@k5602 could you reproduce this bug? Because I have had it inside the linux distributable but it does not happen inside the CI. Does your test reproduce the problem? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Each call to
metacall_await/metacall_await_futureallocates aloader_impl_async_future_await_trampoline_typestruct (48 bytes) on the C++ heap and passes it into JavaScript as annapi_wrap-ped object (trampoline_ptr). When the JS promise settles,bootstrap.jscalls back intonode_loader_trampoline_resolveornode_loader_trampoline_reject, which extract the pointer vianapi_remove_wrap.napi_remove_wrapdetaches the native pointer from the JS object and suppresses the NAPI finalizer — transferring full ownership to C. Before this fix, the pointer was used and then silently abandoned, producing a 48-byte leak per settledawaitcall. Across the 3 allocations reported in #578 that accounts for the 144-byte figure observed under both ASAN and Heaptrack.Fix
Wrap the detached pointer in a
std::unique_ptrimmediately afternapi_remove_wrapin bothnode_loader_trampoline_resolveandnode_loader_trampoline_reject:loader_impl_async_future_await_trampoline trampoline = static_cast<loader_impl_async_future_await_trampoline>(result); std::unique_ptr<loader_impl_async_future_await_trampoline_type> trampoline_guard(trampoline); // trampoline_guard destructs at end of block, after return value is fully materialised return trampoline->resolve_trampoline(trampoline->node_impl, env, trampoline->resolve_callback, recv, args[1], trampoline->context);No behaviour change. The
returnexpression evaluates fully before theunique_ptrdestructor runs (end-of-block), so there is no use-after-free.Fixes #578
Type of change
Checklist:
make testorctest -VV -R <test-name>)../docker-compose.sh test &> outputand attached the output. (non-breaking single-file fix; full Docker suite not run locally)OPTION_BUILD_ADDRESS_SANITIZERor./docker-compose.sh test-address-sanitizer &> outputandOPTION_TEST_MEMORYCHECK. (built with-fsanitize=address,leak;AwaitTrampolineOwnershipRegressionpasses with zeroDirect leak/Indirect leakreports)OPTION_BUILD_THREAD_SANITIZERor./docker-compose.sh test-thread-sanitizer &> output. (fix is single-threaded ownership; no new synchronisation primitives introduced)Helgrindin case my code works with threading. (no threading changes;unique_ptrdestructs on the same thread that callednapi_remove_wrap)make clang-formatin order to format my code and my code follows the style guidelines.Test:
AwaitTrampolineOwnershipRegressionAdded to
metacall-node-async-test, exercising all three trampoline settlement paths:f(x)—new Promise(r => r(x))resolve_cbfiredg(x)—new Promise((_, r) => r(x))reject_cbfiredi()—asyncthrowreject_cbfiredPost-
metacall_destroy()assertions confirm exactly 1 resolve and 2 reject callbacks fired. Under ASAN withdetect_leaks=1the test produces a clean report with the fix applied.Additional info
this is my first pr in my GSoC campaign so i hope it's good and very welcoming to reviews from mentors.