fix(TypeResolver): bound nodeIsTypeRef alias recursion (stack-overflow DoS)#5
Merged
Merged
Conversation
…w DoS) WHY: ziglint analyzes untrusted source. The nodeIsTypeRef / varDeclIsTypeRef / identifierIsTypeRef trio recursed with no depth guard, so a cyclic alias such as `const a = b; const b = a;` (or self-referential `const a = a;`) in any linted file caused unbounded recursion and a stack-overflow crash. PR #4's "harden resolver" added max_alias_depth=32 but only to findMethodInFileAsStructDepth, missing this sibling family. (Pre-existing upstream; introduced by upstream 6b11b20.) WHAT: Thread a `depth: u32` parameter through nodeIsTypeRef (via new nodeIsTypeRefDepth), varDeclIsTypeRef, identifierIsTypeRef, and fieldAccessIsTypeRef, incrementing on each recursive hop and returning false once depth >= max_alias_depth. Public isTypeRef() entry starts at depth 0. Mirrors the existing findMethodInFileAsStructDepth guard pattern. Added regression tests: cyclic alias, self-referential alias (both via isTypeRef), and a cyclic method alias (via findFnInCurrentModule, locking in the pre-existing guard). IMPACT: src/TypeResolver.zig only. No behavior change for non-cyclic input; the 32-hop cap is far beyond any real alias chain. Eliminates a remotely triggerable linter crash on adversarial source. VALIDATION: New tests crash (10001-frame stack overflow) before the fix and pass after. `zig build test --summary all` -> 7/7 steps, 277/277 tests pass, zig fmt clean, self-lint (run exe ziglint) clean. Found via multi-agent audit of the fork diff (bfcb30d..main) with adversarial verification.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughSummary by CodeRabbitRelease Notes
Walkthrough
ChangesCyclic Alias Recursion Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens TypeResolver.isTypeRef() against stack-overflow denial-of-service by bounding recursion when resolving cyclic const aliases in linted (untrusted) Zig source.
Changes:
- Introduces a depth-bounded
nodeIsTypeRefDepthhelper and threadsdepththrough the recursive type-ref checks (nodeIsTypeRef/varDeclIsTypeRef/identifierIsTypeRef/fieldAccessIsTypeRef). - Adds regression tests to ensure cyclic/self-referential aliases terminate without stack overflow, and to lock in existing alias-depth behavior for
findFnInCurrentModule.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1194
to
1201
| for (tree.rootDecls()) |decl_node| { | ||
| switch (tree.nodeTag(decl_node)) { | ||
| .simple_var_decl, .aligned_var_decl, .local_var_decl, .global_var_decl => { | ||
| const var_decl = tree.fullVarDecl(decl_node) orelse continue; | ||
| const name_token = var_decl.ast.mut_token + 1; | ||
| if (!std.mem.eql(u8, tree.tokenSlice(name_token), name)) continue; | ||
| return self.varDeclIsTypeRef(tree, decl_node, module_path); | ||
| return self.varDeclIsTypeRef(tree, decl_node, module_path, depth + 1); | ||
| }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TypeResolver.zig`:
- Line 1187: The depth tracking in the nodeIsTypeRefDepth function is
incrementing twice per single alias resolution step, which effectively halves
the max_alias_depth guard. In the recursive call at line 1187 where
nodeIsTypeRefDepth is invoked with depth + 1, and at the corresponding location
at line 1200, remove one of these increments so that a single alias hop
(identifier -> var_decl -> init) only consumes one depth increment instead of
two. This will allow the depth guard to properly protect against deep alias
chains without producing false negatives for legitimate acyclic alias sequences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 258e90e6-e272-419e-be6b-2a27cedcd2f0
📒 Files selected for processing (1)
src/TypeResolver.zig
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
EugOT/dotfiles(manual)
WHY: CodeRabbit and Copilot both flagged that the alias-recursion guard incremented depth twice per logical hop (identifierIsTypeRef -> varDeclIsTypeRef(depth+1) -> nodeIsTypeRefDepth(init, depth+1)), so max_alias_depth=32 effectively capped at ~16 alias links and could false-negative on long but acyclic alias chains. WHAT: identifierIsTypeRef now passes `depth` (not depth+1) into varDeclIsTypeRef; the single increment stays at the actual alias-following recursion in varDeclIsTypeRef. One alias link now costs exactly one increment, matching the intended "~32 hops". Termination is unchanged (every cycle still passes the incrementing call). IMPACT: src/TypeResolver.zig only. No safety change (still terminates, no overflow); removes a false-negative risk for deep legitimate alias chains. VALIDATION: zig build test --summary all -> 7/7 steps, 277/277 tests pass, fmt clean, self-lint clean. Cyclic/self-referential alias regression tests still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A deep multi-agent audit of the fork diff (
bfcb30d..main, the Zig 0.16 migration + hardening work from PRs #1–#4) surfaced one genuinely actionable defect, now fixed here.🔴 Critical: stack-overflow DoS on cyclic type aliases. The
nodeIsTypeRef/varDeclIsTypeRef/identifierIsTypeReftrio insrc/TypeResolver.zigrecursed with no depth guard. A cyclic alias such asconst a = b; const b = a;(or self-referentialconst a = a;) in any linted file triggers unbounded recursion → stack overflow → crash. Since ziglint analyzes untrusted source, this is remotely triggerable.PR #4's "harden resolver" added
max_alias_depth = 32but applied it only tofindMethodInFileAsStructDepth, missing this sibling family. The bug is pre-existing upstream (introduced by upstream6b11b20), so it's also a candidate upstream PR.Fix
Thread a
depth: u32parameter through the recursive trio (via a newnodeIsTypeRefDepth), plusfieldAccessIsTypeRef. Each recursive hop incrementsdepth; oncedepth >= max_alias_depththe chain returnsfalse. The publicisTypeRef()entry starts at depth 0. This mirrors the existingfindMethodInFileAsStructDepthguard pattern exactly. No behavior change for non-cyclic input (32 hops is far beyond any real alias chain).Tests (TDD)
Three regression tests added to
src/TypeResolver.zig:isTypeRef: cyclic alias terminates without stack overflow—const a = b; const b = a;isTypeRef: self-referential alias terminates without stack overflow—const a = a;findFnInCurrentModule: cyclic method alias terminates via max_alias_depth— locks in the pre-existing method-alias guardThe two
isTypeReftests crash with a 10001-frame stack overflow before the fix and pass after — proving they test the right thing.Validation
(274 baseline + 3 new tests.)
Audit notes
The audit ran 5 review dimensions (correctness, memory-safety, robustness, performance, testing) with adversarial verification of every finding. Everything else was either already-correct fork code (verifiers confirmed PRs #1–#4 fixed the bugs they flagged) or refuted false positives (arena-allocator non-leaks, bounded AST/path loops, non-material micro-perf). Two minor test gaps were noted; the higher-value one (the method-alias guard) is covered here. The remaining gap — a fault-injection test for
doc_tests.zig:203's infra-error rethrow — is low severity and intentionally deferred (would require fragile I/O mocking).