Repository navigation
[BugFix]: fixing panic in evaluator when query from typeblock is unresolved - #432
Merged
Merged
Conversation
joshfried-aws
requested review from
Brianwithay21,
Neel-Renavikar,
brianlaoaws,
frankret,
minhphuong98,
razcloud,
reza-aws,
rpdutt,
setttings and
wkexiang
December 7, 2023 18:37
razcloud
approved these changes
Dec 7, 2023
brianlaoaws
approved these changes
Dec 8, 2023
awsmadi
added a commit
to awsmadi/cloudformation-guard
that referenced
this pull request
Aug 18, 2026
A regression this branch introduced, in the same place as a defect it inherited.
Found by differential against the merge-base, not by reading.
Moving a type block's conditions per-resource removed an early return that had
been masking an error path. A condition written resource-relative never passed
against the file root, so the block's query never ran; with the conditions inside
the loop it always runs, and against a document with no `Resources` at its root
it yields an `UnResolved` slot whose arm returned `Err`. An `Err` from one rule
aborts the whole rules file, so this happened:
rule unrelated_violation { Region == "eu-west-1" }
rule type_block_rule { AWS::EC2::Volume when Properties.Size > 10 { ... } }
merge-base: exit 19, unrelated_violation FAIL reported
this branch: exit 255, nothing reported
The violation was still found and then thrown away. The inherited half is that a
type block with no `when` reached the same `Err` on the merge-base, so the abort
predates this work -- the change only made it reachable for the guarded form.
Both are fixed by treating an unselectable slot the way the `values.is_empty()`
branch already treats an empty selection: the document does not contain the type,
so the block does not apply. `ur.reason` moves onto the record instead of into an
error, so the explanation still reaches the reader through `find_skip_reason`.
The fold now distinguishes its two SKIPs, because they mean different things to
whoever reads the output and naming the wrong one is worse than saying nothing:
nothing could be selected, or resources were selected and the condition exempted
all of them.
`test_with_payload_failing_type_block` changes expectation from INTERNAL_FAILURE
to VALIDATION_ERROR, and the reasoning is recorded on the test. It came from aws-cloudformation#432,
"fixing panic in evaluator when query from typeblock is unresolved", which
replaced `unreachable!()` -- a real panic on that fuzzed input -- with a recorded
failure and an `Err`. The exit code was the observable proof the panic was gone.
That property still holds; only the representation changed, from an abort to a
reported verdict. The assertion is deliberately not SUCCESS: a fuzzed rule
quietly accepted is the failure mode this branch exists to remove.
Why the earlier differential missed it: the corpus had one rule per file, and
with one rule an abort and a clean skip are indistinguishable by exit code. Adding
canary variants -- every rules file with an always-failing rule prepended -- is
what exposed it, and is worth keeping for the next sweep.
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.
Issue #, if available:
N/A
Description of changes:
We discovered another panic when running our fuzzer, this was when a query from a type_block is unresolved. Previously this was considered unreachable. Now instead of it being unreachable we return a
MissingValueerror, with some contest as to why this error occurred.We have also added an integration test to catch this.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license