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.

Sign up for GitHub

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

Jump to bottom

[naga] PendingArraySize::Expression handle not validated #6793

Open
jimblandy opened this issue Dec 20, 2024 · 1 comment · May be fixed by #6800
Open

[naga] PendingArraySize::Expression handle not validated #6793

jimblandy opened this issue Dec 20, 2024 · 1 comment · May be fixed by #6800
Labels
area: naga middle-end Intermediate representation area: naga processing Passes over IR in the middle area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator

Comments

@jimblandy
Copy link
Member

Naga validation does not ensure that the expr handle in PendingArraySize::Expression(expr) is valid. That variant is not mentioned in naga/src/valid/handles.rs at all.

Fixing this is not as simple as just adding a bit of code to validate_module_handles. Handle validation needs to show that the structure of the module is acyclic, but with the introduction of PendingArraySize::Expression, Module::types and Module::global_expressions can now refer to each other, making it a bit trickier to show an absence of cycles.
@jimblandy jimblandy added area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator area: naga middle-end Intermediate representation area: naga processing Passes over IR in the middle labels Dec 20, 2024
@jimblandy jimblandy mentioned this issue Dec 20, 2024
Open
@jimblandy
Copy link
Member Author

cross-posting #6788 (comment):

I wonder if we can get away with saying that no expression can refer to an override-sized array and check this in the validator.

This would require not just checking the Handle<Type>s that appear in Expression variants, but the types those types refer to. But since override-sized arrays can't be used in many contexts (@kentslaney added the CREATION_RESOLVED flag to check this), we could just assume that they don't appear deeper in the type.

But one of the principles of the validator is that handle validation runs first, so that the rest of the validator can just assume that whatever handles it runs into are okay. We used to sort of check handles in the midst of everything else, and of course we forgot to do it all the time.

Being cycle-free, well, I'm not positive where this guarantee is taken advantage of. But assuming the absence of cycles where they might actually be present is an absolutely classic kind of bug, in every variety of programming, and I think this shows that people sort of generally don't anticipate cycles, whether they should know better or not. So I think it's good for handle validation to just settle the question once and for all, to allow rest of the validator to be naive and common-sensey.

And I think that means that we shouldn't be depending on type validation to establish the absence of cycles.

jimblandy added a commit to jimblandy/wgpu that referenced this issue Dec 21, 2024
@jimblandy
[naga] Forbid cycles between global expressions and types.
79d747d 
Update `valid::handles` to traverse `Module::types` and
`Module::global_expressions` in tandem, to ensure that the types and
expressions are acyclic.

Fixes gfx-rs#6793.
@jimblandy jimblandy linked a pull request Dec 21, 2024 that will close this issue
Open
jimblandy added a commit to jimblandy/wgpu that referenced this issue Dec 21, 2024
@jimblandy
[naga] Forbid cycles between global expressions and types.
75a5565 
Update `valid::handles` to traverse `Module::types` and
`Module::global_expressions` in tandem, to ensure that the types and
expressions are acyclic.

Fixes gfx-rs#6793.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area: naga middle-end Intermediate representation area: naga processing Passes over IR in the middle area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[naga] Forbid cycles between global expressions and types. jimblandy/wgpu
1 participant
@jimblandy