(iceberg-cpp) branch main updated: refactor: guard oversized bucket hash input in BucketIndex (#1023)

[ga...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

wgtmac pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git


The following commit(s) were added to refs/heads/main by this push:
     new 60e67fa2 refactor: guard oversized bucket hash input in BucketIndex (#1023)
60e67fa2 is described below

commit 60e67fa2aecf41c3626e26fbc71be6afe5c8ef4a
Author: Gang Wu <us...@gmail.com>
AuthorDate: Thu Oct 15 17:47:05 2026 +0800

    refactor: guard oversized bucket hash input in BucketIndex (#1023)
    
    BucketUtils::HashBytes still rejects inputs larger than INT32_MAX with an
    IcebergError, but BucketIndex now checks the byte length of string, binary, and
    fixed literals before hashing and returns InvalidArgument instead, so the
    Result-returning entry point no longer throws.
    
    This replaces the internal HashBytes(const uint8_t*, size_t) overload and the
    bucket_util_internal.h header, which existed only so a test could reach the
    guard without materializing an oversized value. That overload, the header, and
    the HashBytesRejectsOversizedInput test are all removed: the guard cannot be
    exercised from a test without either a 2 GiB allocation or an invalid
    std::span.
---
 src/iceberg/test/bucket_util_test.cc    | 18 -----------
 src/iceberg/util/bucket_util.cc         | 54 ++++++++++++++++++++++-----------
 src/iceberg/util/bucket_util.h          |  2 --
 src/iceberg/util/bucket_util_internal.h | 45 ---------------------------
 4 files changed, 37 insertions(+), 82 deletions(-)

diff --git a/src/iceberg/test/bucket_util_test.cc b/src/iceberg/test/bucket_util_test.cc
index e75933cf..bee84a7c 100644
--- a/src/iceberg/test/bucket_util_test.cc
+++ b/src/iceberg/test/bucket_util_test.cc
@@ -20,16 +20,11 @@
 #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"
 
@@ -113,19 +108,6 @@ 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,
diff --git a/src/iceberg/util/bucket_util.cc b/src/iceberg/util/bucket_util.cc
index 53a93d55..23a4d61c 100644
--- a/src/iceberg/util/bucket_util.cc
+++ b/src/iceberg/util/bucket_util.cc
@@ -20,11 +20,11 @@
 #include "iceberg/util/bucket_util.h"
 
 #include <limits>
+#include <optional>
 #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"
@@ -32,6 +32,29 @@
 namespace iceberg {
 
 namespace {
+
+/// MurmurHash3_x86_32 takes a signed 32-bit length. Iceberg's bucket transform is
+/// defined on byte sequences of at most 2^31 - 1 bytes (Java arrays are int-indexed),
+/// and longer inputs would wrap the length negative and make the hash loop read far
+/// outside the buffer, so they are rejected instead.
+constexpr size_t kMaxHashBytes = static_cast<size_t>(std::numeric_limits<int32_t>::max());
+
+/// \brief Returns the number of bytes `HashLiteral` will hash for `literal`.
+///
+/// Returns `std::nullopt` for values hashed as fixed-width integers, which have no
+/// length limit.
+std::optional<size_t> HashedByteSize(const Literal& literal) {
+  switch (literal.type()->type_id()) {
+    case TypeId::kString:
+      return std::get<std::string>(literal.value()).size();
+    case TypeId::kBinary:
+    case TypeId::kFixed:
+      return std::get<std::vector<uint8_t>>(literal.value()).size();
+    default:
+      return std::nullopt;
+  }
+}
+
 template <TypeId type_id>
 int32_t HashLiteral(const Literal& literal) {
   std::unreachable();
@@ -113,29 +136,16 @@ 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()),
+      bytes.size() <= kMaxHashBytes,
       "BucketUtils::HashBytes: input length {} exceeds the maximum supported "
       "length {}",
-      len, std::numeric_limits<int32_t>::max());
+      bytes.size(), kMaxHashBytes);
   int32_t hash_value = 0;
-  MurmurHash3_x86_32(data, static_cast<int>(len), 0, &hash_value);
+  MurmurHash3_x86_32(bytes.data(), static_cast<int>(bytes.size()), 0, &hash_value);
   return hash_value;
 }
 
-}  // namespace internal
-
 int32_t BucketUtils::HashLong(int64_t value) {
   int32_t hash_value = 0;
   value = ToLittleEndian(value);
@@ -157,6 +167,16 @@ Result<int32_t> BucketUtils::BucketIndex(const Literal& literal, int32_t num_buc
     return NotSupported("Cannot compute bucket index for {}", literal.ToString());
   }
 
+  // HashBytes rejects oversized inputs by throwing, but this function reports failures
+  // through its Result, so check the length up front.
+  if (auto byte_size = HashedByteSize(literal);
+      byte_size.has_value() && *byte_size > kMaxHashBytes) [[unlikely]] {
+    return InvalidArgument(
+        "Cannot compute bucket index for {}: value of {} bytes exceeds the maximum "
+        "supported length {}",
+        literal.type()->ToString(), *byte_size, kMaxHashBytes);
+  }
+
   int32_t hash_value = 0;
   switch (literal.type()->type_id()) {
     DISPATCH_HASH_LITERAL(TypeId::kInt)
diff --git a/src/iceberg/util/bucket_util.h b/src/iceberg/util/bucket_util.h
index 3947d3fc..07e3f47f 100644
--- a/src/iceberg/util/bucket_util.h
+++ b/src/iceberg/util/bucket_util.h
@@ -62,8 +62,6 @@ class ICEBERG_EXPORT BucketUtils {
   /// \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);
 };
 
diff --git a/src/iceberg/util/bucket_util_internal.h b/src/iceberg/util/bucket_util_internal.h
deleted file mode 100644
index bda22dcc..00000000
--- a/src/iceberg/util/bucket_util_internal.h
+++ /dev/null
@@ -1,45 +0,0 @@
-/*
- * 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