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

Texture: prevent SIGILL in sampler validator. #8468

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KostyaAtG
Copy link
Contributor

Lambda's switch handled all valid enum values without consideration of malicious input.
However, if input is maliciously crafted, then it will lead to undefined behavior (function shall return something, but code does not define what has to be returned if switch body is skipped).

Moved validator close to the beginning of the method to validate before use and added missing return.

@KostyaAtG
Copy link
Contributor Author

@pixelflinger
Quick review identified few other places with similar patterns (switch over scoped enums, no return if switch body is skipped). To name a few:

  • DriverEnums.h 1, 2
  • Camera.h 1

Identification of all such places is tedious job, so I would recommend identifying such places automatically and patching them up to prevent undefined behavior in case of malicious input.

Clang18+ can be set up to detect missing "default" branches (-Wswitch-default), but even trunk version of clang does not detect absent return value in case of enums (-Wreturn-type).
https://godbolt.org/z/TKW9rE155

MSVC does better job and will report a warning C4715: not all control paths return a value in case of enums too.
https://godbolt.org/z/WzKGee8a7

@pixelflinger
Copy link
Collaborator

I don't want to add default to switch/case because then, if we add an enum, we don't get the warning/error in all the places where we might need to handle it.

Ideally we would never have a "default" in any switch/case, but instead always handle all the enum values

@KostyaAtG
Copy link
Contributor Author

I am not rooting for having defaults everywhere, I am rooting to have a function to return something if switch body is skipped at least in user input validation cases.

@poweifeng
Copy link
Contributor

poweifeng commented Feb 21, 2025

I am not rooting for having defaults everywhere, I am rooting to have a function to return something if switch body is skipped at least in user input validation cases.

Another possibility is to call FILAMENT_CHECK_POSTCONDITION outside of the swith. That way we wouldn't have to add a default (satisfying Mathia's point at compile time). And might be a way for a validation-only mechanism to intercept the assert (though I don't know about the customizability of FILAMENT_CHECK_POSTCONDITION today).

@KostyaAtG
Copy link
Contributor Author

From my perspective and use cases that I'm working on, If a function is participating in user input validation, it's better to return value then to panic. Panic cannot be handled, but user input validation API can be exposed to public to perform check beforehand to prevent potential panic in cases like FILAMENT_CHECK_PRECONDITION(userInputValidationAPI())

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.

3 participants