ReviewOS

zig-utils/zig-js

Threads: scope current-Thread TLS to Context entry and lifetime

#905
Closed chris opened this 9 days ago ·
9 days ago

Parents: #482, #495 Related: #886, #887, #896, #897

Source finding

At 54bb7b0, src/jsthread.zig has a native thread-local t_current pointing to a Context-owned ThreadRecord:

  • install sets it to the newly created realm's main record.
  • threadEntry replaces it with a spawned Thread's record.
  • No reset/restore assignment appears on Context destruction or host entry into a different Context.
  • currentThreadId, Thread.current, Thread-local storage helpers, property waitAsync queue routing, and asyncJoin destination selection read this pointer.
  • isRealmHostThread recognizes only t_current == null, although installation sets a non-null main record with id 0. Whole-run-loop quiescence uses this predicate.

This is a source audit, not a measured UAF or deadlock. Two live threaded Contexts on one OS thread can select the wrong main record; destroying the most recently installed Context appears to leave a pointer to its released record. The waitAsync ownership work in #897 makes this lifecycle invariant particularly relevant.

Required work

  • Produce deterministic witnesses for alternating two live Contexts, destroying the most recently created one then using the survivor, nested host entry, Thread.current identity, and host-versus-worker quiescence classification.
  • Give native Thread identity explicit lifetime and scoped realm-entry ownership. Host entry must select the correct realm's main record; exit and destruction must not retain dangling Context-owned state.
  • Preserve worker identity across nested calls and Thread-local cleanup, with the existing cross-realm and parallel-JS ownership rules.
  • Audit every t_current consumer, including local/host queue routing, Thread-local values, self-join checks, and contention attribution.
  • No dangling-pointer probing, arbitrary lifetime extension, process-global serialization, or silent queue fallback.

Acceptance

Exact identity and once-only queue delivery tests; destruction/alternating-entry and nested-host tests; moving/concurrent GC and suppression-free TSan; public/private ABI embedding fixtures and relevant Thread lifecycle/no-GIL cases. Cross-link implementation commits and distinguish source findings from measured failures.

9 days ago

The alternating-live-Context defect is now reproduced at 54bb7b0 (issue-905-before.log): one focused test, one failure, no leaks.

The witness creates threaded Context A with GC disabled and saves its Thread.current wrapper; creates Context B and saves B's distinct wrapper; then evaluates Thread.current in A again. The returned wrapper is not A's original wrapper. Both Contexts remain alive during the assertion, so this establishes wrong-realm native Thread identity without requiring a dangling-pointer dereference.

The after-destruction UAF hypothesis, nested-host entry behavior, and host-quiescence classification are still unmeasured. No implementation or closure is claimed yet. Witness source: the durable task evidence file thread-tls-905-witness.zig.

9 days ago

The candidate now passes two focused Debug tests, with zero failures/leaks. Coverage includes alternating live threaded Contexts, destruction followed by survivor use, arena and precise hosts, exact waitAsync delivery/reservation release, nested Context creation/destruction from both hosts and native workers, correct host-versus-worker classification, and removal of the worker's ThreadLocal map entry in GIL/no-GIL modes.

The implementation removes host records from native TLS entirely. TLS borrows only a live worker record and clears it before native exit. Each interpreter retains its owning Context even without GC; that Context publishes an immutable main-record pointer before entry/spawn. Every current-Thread consumer selects the native worker only for its owning Context and otherwise selects the entered realm's main record. Pending asyncJoin uses its realm destination directly.

The previously observed alternating-host identity defect is covered without probing a dangling pointer. The candidate is locally integrated with #896's checkpoint ownership changes; ABI, conformance, ReleaseSafe, TSan and wider lifecycle validation are in progress. No closure or performance claim yet.

9 days ago

Implemented and pushed as 4b6b5ec2 — fix(threads): scope current identity to the entered Context (#905).

The deterministic identity/lifetime witnesses pass Debug, ReleaseSafe and suppression-free TSan. They cover alternating live Contexts, destruction then survivor entry, nested host entry from both native hosts and workers, correct quiescence classification, exact waitAsync delivery/reservation release, and worker ThreadLocal cleanup in GIL/no-GIL. TLS now holds only a live native worker, clears before native exit, and is selected only for its owning Context; every host entry selects the entered Context's immutable main record. All current-TLS consumers were converted.

The final integrated source tree passes 2,380 full Debug tests, one Linux-only skip, zero failures/leaks; public C/Home private/global lifecycle/readable-stream/Bun C-API extension fixtures pass Debug, ReleaseSafe and unsuppressed TSan. Eighteen relevant PR-249 executions pass (nine lifecycle/identity/join/ThreadLocal/waiter cases in both GIL modes). Fuzz passes: default400, verify300, lifecycle3360, broad80, amplify30. Affected Test262 diagnostics remain 2,893/2,893 before/after, zero failures.

The full mid-GC run reports one separate collection-completion failure among 900 programs. Its exact evidence, inconclusive baseline/candidate replays, and new diagnostics remain open in #907; this is not a blanket claim of green collector acceptance. No identity/lifetime failure or causal attribution to this change was established there.

The requested scoped native identity/lifetime behavior and relevant embedding/GC/thread validation are complete. Closing this issue; broader collector progress and performance work remain tracked in their owning issues.

Sign in to comment on this issue.