Fix invalid pointer casts in arm64 implementation #65
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It is UB in C and C++ to type-pun pointers in this way, for two reasons. First, casting to a pointer is forbidden if the pointer is not aligned. Second, this kind of type-punning is a strict aliasing violation. Instead, the way to read 8 bytes as a time as a uint64_t is to call memcpy. Compilers recognize this pattern and optimize it, lowering it to the same code, but without breaking the language's abstract state machine. This is needed to fix some UBSan warnings in Chromium.
pwnall
requested changes
Sep 6, 2024
src/crc32c_arm64.cc
Outdated
| @@ -24,17 +25,31 @@ | |||
| #define KBYTES 1032 | |||
| #define SEGMENTBYTES 256 | |||
|
|
|||
| static uint64_t LoadU64(const uint8_t in[sizeof(uint64_t)]) { | |||
There was a problem hiding this comment.
Can you use and extend crc32c_read_le.h?
I think that ReadUint32LE() and ReadUint64LE() there work as you need them to, and we checked that they're compiled as efficiently as memcpy on little-endian architectures.
You can add a ReadUint16LE() with tests, and then use it here.
There was a problem hiding this comment.
Ah nice! Done. See commit message of 1b4eb1a for some notes on what I changed there.
In doing so, I fixed up crc32c_read_le.h a bit:
1. The documentation and tests say the buffers need to be aligned, but
they don't.
2. Add a 16-bit version.
3. Remove an unnecessary static_cast<uint8_t>. The pointer is already
uint8_t.
4. Replace the necessary static_casts with uintN_t{x} per the Google
style guide. https://google.github.io/styleguide/cppguide.html#Casting
pwnall
approved these changes
Sep 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is UB in C and C++ to type-pun pointers in this way, for two reasons. First, casting to a pointer is forbidden if the pointer is not aligned. Second, this kind of type-punning is a strict aliasing violation.
Instead, the way to read 8 bytes as a time as a uint64_t is to call memcpy. Compilers recognize this pattern and optimize it, lowering it to the same code, but without breaking the language's abstract state machine.
This is needed to fix some UBSan warnings in Chromium.