ReviewOS

zig-utils/zig-js

Runtime: make StringCell allocation failures fallible instead of panicking

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

While working #27, a heap-limit probe using string doubling crashed the process instead of surfacing a catchable or host OOM:

thread panic: strcell.makeCell OOM
src/strcell.zig:236:35 in makeCell
src/value.zig:2226:62 in Value.str
src/interpreter.zig:13867:32 in evalBinary

Current shape:

let s = "x";
for (;;) s = s + s;

Under Context.Options.heap_limit_bytes, the string concatenation path eventually reaches strcell.makeCell, which catches allocator failure by panicking. That bypasses the interpreter's EvalError/error.Throw machinery and can abort a host process.

Acceptance:

  • Dynamic string creation has a fallible path that returns error.OutOfMemory instead of panicking.
  • Allocation-site OOM conversion from #27 can then make string allocation pressure catchable when a JS handler is active.
  • Static/builtin string constants can keep using infallible helpers where that is genuinely safe during bootstrap.
  • Add a focused heap-limit regression that exercises string allocation pressure without process abort.
9 days ago

Found while implementing 75a7e49 for #27. I intentionally did not mix this fix into that commit: the #27 patch stays focused on converting fallible capped allocator failures into catchable JS throws when a handler exists, while this issue tracks making dynamic string-cell creation itself fallible instead of panic-based.

9 days ago

Landed a focused #28 slice in e99dec7: https://github.com/zig-utils/zig-js/commit/e99dec7510233a1a63ccc8a072eda4101092d0c8

What changed:

  • Added fallible Value.strAlloc / Value.strOwned constructors backed by fallible strcell.createCell / new strcell.createCellOwned.
  • StringCell can now adopt already-owned dynamic bytes, avoiding the concat path's previous allocate-then-duplicate pattern.
  • Dynamic string + concatenation now uses the owned/fallible string-cell path.
  • Added a heap-limit regression where string doubling reaches OOM, is caught by JS, and throws a numeric sentinel instead of aborting the process with strcell.makeCell OOM.

Validation:

  • /Users/chris/.local/bin/zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='heap_limit_bytes allocation pressure' --summary all
  • git diff --check

Leaving this issue open because many dynamic string-producing paths still call infallible Value.str(...); this commit fixes the discovered concat/string-doubling crash path and adds the safer constructor for follow-up migration.

9 days ago

Landed another #28 migration slice in 238e202: https://github.com/zig-utils/zig-js/commit/238e20217e62478098bb368686ea1feb80c70f05

What changed:

  • Migrated obvious owned dynamic string buffers in common builtins to fallible string-cell construction (Value.strOwned / Value.strAlloc).
  • Covered string results from Function/RegExp/Date/Error/Object/TypedArray conversions and Uint8Array base64 output where the bytes are already dynamically produced.
  • This avoids the old infallible Value.str(...) path for those allocated strings and skips redundant byte duplication where the buffer can be adopted.

Validation:

  • /Users/chris/.local/bin/zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Function.prototype.toString returns source' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='RegExp flags and toString' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Date setters + string conversions' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Error prototypes' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='TypedArray toString shares' --summary all
  • git diff --check

Keeping #28 open: this is a safe owned-buffer cluster, but there are still dynamic string-producing Value.str(...) sites to migrate deliberately.

9 days ago

Landed another #28 owned-buffer migration in bf2f800: https://github.com/zig-utils/zig-js/commit/bf2f8008f8e2674ee13f45834cd0e16ff262a072

What changed:

  • Migrated the remaining obvious toOwnedSlice(self.arena) and self.arena.dupe(...) dynamic string results in src/interpreter.zig to Value.strOwned(...).
  • This covers Array/String/RegExp/Intl/Temporal/iterator-style paths where the bytes are already freshly allocated and can be adopted by the StringCell.
  • A follow-up search for the targeted owned-buffer patterns now has no matches.

Validation:

  • /Users/chris/.local/bin/zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Array.prototype generics on array-likes' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='String.prototype.split' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='String.prototype[Symbol.iterator]' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='RegExp.escape' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Intl.DateTimeFormat German numeric date pattern' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='TypedArray toLocaleString' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Temporal June 2024 removed API surface is absent' --summary all
  • git diff --check

