From ca1b17591f3aac5cd6978e24f1a2d7b8fed7c506 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 25 Jul 2026 17:15:49 +0200 Subject: [PATCH 1/2] fix: reject malformed Arrow field IDs --- src/iceberg/schema_internal.cc | 15 ++++++++++----- src/iceberg/test/arrow_test.cc | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index 5dac0d3bf..8808cd582 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -227,7 +227,7 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { namespace { -int32_t GetFieldId(const ArrowSchema& schema) { +Result GetFieldId(const ArrowSchema& schema) { if (schema.metadata == nullptr) { return kUnknownFieldId; } @@ -240,9 +240,14 @@ int32_t GetFieldId(const ArrowSchema& schema) { return kUnknownFieldId; } - int32_t field_id = kUnknownFieldId; - std::from_chars(field_id_value.data, field_id_value.data + field_id_value.size_bytes, - field_id); + int32_t field_id = 0; + const auto* end = field_id_value.data + field_id_value.size_bytes; + const auto [ptr, ec] = std::from_chars(field_id_value.data, end, field_id); + if (ec != std::errc{} || ptr != end) { + return InvalidSchema( + "Invalid Arrow field ID: '{}'", + std::string_view(field_id_value.data, field_id_value.size_bytes)); + } return field_id; } @@ -252,7 +257,7 @@ Result> FromArrowSchema(const ArrowSchema& schema) { [](const ArrowSchema& schema) -> Result> { ICEBERG_ASSIGN_OR_RAISE(auto field_type, FromArrowSchema(schema)); - auto field_id = GetFieldId(schema); + ICEBERG_ASSIGN_OR_RAISE(auto field_id, GetFieldId(schema)); bool is_optional = (schema.flags & ARROW_FLAG_NULLABLE) != 0; if (field_type->type_id() == TypeId::kUnknown && !is_optional) { return InvalidSchema("Arrow null field '{}' must be nullable", schema.name); diff --git a/src/iceberg/test/arrow_test.cc b/src/iceberg/test/arrow_test.cc index d18a6eaf9..3450076d1 100644 --- a/src/iceberg/test/arrow_test.cc +++ b/src/iceberg/test/arrow_test.cc @@ -371,6 +371,25 @@ TEST_P(FromArrowSchemaTest, PrimitiveType) { ASSERT_EQ(*field.type(), *param.iceberg_type); } +TEST(FromArrowSchemaTest, RejectMalformedFieldIdMetadata) { + for (const auto& field_id : {"1x", "2147483648", ""}) { + auto metadata = + ::arrow::key_value_metadata(std::unordered_map{ + {std::string(kParquetFieldIdKey), field_id}}); + auto arrow_schema = ::arrow::schema({::arrow::field( + "foo", ::arrow::int32(), /*nullable=*/true, std::move(metadata))}); + ArrowSchema exported_schema; + ASSERT_TRUE(::arrow::ExportSchema(*arrow_schema, &exported_schema).ok()); + + auto result = FromArrowSchema(exported_schema, /*schema_id=*/1); + ArrowSchemaRelease(&exported_schema); + + EXPECT_THAT(result, IsError(ErrorKind::kInvalidSchema)); + EXPECT_THAT(result, + HasErrorMessage(std::format("Invalid Arrow field ID: '{}'", field_id))); + } +} + INSTANTIATE_TEST_SUITE_P( SchemaConversion, FromArrowSchemaTest, ::testing::Values( From 79ba594ced92b665d41ad5155061db3f7fdfaa91 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Mon, 3 Aug 2026 10:01:47 +0200 Subject: [PATCH 2/2] refactor: use shared Arrow field ID parser --- src/iceberg/schema_internal.cc | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index 8808cd582..087d90a90 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -19,8 +19,6 @@ #include "iceberg/schema_internal.h" -#include -#include #include #include #include @@ -29,6 +27,7 @@ #include "iceberg/schema.h" #include "iceberg/type.h" #include "iceberg/util/macros.h" +#include "iceberg/util/string_util.h" namespace iceberg { @@ -240,16 +239,14 @@ Result GetFieldId(const ArrowSchema& schema) { return kUnknownFieldId; } - int32_t field_id = 0; - const auto* end = field_id_value.data + field_id_value.size_bytes; - const auto [ptr, ec] = std::from_chars(field_id_value.data, end, field_id); - if (ec != std::errc{} || ptr != end) { + std::string_view field_id(field_id_value.data, field_id_value.size_bytes); + auto field_id_result = StringUtils::ParseNumber(field_id); + if (!field_id_result.has_value()) { return InvalidSchema( - "Invalid Arrow field ID: '{}'", - std::string_view(field_id_value.data, field_id_value.size_bytes)); + "Invalid Arrow field ID: '{}'", field_id); } - return field_id; + return field_id_result.value(); } Result> FromArrowSchema(const ArrowSchema& schema) {