Copilot commented on code in PR #961: URL: https://github.com/apache/iceberg-cpp/pull/961#discussion_r4280563115
########## 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. Review Comment: The `data` contract is partially implied but not explicit for successful calls. To prevent misuse, document that `data` must be non-null when `len > 0` (and optionally clarify whether `data` may be null when `len == 0`). ########## src/iceberg/util/bucket_util.cc: ########## @@ -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()); Review Comment: `bucket_util.h` and `bucket_util_internal.h` both document this as throwing `IcebergError`, and the new test expects an exception. If `ICEBERG_CHECK_OR_DIE` terminates the process (as its name suggests) instead of throwing, callers/tests won’t be able to catch it. Use/introduce a throwing check (or explicit `throw IcebergError`) for this validation, or adjust the documentation/test to match the actual behavior. ########## src/iceberg/test/bucket_util_test.cc: ########## @@ -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); }, Review Comment: The PR description says the test 'fabricates an oversized span over a 1-byte buffer', but the test now calls `internal::HashBytes(nullptr, oversized_length)` instead. Please update the PR description to match the implemented approach (or adjust the test if the span-based approach is still the intended one). ########## 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); Review Comment: This is labeled as an internal entry point, but `ICEBERG_EXPORT` makes it part of the library’s public ABI surface. If this API is meant to be internal-only, avoid exporting it (or keep it in a non-installed/private header and/or a `detail`/`internal` library target with hidden visibility) to prevent long-term ABI/support commitments. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
