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

bug: out-of-bounds read when using inline assembly code path #111

Closed
mayeut opened this issue Oct 23, 2022 · 3 comments
Closed

bug: out-of-bounds read when using inline assembly code path #111

mayeut opened this issue Oct 23, 2022 · 3 comments
Assignees
Labels

Comments

@mayeut
Copy link
Contributor

mayeut commented Oct 23, 2022

While upgrading base64 in a project I'm working on, valgrind started to report out-of-bounds read in the optimized encoding functions.
Those are now using the inline assembly code path instead of the intrinsics code path.

A draft PR with a reproducer will follow (I do not plan to fix this though).

@aklomp
Copy link
Owner

aklomp commented Oct 23, 2022

This is probably due to choosing the wrong cutoff value for the source length. I'll look into it later. Thanks for reporting.

@aklomp aklomp self-assigned this Oct 23, 2022
@aklomp
Copy link
Owner

aklomp commented Oct 23, 2022

Found the bug. It's in this calculation. Note how it differs (mistakenly) from the equivalent calculation in the non-asm code.

When calculating the number of rounds that the SIMD encoder can complete, we need to adjust for the fact that rounds consume 12 bytes of inputs, but the read size is 16 bytes. That's why we should subtract four bytes from the input size to ensure that a 16-byte read will not extend past the input buffer.

I'll fix and merge this after I review your pull request. The changes to CI in the pull request should validate the fix.

@aklomp
Copy link
Owner

aklomp commented Oct 23, 2022

The bug occurred because the first encoder to be converted to inline asm was the AVX2 encoder. That codec treats the first round specially by reading it at an offset of +0 bytes, while all following rounds are read at an offset of -4 bytes. This led me to believe that subtracting the four bytes only applied to that codec, and not to AVX and SSSE3, which is why I left it out from those codecs. I've verified locally that that fixes the valgrind warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants