fix: cap AttrValue nesting depth to prevent stack overflow on deeply nested config - #1216
Draft
gominimal-aw-bot[bot] wants to merge 1 commit into
Draft
Conversation
Deeply nested config (lists, maps, or enum variants ~100+ levels deep) aborted the process with an uncatchable stack overflow: AttrValue::from_term recursed with no depth limit, and evaluation forces one level of nesting per recursive call. Cap construction at 128 levels and return a structured Error::AttrTooDeep past the cap, turning the abort into a recoverable error. Add a regression test asserting the structured error rather than an abort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABMrkm-g
Evaluating a layer whose config nests lists, maps, or enum variants ~100+ levels deep aborted the entire process with an uncatchable
fatal runtime error: stack overflow.AttrValue::from_termin thedecodecrate recursed through theList,Map, andEnumVariantcases with no depth limit, and Nickel forces one level of evaluation per recursive call, so deep nesting exhausted the stack before any error could be returned.This caps attribute-value construction at 128 nesting levels and returns a new structured
Error::AttrTooDeeponce the cap is exceeded, turning hostile or accidental deep nesting into a recoverable error instead of a crash. A regression test drives evaluation past the cap on a large-stack worker thread and asserts the structured error rather than an abort. (The TOML argument path was also considered but is unaffected: thetomlcrate enforces its own nesting limit and returns a structured parse error.)Verification
cargo fmt --all --check— clean, no diffcargo clippy --workspace --locked -- -D warnings— exit 0, no warningscargo build --workspace --locked— exit 0cargo test --workspace --locked— exit 0; decode: 57 passed, 0 failed (incl.nesting_past_cap_errors_instead_of_overflowing)Note
Cap
AttrValuenesting depth at 128 to prevent stack overflow on deeply nested configDeeply nested Nickel attribute values could cause unbounded recursion in
AttrValue::from_term, leading to a stack overflow. AMAX_ATTR_DEPTHconstant (128) is introduced in attrs.rs, and recursion is tracked via a newfrom_term_athelper that returnsErr(Error::AttrTooDeep { max_depth: 128 })when the limit is exceeded. The new error variant is formatted and reported with a human-readable message in error.rs.Macroscope summarized 97603c1.