Skip to content

sha1dc: avoid misaligned uint32_t load when unaligned access is allowed - #7361

Open
afonsojanu wants to merge 1 commit into
libgit2:mainfrom
afonsojanu:fix/sha1dc-misaligned-load
Open

sha1dc: avoid misaligned uint32_t load when unaligned access is allowed#7361
afonsojanu wants to merge 1 commit into
libgit2:mainfrom
afonsojanu:fix/sha1dc-misaligned-load

Conversation

@afonsojanu

Copy link
Copy Markdown

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):

  • Before this change: UBSan reports "load of misaligned address ... for type 'const uint32_t', which requires 4 byte alignment" on every 64-byte block processed by SHA1DCUpdate when the source pointer is offset by one byte.
  • After this change: the same input produces no UBSan diagnostic.
  • In both cases, and in a build that forces the always-safe aligned path (SHA1DC_FORCE_ALIGNED_ACCESS), the resulting SHA-1 digest is identical, confirming the fix only removes the undefined behavior without changing any output.

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.

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.
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.

1 participant