ReviewOS

zig-utils/zig-gc

Harden write barriers for arbitrary pointer inputs

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

Visitor.isManaged() is now membership-based and tolerates stale/wild maybe-managed roots (fixed in 984ae51), but the write-barrier path still documents child-only barrier calls as safe with any pointer while some internals still derive a header from the candidate address before proving heap membership.

Why this matters:

  • Embedders use barrier(cell) and barrierFrom(owner, cell) at ownership boundaries where legacy/raw pointers may appear.
  • The mid-script parallel-GC crash in zig-js showed that one unsafe header peek is enough to take down the collector under stress.
  • Backward compatibility is not important yet; the API contract should be made robust before consumers depend on it.

Acceptance criteria:

  • Audit writeBarrier, writeBarrierFrom, writeBarrierWeak, rememberStrongStore, incrementalBarrier, and nursery remembered-set paths for untrusted pointer inputs.
  • Decide which APIs require precise managed payloads and which are maybe-managed compatibility APIs.
  • For maybe-managed APIs, prove heap membership without dereferencing the candidate address first, or split out explicit safe/unsafe variants.
  • Add regression coverage with live heap pointers, stack/arena pointers, and deliberately wild aligned pointer values.
  • Keep the hot precise path available if needed, but make the public compatibility contract impossible to violate by accident.
9 days ago

Fixed in 6ed2ca8. writeBarrier(), writeBarrierFrom(), writeBarrierWeak(), rememberStrongStore(), and incrementalBarrier now use exact live-cell membership checks before reading candidate headers, so stack/arena/wild maybe-managed values are rejected without dereferencing them. Added regression coverage for stack and deliberately wild pointer values across nursery and incremental barrier paths.

Validation:

  • zig build test --summary none in zig-gc
  • zig-js focused integration: zig build threadfuzz-bin --summary none && ./zig-out/bin/threadfuzz midgcpromise 1 1
9 days ago

Closing as fixed by 6ed2ca8; follow-up CI is running from zig-js c2b5e61.

9 days ago

Follow-up pushed in 49c7d8f (perf(gc): index live payload lookups). The previous safety hardening kept maybe-managed barriers exact by walking the live cell list, but zig-js CI exposed that as too expensive: threadfuzz-amplified seed 1 timed out at the 120s per-seed watchdog while spending heavily in headerForPayload from write barriers. This commit keeps the exact live-payload semantics but adds a payload -> header index maintained with the all-list under alloc_lock, with fallback to the old walk if the index cannot grow.\n\nValidation:\n- zig-gc: zig build test --summary none\n- zig-js focused repro: ./zig-out/bin/threadfuzz amplify 1 1 now completes with 0 failures (~19s locally), where it previously hit the 120s watchdog.

Sign in to comment on this issue.