sha1dc: avoid misaligned uint32_t load when unaligned access is allowed - #7361
Open
afonsojanu wants to merge 1 commit into
Open
sha1dc: avoid misaligned uint32_t load when unaligned access is allowed#7361afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
SHA1DCUpdate's whole-block loop directly casts the caller-supplied input pointer to uint32_t* and dereferences it when SHA1DC_ALLOW_UNALIGNED_ACCESS is defined, which is the case by default on any x86/x86_64 build. That pointer's alignment is only known to the caller, so whenever a hashed buffer (for example a git object whose header length isn't a multiple of four) leaves it at a non-4-byte-aligned address, this reads a uint32_t through a misaligned pointer, which is undefined behavior in C and is flagged by UBSan's alignment check. Only take the direct-cast fast path when the buffer is actually 4-byte aligned, falling back to the existing memcpy-into-the-context path otherwise. Verified with a standalone harness built with -fsanitize=undefined,alignment and -DSHA1DC_FORCE_UNALIGNED_ACCESS: the misaligned load is reported before this change and gone after, with the resulting hash unchanged in both cases and matching a build that forces the always-safe aligned path. Added a regression test asserting that hashing the same bytes at an odd alignment produces the same digest as hashing them aligned.
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
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.
SHA1DCUpdate's whole-block loop casts the caller-supplied input pointer directly to uint32_t* and dereferences it whenever SHA1DC_ALLOW_UNALIGNED_ACCESS is defined, which happens by default on any x86/x86_64 build (see the SHA1DC_ON_INTEL_LIKE_PROCESSOR detection earlier in the file). That pointer's alignment is only known to the caller of git_hash_update/git_hash_buf, so whenever a hashed buffer is left at a non 4-byte-aligned address (for example, an object whose header length isn't a multiple of four, hashed together with its content in one call), this line reads a uint32_t through a misaligned pointer, which is undefined behavior in C and gets flagged by UBSan's alignment check.
The fix only takes the direct-cast fast path when the buffer is actually 4-byte aligned, falling back to the existing memcpy-into-the-context-buffer path otherwise, exactly like the code already does for the very first partial block.
Verification: since ordinary x86/arm64 hardware tolerates unaligned loads at the instruction level, this bug doesn't corrupt hash output on typical hardware/optimization levels, so a plain output comparison can't detect it. I verified it with a standalone harness built with -fsanitize=undefined,alignment (and -DSHA1DC_FORCE_UNALIGNED_ACCESS on this arm64 machine, to force the same code path that's active by default on x86_64):
Also added a regression test in tests/libgit2/object/raw/hash.c asserting that hashing the same bytes through a 4-byte-aligned buffer and through a deliberately misaligned buffer produces the same digest. The full test suite (libgit2_tests) passes locally.