fix(addon): unref TSFN, env cleanup hook + sys.path strip#60
Open
klodr wants to merge 2 commits into
Open
Conversation
The logger ThreadSafeFunction was created with strong-ref + count=1
and never Unref()'d or Release()'d. Two observable consequences:
1. `node -e "require('./')"` hangs forever after the require resolves
because the TSFN holds a ref on the loop with no pending work.
`tsfn.Unref(env)` after creation lets short-lived CLIs / scripts
exit cleanly while still allowing the BlockingCall to deliver
while the loop is alive for other reasons.
2. On N-API env teardown (Worker shutdown, vm context reload), a
later libsession log emission from a background thread would
BlockingCall into a destroyed env, which is UB and can abort or
call into a stale handle. `napi_add_env_cleanup_hook` Releases
the TSFN and nulls the global; the add_logger lambda guards with
`if (!tsfn) return` so a race between Release and the log thread
drops silently instead of aborting.
The single-env case (95% of consumers, including the default
require flow) is now safe. Multi-env scenarios (multiple Workers
loading the addon concurrently) still race on the process-global
handle — that needs a per-env refactor via napi_set_instance_data
and is left for a follow-up.
Signed-off-by: Claude Perrin <klodr@users.noreply.github.com>
`python3 build_release_archive.py` imports `git_archive_all` while `sys.path[0]` is the repo root. A checked-in `git_archive_all.py` (or directory) shadowing the hash-pinned wheel would execute at release-tarball-creation time — nullifying the pin and giving arbitrary code execution to anyone who can land a PR with such a file. Drop the leading sys.path entry so the venv / site-packages is the only source of truth for the import. Signed-off-by: Claude Perrin <klodr@users.noreply.github.com>
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
1 participant
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.
Two unrelated fixes bundled (small surface):
src/addon.cpp:tsfn.Unref(env)+napi_add_env_cleanup_hookso a CLI-stylerequire()doesn't hang on the TSFN's strong ref, and a later libsession log from a background thread doesn't BlockingCall into a destroyed env (abort / UAF). The logger callback already drops messages when the TSFN has been released.build_release_archive.py: stripsys.path[0](the script's own directory) beforeimport git_archive_all, so a checked-ingit_archive_all.py(or directory) at the repo root can't shadow the hash-pinned wheel and execute arbitrary code at release-tarball time.Test plan
pnpm installfrom a freshly-cloned repo worksrequire()s this binding exits cleanly (no hang)prepare_release.shbuilds the tarball as before