Bound extract_from_meta's descent into nested rain meta documents - #288
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe CLI adds ChangesMetadata nesting validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR bounds nested metadata traversal and returns an error instead of allowing excessive nesting to exhaust the stack, while preserving supported nesting up to the documented limit. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.97.1)Clippy execution timed out Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
1 participant
Closes #226.
The defect, confirmed
OrderBuilderStateV1::extract_from_metarecursed into any item carryingKnownMagic::RainMetaDocumentV1with nothing bounding the descent. Its input ismetaboard bytes, which
IDescribedByMetaV1states come from emitters that areuntrusted and "NOT expected to even be aware of the contract", so the nesting
depth of that input is chosen by whoever wrote the meta. Depth N drives N
frames; the payload grows only ~25 bytes per level, so a few hundred kilobytes
reaches thousands of levels and the process aborts on a stack overflow instead
of returning
Err.Confirmed on this branch by mutating the bound back out (
remaining_depthfedusize::MAX) and rerunning the new tests:With the bound in place the same 18 tests pass.
What changed
crates/cli/src/meta/types/dotrain/order_builder_state_v1.rsextract_from_metais now a thin entry point that seeds a depth budget ofMAX_NESTED_DOCUMENT_DEPTH(32) into a privateextract_from_meta_within,which carries the recursion. Each descent into a nested document spends one unit
of budget through
checked_sub(1); spending the last one yieldsError::MetaNestingTooDeep. The public signature, and the behaviour for everyinput at or under the bound, are unchanged.
Nesting stays a first-class shape, per #281. #226 is about the unbounded
descent, not the descent itself, so the fix bounds it rather than removing it:
the pre-existing
test_extract_from_meta_nested_rain_documentand the codec'stest_document_magic_item_carries_a_nested_documentboth still pass untouched,and a new test walks 32 full levels of nesting to an instance and finds it.
32 is far above anything the "meta about other meta" graph asks for in practice
(the live shapes are one or two levels) and far below any depth that troubles a
stack.
crates/cli/src/error/mod.rsNew
MetaNestingTooDeep(usize)variant. It carries the bound it was measuredagainst so the rendered message cannot drift from the constant:
"nested meta documents deeper than 32 levels". Display is pinned by a testalongside the existing Display tests.
Tests
Three new tests in
order_builder_state_v1.rs, plus one inerror/mod.rs:test_extract_from_meta_at_the_nesting_bound— an instance under exactlyMAX_NESTED_DOCUMENT_DEPTHlevels of nesting is still found. Legitimatenesting keeps working right up to the edge.
test_extract_from_meta_past_the_nesting_bound— one level further isErr(MetaNestingTooDeep(32)), and the variant carries the bound.test_extract_from_meta_deep_nesting_does_not_exhaust_the_stack— the DoSitself. 5000 levels, run on a 512 KiB stack that could not hold a fraction of
the frames the unbounded walk would need, and the call must return an error
rather than abort. This is the test that aborts the whole binary if the bound
is ever removed again, which is the point: a stack overflow is not a catchable
panic.
test_display_meta_nesting_too_deep_carries_the_bound.QA
test_extract_from_meta_past_the_nesting_bound,test_extract_from_meta_deep_nesting_does_not_exhaust_the_stack— each fails on base (base's behaviour reproduced in-tree by seeding the budget withusize::MAX, which restores the unbounded recursion exactly: the first FAILED, the second killed the whole binary withthread '<unknown>' has overflowed its stack / fatal runtime error: stack overflow, aborting, SIGABRT). The other two cannot fail on base and are pinned by mutation instead:test_extract_from_meta_at_the_nesting_boundPASSES on base — it is the no-regression guard that the fix does not delete legitimate nesting (The document magic as an item magic is a nested document, not a defect #281) — andtest_display_meta_nesting_too_deep_carries_the_boundnames a variant base does not have.extract_from_metabudget seedMAX_NESTED_DOCUMENT_DEPTH→usize::MAX(base behaviour) → killed bytest_extract_from_meta_past_the_nesting_bound(FAILED) and bytest_extract_from_meta_deep_nesting_does_not_exhaust_the_stack(SIGABRT); same seed →MAX_NESTED_DOCUMENT_DEPTH - 1(off-by-one) → killed bytest_extract_from_meta_at_the_nesting_boundalone, the other 17 still pass, so the at-bound test is not vacuous;Displayarm text"... {} levels"→"... {} tiers"→ killed bytest_display_meta_nesting_too_deep_carries_the_bound.IDescribedByMetaV1states meta emitters are untrusted and "NOT expected to even be aware of the contract", which is what makes the depth attacker-chosen; The document magic as an item magic is a nested document, not a defect #281 adjudicated that aRainMetaDocumentV1magic on an item means a nested document, which fixes what "legitimate nesting" means and therefore that the fix must bound the descent rather than remove it. Expected outcomes derived from that: an instance at any depth within the bound is still found; past the bound the call returnsErr; a deep hostile chain returns rather than aborts. The stack-overflow evidence is the runtime's own abort message, not a value read off the code.Errinstead, (c) triage note that a depth bound may be the answer, (d) implicit from The document magic as an item magic is a nested document, not a defect #281 and the task framing, legitimate nesting must keep working. Covered a (..._deep_nesting_does_not_exhaust_the_stack, 5000 levels on a 512 KiB stack), b (..._past_the_nesting_boundassertsErr(MetaNestingTooDeep(32))), c (the bound is the fix), d (..._at_the_nesting_boundwalks 32 full levels to an instance; pre-existingtest_extract_from_meta_nested_rain_documentand The document magic as an item magic is a nested document, not a defect #281'smeta::tests::test_document_magic_item_carries_a_nested_documentuntouched and still passing).Also clean locally:
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings, 18 pass inorder_builder_state_v1, 7 inerror, 2 inmeta::tests::test_document_magic_*.🤖 Generated with Claude Code
Summary by CodeRabbit