Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fuzz] Fix crash with missing choices in aggregate. #1104

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

NikLeberg
Copy link
Contributor

Based on crashing fuzzer input da346642705230c6f2c89b7c0a522d1078c44b3d0dfd5fe52e2859212edd2784 from issue #1091.

The crash happens because simplify_local cannot find the missing choice in the aggregate. Bounds checking reports an error for it, but the check is only done afterwards. This adds a check for missing aggregate choices in eval_possible and forbids the early evaluation of such erroneous constructs.

Alternatively one could also do the bounds check before the local simplify and only if there were no errors, but I think that would change the code flow in a lot of places and tests.

Cheers

@NikLeberg NikLeberg force-pushed the fix_aggregate_choice branch from d39aecb to ac8778f Compare December 14, 2024 06:37
@nickg
Copy link
Owner

nickg commented Dec 17, 2024

Alternatively one could also do the bounds check before the local simplify and only if there were no errors, but I think that would change the code flow in a lot of places and tests.

The problem with that is some bounds errors can only be detected after you've folded locally static expressions. I agree the current order is not great either as it means invalid VHDL ends up in the code generator (this isn't the first case like this either).

Adding a defensive check here is fine. Perhaps a better organisation would be to merge the simplify and bounds checks passes rather than doing them one after another. I'll take a look at this.

@nickg nickg merged commit 78772e8 into nickg:master Dec 17, 2024
12 checks passed
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.

2 participants