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

Add out-of-bounds check #2380

Closed
wants to merge 1 commit into from

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Sep 12, 2024

The size of the buffer from size is used as an array index. This value could be one larger than the last possible index of the array, causing a buffer overread.

@CLAassistant
Copy link

CLAassistant commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

@tygyh tygyh force-pushed the Prevent-buffer-overread branch from 45a490c to 32384a3 Compare September 12, 2024 16:52
@tygyh tygyh force-pushed the Prevent-buffer-overread branch from 32384a3 to 9ab1b3c Compare September 12, 2024 16:57
@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Sep 18, 2024

When is this case ever hit? Does it fix a concrete bug?

@tygyh
Copy link
Contributor Author

tygyh commented Sep 18, 2024

Does it fix a concrete bug?

None which I know of. I tried to fix this since this project is a dependency for a project I'm working on and I wanted to fix it at the root.

@HansKristian-Work
Copy link
Contributor

None which I know of. I tried to fix this since this project is a dependency for a project I'm working on and I wanted to fix it at the root.

I'm not comfortable adding arbitrary code like this when it doesn't solve a concrete problem in SPIRV-Cross. An assert() for being in-range would be acceptable, but actual range-checking should be done on the outside.

@HansKristian-Work
Copy link
Contributor

Closing as not being relevant for upstream.

@tygyh tygyh deleted the Prevent-buffer-overread branch November 12, 2024 07:41
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