fix(bucket): reject oversized input in BucketUtils::HashBytes (#961)

* fix(bucket): reject oversized input in BucketUtils::HashBytes

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.

* test(bucket): guard raw byte input so the oversized-length test is well-formed

The regression test built a std::span over INT32_MAX + 1 elements of a
one-byte object, which violates the std::span constructor precondition and
is undefined behavior even though the data is never read.

Move the length guard into an internal HashBytes(const uint8_t*, size_t)
overload and keep BucketUtils::HashBytes(std::span) as a thin wrapper, so the
test can pass a null pointer with the oversized length instead.

Also document that HashBytes and BucketIndex throw IcebergError for inputs
larger than INT32_MAX, which the public headers did not mention.

---------

Co-authored-by: Gang Wu <ustcwg@gmail.com>

Author: WZhuo

src/iceberg/test/bucket_util_test.cc

@@ -20,11 +20,16 @@
 #include "iceberg/util/bucket_util.h"
 
 #include <chrono>
+#include <cstdint>
+#include <limits>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include "iceberg/exception.h"
 #include "iceberg/expression/literal.h"
 #include "iceberg/test/temporal_test_helper.h"
+#include "iceberg/util/bucket_util_internal.h"
 #include "iceberg/util/decimal.h"
 #include "iceberg/util/uuid.h"
 
@@ -108,6 +113,19 @@ TEST(BucketUtilsTest, HashHelper) {
   EXPECT_EQ(BucketUtils::HashBytes(fixed), -188683207);
 }
 
+TEST(BucketUtilsTest, HashBytesRejectsOversizedInput) {
+  // Lengths above INT32_MAX would wrap the 32-bit signed length parameter of
+  // MurmurHash3_x86_32 and read out of bounds. The guard rejects them before the
+  // data is touched, so pass a null pointer instead of building a std::span over
+  // more bytes than the object provides (which is undefined behavior) or
+  // allocating 2 GiB.
+  const auto oversized_length =
+      static_cast<size_t>(std::numeric_limits<int32_t>::max()) + 1;
+  ASSERT_THAT([&]() { internal::HashBytes(nullptr, oversized_length); },
+              ::testing::ThrowsMessage<IcebergError>(
+                  ::testing::HasSubstr("exceeds the maximum supported length")));
+}
+
 TEST(BucketUtilsTest, BucketTimestampNanosMatchesMicros) {
   constexpr int32_t kNumBuckets = 1000;
   const auto ts_micros = TemporalTestHelper::CreateTimestamp({.year = 2017,

src/iceberg/util/bucket_util.cc

@@ -19,9 +19,12 @@
 
 #include "iceberg/util/bucket_util.h"
 
+#include <limits>
 #include <utility>
 
+#include "iceberg/exception.h"
 #include "iceberg/expression/literal.h"
+#include "iceberg/util/bucket_util_internal.h"
 #include "iceberg/util/endian.h"
 #include "iceberg/util/murmurhash3_internal.h"
 #include "iceberg/util/temporal_util.h"
@@ -110,11 +113,29 @@ int32_t HashLiteral<TypeId::kFixed>(const Literal& literal) {
 }  // namespace
 
 int32_t BucketUtils::HashBytes(std::span<const uint8_t> bytes) {
+  return internal::HashBytes(bytes.data(), bytes.size());
+}
+
+namespace internal {
+
+int32_t HashBytes(const uint8_t* data, size_t len) {
+  // MurmurHash3_x86_32 takes a signed 32-bit length. Passing a larger size_t
+  // would wrap it negative and make the hash loop read far outside `data`
+  // (and silently hash the wrong prefix for even larger inputs). Iceberg's
+  // bucket transform is defined on byte sequences of at most 2^31 - 1 bytes
+  // (Java arrays), so reject anything longer before narrowing.
+  ICEBERG_CHECK_OR_DIE(
+      len <= static_cast<size_t>(std::numeric_limits<int32_t>::max()),
+      "BucketUtils::HashBytes: input length {} exceeds the maximum supported "
+      "length {}",
+      len, std::numeric_limits<int32_t>::max());
   int32_t hash_value = 0;
-  MurmurHash3_x86_32(bytes.data(), bytes.size(), 0, &hash_value);
+  MurmurHash3_x86_32(data, static_cast<int>(len), 0, &hash_value);
   return hash_value;
 }
 
+}  // namespace internal
+
 int32_t BucketUtils::HashLong(int64_t value) {
   int32_t hash_value = 0;
   value = ToLittleEndian(value);

src/iceberg/util/bucket_util.h

@@ -51,12 +51,19 @@ class ICEBERG_EXPORT BucketUtils {
   /// \brief Hash a byte array using MurmurHash3 and return a 32-bit hash value.
   /// \param bytes The input byte array to hash.
   /// \return A 32-bit hash value.
+  /// \throws IcebergError if `bytes.size()` exceeds `INT32_MAX`. MurmurHash3 takes a
+  ///     signed 32-bit length, and the bucket transform is only defined for byte
+  ///     sequences up to that size, so longer inputs are rejected rather than
+  ///     truncated. Java cannot produce such a value because its arrays are
+  ///     int-indexed, so nothing iceberg-java could hash is rejected here.
   static int32_t HashBytes(std::span<const uint8_t> bytes);
 
   /// \brief Compute the bucket index for a given literal and number of buckets.
   /// \param literal The input literal to hash.
   /// \param num_buckets The number of buckets to hash into.
   /// \return (murmur3_x86_32_hash(literal) & Integer.MAX_VALUE) % num_buckets
+  /// \throws IcebergError if the literal is a string, binary, or fixed value whose
+  ///     byte length exceeds `INT32_MAX`; see HashBytes.
   static Result<int32_t> BucketIndex(const Literal& literal, int32_t num_buckets);
 };
 

src/iceberg/util/bucket_util_internal.h

@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <cstddef>
+#include <cstdint>
+
+#include "iceberg/iceberg_export.h"
+
+/// \file iceberg/util/bucket_util_internal.h
+/// \brief Internal entry points for the bucket transform.
+
+namespace iceberg::internal {
+
+/// \brief Hash `len` bytes starting at `data` using MurmurHash3.
+///
+/// The raw pointer and length are taken separately so that a length that does not
+/// fit the pointee object can be passed without forming an invalid `std::span`.
+/// `BucketUtils::HashBytes` forwards to this function.
+///
+/// \param data Pointer to the first byte to hash. It is only dereferenced once `len`
+///     is accepted, so it may be null when the call is expected to be rejected.
+/// \param len The number of bytes to hash.
+/// \return A 32-bit hash value.
+/// \throws IcebergError if `len` exceeds `INT32_MAX`; see `BucketUtils::HashBytes`.
+ICEBERG_EXPORT int32_t HashBytes(const uint8_t* data, size_t len);
+
+}  // namespace iceberg::internal