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] Raise error when entity aspect is missing in configuration specification. #1092

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

NikLeberg
Copy link
Contributor

Based on crashing fuzzer input 0c1d01dd6f3aea51f8b086c4c788132486f2bfe6079eb33e89f4e69d39945bc7 from issue #1091.

Previously nvc allowed entity aspect to be non existent even though for such explicit cases I think even the standard requires it to be existent.

LRM 19 section 7.3.2.1: When a binding indication is used in an explicit configuration specification, it is an error if the entity aspect is absent.

@nickg
Copy link
Owner

nickg commented Dec 9, 2024

I would check if bind != NULL in p_configuration_specification instead. The code would be simpler and the user gets a better error message:

tree_t bind = p_binding_indication(comp);
if (bind == NULL)
   parse_error(CURRENT_LOC, "a binding indication in an explicit configuration "
               "specification must contain an entity aspect");

@NikLeberg NikLeberg force-pushed the fix_binding_indication branch from f486d8e to f192004 Compare December 10, 2024 08:56
@NikLeberg
Copy link
Contributor Author

The else branch in p_binding_indication prevents that bind will ever be NULL for the case that we care about.

nvc/src/parse.c

Lines 9021 to 9022 in 4062b25

else
bind = tree_new(T_BINDING);

Otherwise that would be a nice and easy fix.

Up to now p_binding_indication returns three kinds of values:

  • for ent0 : ent; => shallow tree obj i.e. a simple tree_new(T_BINDING), nothing else
  • for ent0 : ent use entX; => a valid tree obj i.e. with location, reference, identifier etc.
  • for ent0 : ent use open; => simply NULL

I tried to work around this with the new commit. First I removed the mentioned else branch as it seemed to have no purpose. (All tests pass regardless.) This gives us NULL for the first and also the last input case. We then differentiate one from the other by checking if we have a tOPEN.

I'm not sure if that is acceptable. I sadly don't know the downstream changes that the removal of the else branch introduces.

@NikLeberg NikLeberg marked this pull request as draft December 10, 2024 10:12
@NikLeberg
Copy link
Contributor Author

Nevermind. The removal of the else branch most probably broke something called incremential binding. I'll come up with a MWE for that to see if it worked beforehand. If you happen to have a MWE pleaee share.

@NikLeberg NikLeberg force-pushed the fix_binding_indication branch from f192004 to 511a1d8 Compare December 12, 2024 01:23
@NikLeberg NikLeberg changed the title [Fuzz] Raise error when entity aspect is missing in binding indication. [Fuzz] Raise error when entity aspect is missing in configuration specification. Dec 12, 2024
@NikLeberg NikLeberg marked this pull request as ready for review December 12, 2024 01:24
@nickg nickg merged commit 06b3fbd into nickg:master Dec 12, 2024
12 checks passed
@NikLeberg NikLeberg deleted the fix_binding_indication branch December 12, 2024 16:27
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