This is an automated email from the ASF dual-hosted git repository. wgtmac pushed a commit to branch codex/bucket-index-oversized-guard in repository https://gitbox.apache.org/repos/asf/iceberg-cpp.git
commit ea38bd65e203f5344aef56aa074995afef5d32db Author: Gang Wu <[email protected]> AuthorDate: Thu Oct 15 16:53:38 2026 +0800 refactor: guard oversized bucket hash input in BucketIndex 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
