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

Update XPACK.cc - initialize the table after allocation to avoid bad access #11301

Closed
wants to merge 1 commit into from

Conversation

shukitchan
Copy link
Contributor

Let's determine if we need this after fixing #11287 and whether fuzzing complains on other code path

@shukitchan shukitchan added this to the 10.1.0 milestone Apr 29, 2024
@shukitchan shukitchan self-assigned this Apr 29, 2024
@shukitchan
Copy link
Contributor Author

[approve ci autest]

@maskit maskit added the HTTP/2 label Apr 30, 2024
@maskit
Copy link
Member

maskit commented Apr 30, 2024

Let's say the fuzzer doesn't find anything at the moment. How do we know index numbers are checked appropriately at every place, after we make changes?

Also, the zeros set by memset are only available until actual entries overwrite them.

I still don't get the benefit of doing this.

@shukitchan
Copy link
Contributor Author

I think the actual benefit is that it is a precaution. if you forget to initialize an entry before using it accidentally, it will be less harmful.

@maskit
Copy link
Member

maskit commented Apr 30, 2024

if you forget to initialize an entry before using it accidentally, it will be less harmful.

Until the buffer is overwritten. An argument would be that we should zero-clear an entry when we remove it as well. That would make sense, but I'm not sure if it worth. I think letting the fuzzer find the errors, and/or having more unit tests is safer.

@shukitchan
Copy link
Contributor Author

Let's not worry about this for now till the fuzzer reports another problem related to this part.

@shukitchan shukitchan closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants