From daf537770fff4bf474ae41af060aa7d153a06808 Mon Sep 17 00:00:00 2001 From: Guotao Yu Date: Fri, 23 Oct 2026 11:59:39 +0800 Subject: [PATCH] fix(manifest): reject struct children longer than their parent length nanoarrow FULL validation only requires a struct's children to be at least as long as the parent, but ParseManifestList/ParseDataFile/ ParseManifestEntry index containers sized by the parent's row count using each child's own length. A spec-legal batch with a longer child would drive those parsers past the end of the output vector. Add AssertChildrenLength and call it everywhere a struct's children feed a parent-sized container, including the partition struct case which previously skipped the children-count check entirely. --- src/iceberg/manifest/manifest_reader.cc | 25 +++++++++++++++-- src/iceberg/test/manifest_reader_test.cc | 35 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index df8f036f7..bf2b3c40f 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -51,6 +51,11 @@ namespace iceberg { +// FULL validation only requires struct children to be >= the parent's length, +// but the field parsers below index containers sized by the parent's length +// using each child's own length, so a longer child must be rejected here. +Status AssertChildrenLength(const ArrowArrayView* view, std::string_view field_name); + namespace { [[nodiscard]] Status MakeNullError(std::string_view field_name, int64_t row_idx) { @@ -318,6 +323,7 @@ Result> ParseManifestList(ArrowSchema* arrow_schema, ICEBERG_RETURN_UNEXPECTED(AssertViewTypeAndChildren( &view, ArrowType::NANOARROW_TYPE_STRUCT, schema.fields().size(), "manifest_list")); + ICEBERG_RETURN_UNEXPECTED(AssertChildrenLength(&view, "manifest_list")); std::vector manifest_files; manifest_files.resize(array->length); @@ -476,6 +482,7 @@ Status ParseDataFile(const std::shared_ptr& data_file_schema, ICEBERG_RETURN_UNEXPECTED( AssertViewTypeAndChildren(view, ArrowType::NANOARROW_TYPE_STRUCT, data_file_schema->fields().size(), "data_file")); + ICEBERG_RETURN_UNEXPECTED(AssertChildrenLength(view, "data_file")); for (int64_t col_idx = 0; col_idx < view->n_children; ++col_idx) { ICEBERG_ASSIGN_OR_RAISE(auto field, data_file_schema->GetFieldByIndex(col_idx)); @@ -513,10 +520,12 @@ Status ParseDataFile(const std::shared_ptr& data_file_schema, } break; case DataFile::kPartitionFieldId: { - ICEBERG_RETURN_UNEXPECTED( - AssertViewType(field_view, ArrowType::NANOARROW_TYPE_STRUCT, field_name)); const auto& partition_type = internal::checked_cast(*field->get().type()); + ICEBERG_RETURN_UNEXPECTED( + AssertViewTypeAndChildren(field_view, ArrowType::NANOARROW_TYPE_STRUCT, + partition_type.fields().size(), field_name)); + ICEBERG_RETURN_UNEXPECTED(AssertChildrenLength(field_view, field_name)); for (int64_t part_idx = 0; part_idx < field_view->n_children; part_idx++) { auto part_view = field_view->children[part_idx]; auto part_field_type = internal::checked_pointer_cast( @@ -647,6 +656,7 @@ Result> ParseManifestEntry( ICEBERG_RETURN_UNEXPECTED(AssertViewTypeAndChildren( &view, ArrowType::NANOARROW_TYPE_STRUCT, schema.fields().size(), "manifest_entry")); + ICEBERG_RETURN_UNEXPECTED(AssertChildrenLength(&view, "manifest_entry")); std::vector manifest_entries; manifest_entries.resize(array->length); @@ -835,6 +845,17 @@ class ManifestEntryStreamImpl final : public ManifestEntryStream { } // namespace +Status AssertChildrenLength(const ArrowArrayView* view, std::string_view field_name) { + for (int64_t i = 0; i < view->n_children; i++) { + if (view->children[i]->length != view->length) { + return InvalidArgument( + "Field '{}' child {} should have length {} matching its parent, got {}", + field_name, i, view->length, view->children[i]->length); + } + } + return {}; +} + bool ManifestReader::ShouldDropStats(const std::vector& columns) { // Make sure we only drop all stats if we had projected all stats. // We do not drop stats even if we had partially added some stats columns, except for diff --git a/src/iceberg/test/manifest_reader_test.cc b/src/iceberg/test/manifest_reader_test.cc index 80a8acd72..166d7c0c9 100644 --- a/src/iceberg/test/manifest_reader_test.cc +++ b/src/iceberg/test/manifest_reader_test.cc @@ -30,8 +30,10 @@ #include #include #include +#include #include "iceberg/arrow/arrow_io_util.h" +#include "iceberg/arrow_c_data_guard_internal.h" #include "iceberg/avro/avro_register.h" #include "iceberg/expression/expressions.h" #include "iceberg/file_writer.h" @@ -47,6 +49,9 @@ namespace iceberg { +// Declared in manifest_reader.cc; not exposed via a public header. +Status AssertChildrenLength(const ArrowArrayView* view, std::string_view field_name); + class TestManifestReader : public testing::TestWithParam { protected: void SetUp() override { @@ -743,4 +748,34 @@ TEST(ManifestReaderStaticTest, WithStatsColumnsHandlesEmptyAndSelectAll) { INSTANTIATE_TEST_SUITE_P(ManifestReaderVersions, TestManifestReader, testing::Values(1, 2, 3)); +TEST(AssertChildrenLengthTest, RejectsChildLongerThanParent) { + // nanoarrow FULL validation only requires struct children to be >= the + // parent's length, so build a struct of length 1 whose child has length 2. + auto child = + ::arrow::json::ArrayFromJSONString(::arrow::int32(), "[1, 2]").ValueOrDie(); + auto struct_type = ::arrow::struct_({::arrow::field("a", ::arrow::int32())}); + auto struct_array = std::make_shared<::arrow::StructArray>( + struct_type, /*length=*/1, std::vector>{child}); + + ArrowArray c_array; + ArrowSchema c_schema; + ASSERT_TRUE(::arrow::ExportArray(*struct_array, &c_array, &c_schema).ok()); + internal::ArrowArrayGuard array_guard(&c_array); + internal::ArrowSchemaGuard schema_guard(&c_schema); + + ArrowArrayView view; + ArrowError error; + ASSERT_EQ(ArrowArrayViewInitFromSchema(&view, &c_schema, &error), NANOARROW_OK); + internal::ArrowArrayViewGuard view_guard(&view); + ASSERT_EQ(ArrowArrayViewSetArray(&view, &c_array, &error), NANOARROW_OK); + ASSERT_EQ(ArrowArrayViewValidate(&view, NANOARROW_VALIDATION_LEVEL_FULL, &error), + NANOARROW_OK); + + EXPECT_THAT(AssertChildrenLength(&view, "test_field"), + ::testing::AllOf( + IsError(ErrorKind::kInvalidArgument), + HasErrorMessage("child 0 should have length 1 matching its parent, " + "got 2"))); +} + } // namespace iceberg