taepper commented on code in PR #54939:
URL: https://github.com/apache/arrow/pull/54939#discussion_r4410784074
##########
cpp/src/arrow/ipc/reader.cc:
##########
@@ -455,6 +456,21 @@ class ArrayLoader {
Status Visit(const MapType& type) {
RETURN_NOT_OK(LoadList(type));
+ if (!out_->child_data.empty()) {
+ // Ensure that GetNullCount() calls in MapArray::ValidateChildData
+ // don't access invalid memory.
Review Comment:
True. If we want to validate that the invalid IPC data will be covered by
the respective branches, something like this could be added (mostly generated
by Claude Opus 5.5):
```
// GH-54936: a map array whose entries or keys have an unknown (-1) null
count must be
// rejected, rather than computing the null count from an unvalidated
validity bitmap.
TEST(TestReadRecordBatch, InvalidMapNullCount) {
auto batch = RecordBatchFromJSON(schema({field("f", map(int32(),
int32()))}),
R"([{"f": [[1, 10], [2, 20]]}])");
ASSERT_OK_AND_ASSIGN(auto serialized,
SerializeRecordBatch(*batch,
IpcWriteOptions::Defaults()));
io::BufferReader stream(serialized);
ASSERT_OK_AND_ASSIGN(auto message, ReadMessage(&stream));
// The message metadata has one FieldNode per array, in depth-first order:
// 0 = map, 1 = entries, 2 = keys, 3 = items.
auto checkValidationWithInvalidIPCField = [&](int
invalid_field_node_index, const std::string& error) {
ASSERT_OK_AND_ASSIGN(auto metadata,
message->metadata()->CopySlice(0,
message->metadata()->size()));
auto* fb_batch =
flatbuf::GetMessage(metadata->data())->header_as_RecordBatch();
// The writer emitted an empty validity bitmap for this node. With a
huge length
// and an unknown null count, computing the null count would read far
out of
// bounds of that bitmap and crash.
*const_cast<flatbuf::FieldNode*>(fb_batch->nodes()->Get(invalid_field_node_index))
=
flatbuf::FieldNode(/*length=*/int64_t{1} << 60, /*null_count=*/-1);
io::BufferReader body(message->body());
DictionaryMemo memo;
EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr(error),
ReadRecordBatch(*metadata,
batch->schema(), &memo,
IpcReadOptions::Defaults(), &body));
};
checkValidationWithInvalidIPCField(/*invalid_field_node_index=*/1, "Map
array child array should have no nulls");
checkValidationWithInvalidIPCField(/*invalid_field_node_index=*/2, "Map
array keys array should have no nulls");
}
```
I concede that this could be too reliant on implementation details and be
mostly a maintenance burden without much gain, so I am fine either way. For
what it's worth, it does lead to crashes without your fix :)
--
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]
