From 511d2095133c470ad503ffa8c6039757ba96e329 Mon Sep 17 00:00:00 2001 From: Claude Perrin Date: Sun, 17 May 2026 17:48:06 +0200 Subject: [PATCH 1/4] fix(addon): unref TSFN + release it on env teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/addon.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/addon.cpp b/src/addon.cpp index 15795ab..c957990 100644 --- a/src/addon.cpp +++ b/src/addon.cpp @@ -22,8 +22,29 @@ Napi::Object InitAll(Napi::Env env, Napi::Object exports) { "LoggerCallback", 0, 1); + // The logger callback is fire-and-forget. Without Unref(), the + // TSFN keeps a strong ref on the loop and a `require()` from a + // short-lived CLI hangs forever waiting on the TSFN. + tsfn.Unref(env); + + // Release the TSFN when the N-API env tears down. Without this, + // a later libsession log from a background thread could + // BlockingCall into a destroyed env (abort / UAF). + napi_add_env_cleanup_hook( + env, + [](void*) { + if (tsfn) { + tsfn.Release(); + tsfn = nullptr; + } + }, + nullptr); session::add_logger([](std::string_view msg) { + // env-cleanup may have raced ahead — silently drop the log + // instead of aborting on a released TSFN. + if (!tsfn) + return; tsfn.BlockingCall( new std::string(msg), [](Napi::Env env, Napi::Function jsCallback, std::string* msg) { From e88f16bf6c96166ce063a140816d1043eb443e53 Mon Sep 17 00:00:00 2001 From: Claude Perrin Date: Sun, 17 May 2026 17:48:30 +0200 Subject: [PATCH 2/4] fix(build_release_archive): strip script dir from sys.path before import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- build_release_archive.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/build_release_archive.py b/build_release_archive.py index e0683d1..9bd1103 100755 --- a/build_release_archive.py +++ b/build_release_archive.py @@ -1,6 +1,23 @@ #!/usr/bin/python3 # -*- coding: utf-8 -*- +"""Thin wrapper around the pip-pinned `git-archive-all` console entry. + +Originally `from git_archive_all import main` after a bare `import sys`, +which imports `git_archive_all` with `sys.path[0]` set to this script's +directory (the repo root). A checked-in `git_archive_all.py` or +`git_archive_all/` directory in the repo would then shadow the +hash-pinned wheel and execute arbitrary code at release-tarball time. +Strip `sys.path[0]` before the import so the venv (or site-packages) +is the only source of truth. +""" import sys +import os + +# Drop sys.path[0] (this script's directory) so an attacker can't +# shadow the pinned package by adding a same-named module to the repo. +if sys.path and sys.path[0] in ("", os.path.dirname(os.path.abspath(__file__))): + del sys.path[0] + from git_archive_all import main sys.exit(main()) From edf1733efc07a0015aed17520b8fe8381ad651e1 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 27 Jul 2026 11:12:20 +0200 Subject: [PATCH 3/4] fix: clean up multi thread + git_archive_all dep --- build_release_archive.py | 24 --------- prepare_release.sh | 4 +- requirements.txt | 10 ++++ src/addon.cpp | 104 ++++++++++++++++++++++++--------------- 4 files changed, 76 insertions(+), 66 deletions(-) delete mode 100755 build_release_archive.py create mode 100644 requirements.txt diff --git a/build_release_archive.py b/build_release_archive.py deleted file mode 100755 index 9bd1103..0000000 --- a/build_release_archive.py +++ /dev/null @@ -1,24 +0,0 @@ -#!/usr/bin/python3 -# -*- coding: utf-8 -*- -"""Thin wrapper around the pip-pinned `git-archive-all` console entry. - -Originally `from git_archive_all import main` after a bare `import sys`, -which imports `git_archive_all` with `sys.path[0]` set to this script's -directory (the repo root). A checked-in `git_archive_all.py` or -`git_archive_all/` directory in the repo would then shadow the -hash-pinned wheel and execute arbitrary code at release-tarball time. -Strip `sys.path[0]` before the import so the venv (or site-packages) -is the only source of truth. -""" -import sys -import os - -# Drop sys.path[0] (this script's directory) so an attacker can't -# shadow the pinned package by adding a same-named module to the repo. -if sys.path and sys.path[0] in ("", os.path.dirname(os.path.abspath(__file__))): - del sys.path[0] - -from git_archive_all import main - -sys.exit(main()) - diff --git a/prepare_release.sh b/prepare_release.sh index a3a157f..8e57726 100755 --- a/prepare_release.sh +++ b/prepare_release.sh @@ -12,7 +12,7 @@ read_char() { rm -f ./libsession_util_nodejs*.tar.gz python -m venv .venv . .venv/bin/activate -pip install git-archive-all +pip install -r requirements.txt --require-hashes PACKAGE_VERSION=$(node -p "require('./package.json').version") GIT_COMMIT=$(git rev-parse HEAD) @@ -42,7 +42,7 @@ esac echo "Continuing..." echo "Building tar archive of source..." -python3 build_release_archive.py libsession_util_nodejs-v$PACKAGE_VERSION.tar.gz --include src/version.h +git-archive-all libsession_util_nodejs-v$PACKAGE_VERSION.tar.gz --include src/version.h echo "tar archive size:" du -sh libsession_util_nodejs*.tar.gz diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..794d832 --- /dev/null +++ b/requirements.txt @@ -0,0 +1,10 @@ +# Release-tooling Python dependencies, version- and hash-pinned so a +# compromised PyPI account / dependency-confusion upload can't inject a +# different artifact at release-tarball time. Installed via: +# pip install -r requirements.txt --require-hashes +# prepare_release.sh then invokes the `git-archive-all` console script +# from the venv directly, so a repo-root git_archive_all.py can't shadow +# this pinned wheel (the CWD is never on sys.path for a console script). +git-archive-all==1.23.1 \ + --hash=sha256:d9f9611f2df629de3df1d6f134955ced4c704cbc0e423d769a7c0e55a8484242 \ + --hash=sha256:a42fe0cedd2e0361250a4f3130f3d3543f640d4f1fe28530771df5972108729f diff --git a/src/addon.cpp b/src/addon.cpp index c957990..04bb65c 100644 --- a/src/addon.cpp +++ b/src/addon.cpp @@ -1,5 +1,6 @@ #include +#include #include #include "blinding/blinding.hpp" @@ -13,51 +14,74 @@ #include "user_groups_config.hpp" Napi::ThreadSafeFunction tsfn; +// Guards `tsfn` against the race between libsession's background log +// threads (which read it and BlockingCall through it) and N-API env +// teardown (which Releases and nulls it). The check + call in the +// logger and the check + release in the cleanup hook must each be +// atomic w.r.t. the other, or we risk a torn read / use-after-free on +// the wrapper. +std::mutex tsfn_mutex; Napi::Object InitAll(Napi::Env env, Napi::Object exports) { - tsfn = Napi::ThreadSafeFunction::New( - env, - Napi::Function::New(env, [](const Napi::CallbackInfo& info) {}), - "LoggerCallback", - 0, - 1); - // The logger callback is fire-and-forget. Without Unref(), the - // TSFN keeps a strong ref on the loop and a `require()` from a - // short-lived CLI hangs forever waiting on the TSFN. - tsfn.Unref(env); + { + std::lock_guard lock(tsfn_mutex); + tsfn = Napi::ThreadSafeFunction::New( + env, + Napi::Function::New(env, [](const Napi::CallbackInfo& info) {}), + "LoggerCallback", + 0, + 1); + // The logger callback is fire-and-forget. Without Unref(), the + // TSFN keeps a strong ref on the loop and a `require()` from a + // short-lived CLI hangs forever waiting on the TSFN. + tsfn.Unref(env); + } - // Release the TSFN when the N-API env tears down. Without this, - // a later libsession log from a background thread could - // BlockingCall into a destroyed env (abort / UAF). - napi_add_env_cleanup_hook( - env, - [](void*) { - if (tsfn) { - tsfn.Release(); - tsfn = nullptr; - } - }, - nullptr); + // Release the TSFN when the N-API env tears down. Without this, a + // later libsession log from a background thread could BlockingCall + // into a destroyed env (abort / UAF). Taking the lock makes the + // check + release atomic w.r.t. the logger callback below. + env.AddCleanupHook([]() { + std::lock_guard lock(tsfn_mutex); + if (tsfn) { + tsfn.Release(); + tsfn = nullptr; + } + }); - session::add_logger([](std::string_view msg) { - // env-cleanup may have raced ahead — silently drop the log - // instead of aborting on a released TSFN. - if (!tsfn) - return; - tsfn.BlockingCall( - new std::string(msg), - [](Napi::Env env, Napi::Function jsCallback, std::string* msg) { - Napi::HandleScope scope(env); - Napi::Function consoleLog = env.Global() - .Get("console") - .As() - .Get("log") - .As(); - Napi::String jsStr = Napi::String::New(env, "libsession: " + *msg); - consoleLog.Call({jsStr}); - delete msg; - }); + // Register the libsession -> console.log bridge exactly once per + // process. Re-running InitAll (multiple Workers / vm contexts) must + // not stack duplicate callbacks onto libsession's global logger + // registry — the single persistent callback always reads whatever + // the current `tsfn` is, under the lock. + static std::once_flag logger_once; + std::call_once(logger_once, [] { + session::add_logger([](std::string_view msg) { + // Hold the lock across the check + BlockingCall so env + // teardown can't Release/null `tsfn` underneath us. + std::lock_guard lock(tsfn_mutex); + if (!tsfn) + return; + auto* payload = new std::string(msg); + napi_status status = tsfn.BlockingCall( + payload, [](Napi::Env env, Napi::Function jsCallback, std::string* msg) { + Napi::HandleScope scope(env); + Napi::Function consoleLog = env.Global() + .Get("console") + .As() + .Get("log") + .As(); + Napi::String jsStr = Napi::String::New(env, "libsession: " + *msg); + consoleLog.Call({jsStr}); + delete msg; + }); + // On a rejected call (e.g. napi_closing if a Release slipped + // in) the finalize callback never runs, so the payload would + // leak — free it here instead. + if (status != napi_ok) + delete payload; + }); }); oxen::log::set_level_default(oxen::log::Level::info); From 48ac7b9e640be90e73db4a548d2bc78846860ee9 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 27 Jul 2026 11:20:05 +0200 Subject: [PATCH 4/4] chore: lint --- include/utilities.hpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/utilities.hpp b/include/utilities.hpp index d92e918..ac97829 100644 --- a/include/utilities.hpp +++ b/include/utilities.hpp @@ -459,7 +459,9 @@ std::vector from_base64_to_vector(std::string_view x); // Concept to match containers with a size() method template concept HasSize = requires(T t) { - { t.size() } -> std::convertible_to; + { + t.size() + } -> std::convertible_to; }; template