Skip to content

fix(cfg): replay errdefers on return err from catch/else captures (closes #17)#24

Merged
ericsssan merged 13 commits into
mainfrom
fix/errdefer-at-return
Jun 11, 2026
Merged

fix(cfg): replay errdefers on return err from catch/else captures (closes #17)#24
ericsssan merged 13 commits into
mainfrom
fix/errdefer-at-return

Conversation

@ericsssan

Copy link
Copy Markdown
Owner

Summary

Fixes the errdefer-at-return false negative tracked in #17.

Zig fires errdefer bodies only on returns that yield an error value. lowerReturn previously recognised just the literal return error.X shape, so a return err re-raising a captured error from catch |err| / else |err| was treated as a success return — and the errdefer body was silently dropped. That missed the textbook double-free shape:

const buf = try allocator.alloc(u8, n);
errdefer allocator.free(buf);
something(buf) catch |err| {
    allocator.free(buf);   // explicit free
    return err;            // errdefer ALSO frees buf -> double free, was MISSED
};

What changed

  • src/flow/cfg.zig — added is_error_capture to LocalInfo; corrected the stale header that still listed try/catch as unmodeled (they are modeled), moving the real remaining gap (error-union returns) to the "don't model" list with a soundness note.
  • src/flow/cfg_builder.zig — tag catch |err| / else |err| captures as error values, routed through the single emitCatchFork chokepoint so statement / var-decl / assign / return catch positions all benefit. New returnExprIsError recognises both literal error.X and a returned error-capture identifier; lowerReturn uses it to choose the error-path flush.
  • src/flow/cfg_tests.zig — two tests (see below).

Soundness boundary (intentional)

Error-union returns (return foo() where foo returns !T) are not classified here. They may or may not be an error at runtime; flushing errdefers unconditionally would false-positive on the success path, and deciding them needs callee return-type resolution plus a success/error path fork. This is left as a documented, sound false negative (never a false positive). Flushing errdefers at a known-error return — which is all this PR does — is always correct.

Tests

  • errdefer DOES fire on return err from a catch capture — asserts the errdefer (arena.deinit().arena_kill) is replayed exactly once on the error-return path.
  • errdefer does NOT fire on a non-error return from a catch arm — FP guard: return 0 from a catch arm replays nothing.

Verified meaningful: with the fix neutered, the first test fails (expected 1, found 0) while the companion stays green. zig build + zig build test (3458 tests) pass.

Closes #17

ericsssan added 13 commits June 11, 2026 10:38
…Type

Two latent type-engine bugs that silently disabled semantic suppressions:

1. Relative-path @import resolution: Uri.fromPath turned a relative file_path
   (what `find | xargs zbc` produces every sweep, and the common CLI form) into
   file:///<rel> -> filesystem root -> imported files never loaded -> cross-file
   types (struct fields, aliases) silently unresolved. The analyzed file worked
   (in-memory source), so absolute-path single-file tests passed while sweeps ran
   token-only for cross-file facts. Fix: canonicalize via libc realpath(3) before
   fromPath (std.fs.path.resolve does not prepend CWD in this Zig dev version).

2. typeNameOfNode returns only container names, never primitives, so the float
   check it backed was dead. Added TypeResolver.isFloatType (InternPool
   simple_type f16/f32/f64/f80/f128/c_longdouble) + FileCache.isFloatType wrapper.
operandIsFloat used typeNameOfNode (container-only) to string-match 'f64', so it
never matched any primitive -> the float suppression was dead. Switch to the new
FileCache.isFloatType. Now `(start_y + end_y) / 2` where operands are f64 locals
inferred through cross-file field arithmetic (ghostty font/face.zig aligned_y)
is correctly suppressed; integer midpoints (bun md/unicode.zig usize) still fire.

ghostty 134->128, mach 130->129; bun unchanged; all tests pass; known TPs preserved.
intInfo only matched InternPool .int_type, but usize/isize/c_short/c_int/...
are simple_types, so it returned null for the most common integer type (usize).
Every unsigned-operand query silently failed — notably the value-range oracle's
`i > usize_var ⟹ i != 0` generalization (operandProvablyNonneg never confirmed
usize as unsigned). Delegate to the canonical InternPool.intInfo(idx, target),
gated on zigTypeTag==.int (it is unreachable on non-integers). Resolves usize/
isize via ptrBitWidth and c_* via cTypeBitSize, plus packed-struct/enum widths.
analyzeAssign/analyzeCompoundAdd only checked the RHS for the use token, so a
provably-nonzero index on the WRITE side (`buf[idx - 1] = 0`) was never resolved
— the index-minus-one oracle suppression silently didn't apply to assignment
statements, only to reads/returns. Check the LHS first (the index is read before
the store, so the current state is correct). 3 oracle tests incl. no-FN guard.

Combined with the usize intInfo fix, the unsigned-`>` generalization now works on
real shift/insert/swap loops: bun 777->773, ghostty 128->127, Ez 22->19; the
vendored MultiArrayList `while (i > index) field_slice[i] = field_slice[i-1]`
FP is gone. Unguarded writes and signed-RHS guards still fire (sound).
The oracle refined guards only in `if`-conditions, so the same-expression
last-element idioms `c.len > 0 and c[c.len-1]` and `c.len == 0 or c[c.len-1]`
(endsWith/startsWith) were unproven by the sound layer. Added bool_and/bool_or
handling: in `A and B` the RHS sees A's THEN-facts; in `A or B` the RHS sees
A's ELSE-facts. `return` now descends into its operand so the refinement
applies. Generalizes beyond the token heuristic's fixed window (handles
arbitrarily-nested uses in the RHS). 3 tests incl. no-FN guard.

Corpus delta 0 (token hasAndGuard/hasOrGuard already cover the same-line cases),
but subsumes them with a sound, position-independent fact — the project's
token-proxy -> semantic-fact direction. All tests pass.
… facts

Two oracle completeness gaps that left provable last-element/index accesses
firing:

1. Use inside the loop CONDITION (`while (tmp > beg and self.text[tmp-1]==…)`)
   was answered directly without descending — so the `and`'s short-circuit
   refinement never applied. Now descends via analyzeNode (handles bool_and/or;
   plain comparisons still fall through to answerAt). Clears md/blocks.zig
   trim-loops.

2. A loop var that is nonzero before the loop and ONLY `+=`-mutated inside stays
   nonzero on every iteration (unsigned `+=` can't wrap to 0 on a panic-on-
   overflow build). dropLoopMutated now keeps such monotonic scalars instead of
   dropping them. Clears `var p: usize = 1; while (…) { …a[p-1]…; p += 1; }`
   (diff_match_patch). Sound: a `=`/`-=` reset still drops the fact (2 no-FN
   unit tests; verified the genuine TP diff_match_patch:1030 still fires).

bun 773->757, Ez 19->18 (−17 FPs). All tests pass.
A comptime-known index can never underflow/OOB at RUNTIME: `CONST - 1` with
CONST==0, or an out-of-range constant subscript, is a COMPILE error that never
ships. Added TypeResolver.comptimeIntValue (via the now-public Analyser.
resolveIntegerLiteral) + FileCache.comptimeIntValueOf; Form A suppresses when
the index operand resolves to a comptime value. Covers `bytes[max_inline_len-1]`
(max_inline_len a const usize=8) across the corpus.

bun 757->749 (−8). Runtime indices still fire (verified); all 5 genuine TPs
preserved. All tests pass.
A local `const s = base[0..N]` has `s.len == N` (start fixed at 0), the same
relation as a `.len` snapshot — record it as a {scalar:N, container:s} alias so
a nonzero N (e.g. after `if (N == 0) return`) proves s non-empty, making
`s[s.len-1]` safe. Immediate propagation when N is already nonzero at the decl.

bun 749->748 (PackageInstall.zig: `cache_path = buf2[0..cache_path_length]`
guarded by an earlier length==0 return). 2 tests incl. no-FN (unguarded N stays
unproven). TPs preserved; all tests pass.
Builds the cross-fn lever requested. `const m = callee(arg)` where an in-file
single-return `callee` provably returns non-zero given a non-zero argument, and
`arg` is proven non-zero, marks `m` non-zero.

Pieces (reusing fn_summary.singleReturnExpr for return extraction):
- ValueAlias on State: `const L = self.field` records local==path, so a guard on
  the simple local (`if (bit_length==0) return`) proves the field-path argument
  (`numMasks(self.bit_length)`) non-zero. Dropped on rebind / path-root mutation.
- Cross-fn postcondition: callee's single return is non-zero given param non-zero,
  via exprNonzeroPostcond (positive literal, the param, unsigned add, ceil-div).
- Ceil-div `(base + addend)/divisor` proven non-zero when base non-zero and
  addend >= divisor-1 — two ways: comptime values, OR STRUCTURAL (addend is
  syntactically `divisor - 1`). Structural is essential: the real idiom is
  `(x + (@bitSizeOf(MaskInt) - 1)) / @bitSizeOf(MaskInt)` and the engine does not
  evaluate @bitSizeOf, but addend==divisor-1 holds for any value.

bun 748->744: bit_set.zig toggleSet/setAll/etc. `self.masks[numMasks(self.bit_length)-1]`
(4 findings). Sound: unguarded arg still fires; all 5 genuine TPs preserved;
3 new oracle tests (incl. no-FN). All tests pass.
Re-evaluation (rigorous bucket triage) found slice-from-fixed-offset was ~90% FP,
NOT TP-dominated as I'd claimed — the driver is the wyhash hash-tail idiom
`rem_key = b[0..rem_len]; switch (rem_len) { K => rem_key[N..] }`: in arm K,
rem_len == K so rem_key.len == K, and rem_key[N..] is safe when K >= N. The engine
modeled no switch-arm facts at all.

Added:
- switch-arm exact-value facts: `switch (scalar) { K => … }` records scalar == K
  (a ScalarVal) and scalar != 0, in that arm. analyzeSwitch in the oracle walk
  (also fixes that switch bodies were previously unanalyzed entirely).
- provesMinLen(target, n): target.len >= n via a slice-length alias
  (target.len == scalar) whose scalar has a known value >= n.
- Wired into slice-from-fixed-offset (`buf[off..]` safe when minlen >= off).

bun 744->677 (−67), ghostty 127->126. Sound: arm K < N still fires, no-switch
still fires (3 no-FN unit tests); genuine TPs (css_modules name[2..],
lower_decorators) preserved. All tests pass.
…ists

The bug is a LOCAL ArrayList with capacity > len returning .items (excess
capacity orphaned — caller frees items.len, leaks the rest; bun#23885). When recv
is a POINTER parameter (`out: *std.ArrayList(T)`) the caller owns the list it
passed and will deinit it, so returning .items is an intentional borrow into the
caller's own allocation — no orphaned buffer. Only pointer params qualify: a
by-value param could leak a post-append realloc, so it still fires.

Finds the enclosing fn proto for the return and checks recv's param type is a
pointer. bun 677->674. 2 tests (pointer-param suppressed; by-value still fires);
local lists still fire. All tests pass.
….args

The first `.next()` of a `std.process.args()` / `argsWithAllocator()` ArgIterator
returns argv[0] (the program name), which is always present, so `.?` cannot panic
— same guarantee the rule already uses for `split*`. Extended the first-next
constructor check to ArgIterator (bare `args` gated on a `process.` receiver to
avoid matching unrelated `.args` fields). Subsequent `.next().?` still fires.
tigerbeetle 46->43. 2 tests (first suppressed, second fires).
Zig fires errdefers only on error returns. lowerReturn previously
recognised only literal `return error.X`, so a `return err` re-raising
a captured error from `catch |err|` / `else |err|` was treated as a
success return and the errdefer body was dropped — missing the
textbook errdefer-double-free-on-error-return shape (false negative).

Tag catch/else error captures as `is_error_capture` (via the single
emitCatchFork chokepoint, covering statement/decl/assign/return
positions) and recognise a returned error-capture identifier in a new
`returnExprIsError`. Flushing errdefers at a known-error return is
sound — never a false positive.

Error-UNION returns (`return foo()` yielding `!T`) are deliberately not
classified: that needs callee return-type resolution + a path fork to
stay FP-free. Documented as the remaining sound false-negative; the
stale cfg.zig header is corrected to match.

Tests: errdefer DOES replay on `return err` from a catch; does NOT on a
non-error `return 0` from a catch arm (FP guard).

Closes #17
@ericsssan
ericsssan merged commit aa3d7d9 into main Jun 11, 2026
1 check passed
@ericsssan
ericsssan deleted the fix/errdefer-at-return branch June 11, 2026 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CFG: errdefer not replayed at plain return — error-path false negative (+ stale try/catch doc)

1 participant