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

inflate/bitreader: fix unsoundness in advance() #242

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Nov 1, 2024

BitReader::advance() is unsound: if bytes is too large, we invoke UB in self.ptr.add(bytes) by advancing the pointer out of bounds of the allocated area. This doesn't happen in practice, since all uses of advance() are bounds checking with BitReader::bytes_available(), but we can make this API safer.

Use .wrapping_add() instead, which is safe for all inputs. The docs for it claim that .add() opens up more optimization opportunities. However, the generated assembly on x86_64 and aarch64 in release mode appear to be exactly the same, so I don't think we're losing out on anything here. (The guaranteed zero-cost way to fix this would be to just make advance() unsafe and document the invariant).

@folkertdev folkertdev merged commit 7d2d71d into trifectatechfoundation:main Nov 4, 2024
18 checks passed
@folkertdev
Copy link
Collaborator

yeah that's not worth the unsafe block then

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