Revert XKCP alignment sanitizer suppression#46
Conversation
This reverts commit 47f5de5.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe build configuration simplifies C source compilation flags by removing the alignment sanitizer suppression flag from the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
build.zig (1)
126-126: Revert looks correct, but note the residual alignment risk.Removing the sanitizer suppression is the right call—masking UB isn't a fix. However, the underlying issue persists: the plain64 backend (
KeccakP-1600-opt64.c) performs unaligned 64-bit loads viau8* → uint64_t*casts. This affects:
- aarch64 builds (lines 138-142)
- x86_64 without AVX2/AVX512 (lines 159-163)
- All other architectures (lines 167-171)
On strict-alignment targets or when UBSan is enabled, this may cause runtime crashes or sanitizer failures. Consider tracking a follow-up issue to either:
- Patch the XKCP vendor to use
memcpy-based loads (compiler will optimize to unaligned load intrinsics)- Switch to a maintained XKCP fork that handles alignment properly
,
Would you like me to open a tracking issue for the proper XKCP alignment fix?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.zig` at line 126, The revert removed sanitizer suppression but left unsafe unaligned 64-bit loads in the XKCP backend (KeccakP-1600-opt64.c) that cast from u8* to uint64_t* and can crash on strict-alignment targets or under UBSan; open a tracking issue referencing KeccakP-1600-opt64.c and the problematic casts (unaligned u8* → uint64_t* loads), propose two remediation options (patch XKCP to use memcpy-based loads or switch to a maintained XKCP fork that fixes alignment), and document impacted targets (aarch64, x86_64 without AVX2/AVX512, others) so we can follow up rather than reintroduce sanitizer suppression in build.zig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@build.zig`:
- Line 126: The revert removed sanitizer suppression but left unsafe unaligned
64-bit loads in the XKCP backend (KeccakP-1600-opt64.c) that cast from u8* to
uint64_t* and can crash on strict-alignment targets or under UBSan; open a
tracking issue referencing KeccakP-1600-opt64.c and the problematic casts
(unaligned u8* → uint64_t* loads), propose two remediation options (patch XKCP
to use memcpy-based loads or switch to a maintained XKCP fork that fixes
alignment), and document impacted targets (aarch64, x86_64 without AVX2/AVX512,
others) so we can follow up rather than reintroduce sanitizer suppression in
build.zig.
1 participant
Summary
-fno-sanitize=alignmentto XKCP C flagsu8*touint64_t*without alignment guarantees) should be fixed properly — either by finding a better XKCP vendor or adding an aligned wrapperContext
The original commit was pushed directly to main by mistake. This PR reverts it properly.
Summary by CodeRabbit