Conversation
HashBytes passed a size_t length straight to the vendored MurmurHash3_x86_32, whose length parameter is a signed 32-bit int. For inputs of 2^31 bytes or more the length wrapped negative, making the hash loop read far out of bounds (and nblocks * 4 signed-overflow UB); for inputs above 2^32 bytes a silently wrong prefix was hashed, breaking the bucket contract against other implementations. Every literal hash path (kDecimal/kString/kUuid/kBinary/kFixed) funnels through this one function, so guard it there: reject lengths above INT32_MAX with ICEBERG_CHECK_OR_DIE before the narrowing cast. The bound matches the limit implied by iceberg-java's byte arrays, so no legal bucket input is rejected. The vendored murmurhash3_internal.cc is left untouched (tagged third-party code) and the now-checked narrowing is made explicit with static_cast<int>. Add BucketUtilsTest.HashBytesRejectsOversizedInput, which fabricates an oversized span over a 1-byte buffer (never dereferenced, so no 2 GiB allocation is needed in CI). Without the guard the test segfaults.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new regression test constructs a std::span with a count far beyond the referenced object size, which is undefined behavior even if the span is never dereferenced.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR hardens BucketUtils::HashBytes by rejecting oversized byte inputs before calling the vendored MurmurHash3_x86_32 function, preventing signed-length wraparound that could otherwise lead to out-of-bounds reads and incorrect bucket hashes for very large inputs.
Changes:
- Add a size guard in
BucketUtils::HashBytesto reject inputs larger thanINT32_MAXbefore narrowing to theintlength parameter required byMurmurHash3_x86_32. - Make the narrowing cast explicit (
static_cast<int>(bytes.size())) after the guard. - Add a regression test intended to ensure oversized inputs are rejected without requiring a multi-GB allocation.
| File | Description |
|---|---|
| src/iceberg/util/bucket_util.cc | Adds an explicit maximum-length guard before calling MurmurHash3 with an int length. |
| src/iceberg/test/bucket_util_test.cc | Adds a regression test for rejecting inputs larger than INT32_MAX. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+120
to
+124
| const uint8_t byte = 0; | ||
| const auto oversized_length = | ||
| static_cast<size_t>(std::numeric_limits<int32_t>::max()) + 1; | ||
| std::span<const uint8_t> oversized(&byte, oversized_length); | ||
| ASSERT_THAT([&]() { BucketUtils::HashBytes(oversized); }, |
Collaborator
|
The fix looks correct to me, the UB reported by copilot should be resolved though, thanks. |
This branch has not been deployed
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.
HashBytes passed a size_t length straight to the vendored MurmurHash3_x86_32, whose length parameter is a signed 32-bit int. For inputs of 2^31 bytes or more the length wrapped negative, making the hash loop read far out of bounds (and nblocks * 4 signed-overflow UB); for inputs above 2^32 bytes a silently wrong prefix was hashed, breaking the bucket contract against other implementations.
Every literal hash path (kDecimal/kString/kUuid/kBinary/kFixed) funnels through this one function, so guard it there: reject lengths above INT32_MAX with ICEBERG_CHECK_OR_DIE before the narrowing cast. The bound matches the limit implied by iceberg-java's byte arrays, so no legal bucket input is rejected. The vendored murmurhash3_internal.cc is left untouched (tagged third-party code) and the now-checked narrowing is made explicit with static_cast.
Add BucketUtilsTest.HashBytesRejectsOversizedInput, which fabricates an oversized span over a 1-byte buffer (never dereferenced, so no 2 GiB allocation is needed in CI). Without the guard the test segfaults.