Keeping #28 open because there are still dynamic Value.str(try ...) paths whose returned slices may be borrowed or need per-site ownership review rather than a blanket rewrite.

9 days ago

Landed another #28 helper-return migration in 59f6f08: https://github.com/zig-utils/zig-js/commit/59f6f081590f204d8e2f52daf4f9f7e3fb309682

What changed:

  • Migrated number/date/string/regexp helper-return string paths from infallible Value.str(try ...) to fallible Value.strAlloc(...).
  • This covers helpers that return []const u8 and may either allocate or return borrowed/static bytes, so copying into a fallible string cell is the safe per-site choice.

Validation:

  • /Users/chris/.local/bin/zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='Date setters + string conversions' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='String.prototype.split' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='String.prototype.trim' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='ToPrimitive: own valueOf/toString' --summary all
  • /Users/chris/.local/bin/zig build test -Dtest-filter='RegExp replace uses generic exec' --summary all
  • git diff --check

#28 remains open for the remaining reviewed sites (for example BigInt/Intl range/Temporal calendar-offset helpers and a few alternate-allocator replacement buffers).

9 days ago

Landed final pattern-removal slice 7b6290e: https://github.com/zig-utils/zig-js/commit/7b6290e82b7ca41bf109c13644e8f9196369fec2

What changed:

  • Removed the remaining panic-prone Value.str(try ...) call shape in src/interpreter.zig.
  • Migrated replace buffers, Symbol/BigInt conversions, Intl range/unit strings, string iterator conversion, RegExp source escaping, and Temporal month/offset helper strings to fallible Value.strOwned / Value.strAlloc paths.
  • Confirmed there are no matches for Value.str(try in src/interpreter.zig, src/context.zig, or src/value.zig.

Checks run:

  • zig fmt src/interpreter.zig
  • zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • Smoke filters for Symbol.prototype.toString, BigInt oversized parsing, RegExp source/toString/flags, String iterator, Intl.DateTimeFormat German numeric date pattern, and Temporal BCE round trips
  • git diff --check

This closes the specific Value.str(try ...) panic class. I am leaving the issue open for the broader audit of non-literal Value.str(...) calls, since some borrowed/dynamic-string sites may still need explicit fallible cell construction under heap pressure.

9 days ago

Landed broader runtime-string constructor contract slice 12162ef: https://github.com/zig-utils/zig-js/commit/12162eff158305fcbeed00c558ccb209d304f025

What changed:

  • Made Value.str a compile-time/static string constructor, so literal strings use static cells and cannot hit allocator OOM.
  • Routed runtime string slices through fallible Value.strAlloc / Value.strOwned across interpreter, builtins, C API, compiler, VM, structured clone, thread helpers, and synthetic module paths.
  • Made Interpreter.keyToValue fallible so proxy/property-key trap arguments propagate OutOfMemory instead of using the old panic path.
  • Added Value.strCell for the NaN-box bridge to decode an existing StringCell without allocating.
  • Updated stale strcell/valuebox/value_nb proof comments/tests to match the new static-vs-runtime split.
  • Static scan now reports no non-literal Value.str(...) calls under src/*.zig.

Checks run:

  • zig fmt on touched files
  • git diff --check
  • Static scan for non-literal Value.str(...)
  • zig build test -Dtest-filter='heap_limit_bytes string allocation pressure' --summary all
  • zig build test -Dtest-filter='valuebox' --summary all
  • zig build test -Dtest-filter='value_nb' --summary all

I also started a full zig build test --summary all, but stopped it after ~2 minutes with no output to avoid turning this into an unbounded test run. The focused compile/test batch above is green.

9 days ago

Closing as implemented.

The latest slice 12162ef changes the contract so Value.str is static-only and runtime strings must go through fallible Value.strAlloc / Value.strOwned. The earlier #28 slices migrated owned/helper string paths and added the heap-limit regression.

Acceptance check:

  • Dynamic string creation now has fallible constructors and the main runtime sites have been migrated.
  • The heap-limit string pressure regression catches OOM instead of aborting.
  • Static/builtin constants use static cells through Value.str("literal") and no allocator.
  • Static scan reports no non-literal Value.str(...) calls under src/*.zig.

Latest focused validation is green; CI for the final pushed commit is queued.

Sign in to comment on this issue.