ReviewOS

zig-utils/zig-js

GC: reject poisoned string publication under no-GIL clone churn

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

Parent: #478 Related: #476, #493, #647

Goal

Preserve exact string storage ownership and lifetime while no-GIL property writers race multi-slot clone/enumeration consumers. No engine path may publish or copy a StringCell from freed, poisoned, torn, or otherwise invalid bytes.

Fresh failure

Fresh main CI run 32369900898, Linux nogil-corpus-releasesafe (shard 3/4), failed only cve/mc-val-multislot-clone.js after 61/62 cases passed. The bounded diagnostics from #647 exposed:

thread 2880 panic: StringCell: invalid WTF-8 lead byte 0xaa at index 0 of aaaaaaaaaaaa
src/strcell.zig:432 debugAssertWtf8
src/gc.zig:4332 finishManagedString
src/gc.zig:4351 allocManagedString
src/strcell.zig:450 createCell
src/value.zig:6468 strAlloc

The witness runs two foreign writers that transition/delete rotating properties while the main thread exercises Object.assign, spread, JSON.stringify, and direct for-in. The 0xaa bytes are consistent with poisoned/stale storage, but the exact ownership violation still needs attribution; this issue does not presume use-after-free until a lifetime trace proves it.

Scope

  • Reproduce the failure deterministically or amplify it without weakening the original MC-VAL witness.
  • Trace the source slice, allocator identity, property backing/shape generation, string ownership state, and GC reachability from property read through strAlloc and finishManagedString.
  • Prove publication ordering and lifetime for deleted/quarantined slots, superseded backing, cloned values, and managed-string allocation across realms/threads.
  • Fix the owning lifetime/synchronization contract; keep the WTF-8 tripwire as a security invariant.
  • Audit equivalent tree-walker, VM, clone/spread, JSON, and enumeration paths for the same stale-byte window.

No-workaround rules

  • No GIL, witness skip, reduced churn, added sleep, disabled poison/tripwire, leaked/quarantined storage without a bounded reclamation proof, copied-everything slow path, or retry that hides an invalid read.
  • Do not accept merely converting the panic into replacement text or a catchable error: valid JavaScript strings must retain their exact value.

Acceptance

  • A focused deterministic/amplified witness demonstrates the pre-fix ownership violation and passes after the fix.
  • The exact source allocation and reclamation race is documented with synchronization and memory-order reasoning.
  • cve/mc-val-multislot-clone.js passes no-GIL in Debug, ReleaseSafe, and suppression-free TSan within its existing budget.
  • Object.assign, spread, JSON, for-in, property delete/transition, string GC/compaction, and fault/OOM coverage pass with exact values and no leaks.
  • Full unit, no-GIL corpus, threadfuzz broad/mid-GC/verify, and relevant sanitizer gates pass.
  • #647 remains capable of exposing the exact panic if the invariant is deliberately re-broken.
chris 9 days ago

A second fresh job confirms the same lifetime edge in a different build mode: Debug no-GIL shard 3/4 passed 61/62, then segfaulted in isPrivateKey -> objectHasOwn -> hasPropertyResult while reading the snapshotted key. ReleaseSafe instead survived long enough to expose 0xaa in finishManagedString.

Attribution: Object.ownKeysWithScratch returns key slice headers under property_lock, but key bytes sourced from key_order/accessor backing remain borrowed. A writer can run maybeCompactKeyOrderUnlocked -> replaceKeyOrderUnlocked immediately after unlock and free those spellings while for-in still owns the snapshot. Immutable shape-only spellings do not have this lifetime problem.

The candidate fix snapshots only reclaimable key sources under the existing lock and keeps shape-only enumeration zero-copy. A focused unit test now explicitly frees every source spelling after ownKeysWithScratch returns and verifies the result remains exact. Validation is pending a clear local machine; no CI expectation or corpus budget is being changed.

chris 9 days ago

Implemented and pushed in ca17e46a. Reclaimable key-order/accessor spellings are now copied under the existing property lock; immutable shape-only spellings remain zero-copy. The focused test frees every source key immediately after ownKeysWithScratch returns and proves the snapshot still owns exact bytes.

Local exact dev.1818 evidence:

  • ownership unit: 1/1, 0 leaks;
  • original no-GIL case, Debug: 1/1 in 24.23s;
  • original no-GIL case, ReleaseSafe: 1/1 in 238ms;
  • suppression-free TSan: 1/1 with no warning (timing not promoted because a competing Home job began during collection).

Fresh isolated Linux CI is the remaining closure evidence; no corpus expectation, iteration, timeout, GIL, poison, or tripwire changed.

chris 9 days ago

Follow-up 5385a1d9 corrects the focused ownership test after fresh Linux focused-engine-tests exposed an invalid allocator oracle. FixedBufferAllocator.end_index can retain alignment padding after a correctly freed hash table; the test now uses Zig’s leak-checking test allocator as a scratch store distinct from the result arena. Local result: 1 passed, 0 failed, 0 leaked. Semantics and Test262 expectations are unchanged (0 flips). Fresh CI is pending.

chris 9 days ago

Implemented by fix(gc): own mutable property keys across snapshots (#651) and its focused-test correction test(gc): use leak-checking key snapshot storage (#651).

Fresh cumulative Linux evidence on run 32406160824:

The cause is the documented post-unlock lifetime race in reclaimable key-order/accessor spellings. Copying only those spellings under the existing property lock closes the stale-byte window; immutable shape spellings remain zero-copy, and the WTF-8 poison tripwire remains active. Local exact-value, deletion/transition, clone/spread/JSON/for-in, GC, allocation-failure, and no-leak witnesses passed. The separate TSan unit failure in this run is the tracked user-owned #643 Map/Set iterator race, not this string/key ownership surface.

Sign in to comment on this issue.