[pitrou]

Rationale for this change

When loading a MapArray from IPC, we call MapArray::ValidateChildData to avoid a later crash in MapArray::SetData.

Unfortunately MapArray::ValidateChildData calls GetNullCount() which is unsafe if the advertised length is too large for the actual validity buffer size.

Found by OSS-Fuzz in https://issues.oss-fuzz.com/issues/556994537

What changes are included in this PR?

Check that the null_count of the relevant fields is equal to zero, so that GetNullCount does not actually try to read buffer memory.

Are these changes tested?

Yes, by additional fuzz regression test.

Are there any user-facing changes?

Just a bugfix.

This PR contains a "Critical Fix". An invalid IPC file would lead to reading memory out of bounds for the purpose of counting validity bits. It is not obvious how this could be exploited by an attacker except for causing a denial of service.

Was AI used for this PR?

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human
• AI
• Not reviewed

• GitHub Issue: [C++] Reading a MapArray from invalid IPC stream/file can read memory out of bounds #54936

[github-actions]

⚠️ GitHub issue #54936 has been automatically assigned in GitHub to PR creator.

[dmitry-chirkov-dremio]

LGTM.

p.s for the lineage - this was missed during #51093 review

[pitrou]

@taepper I don't know if you're familiar with this code but I would welcome extra eyes on this.

[taepper]

I think we can turn this into a

    DCHECK_EQ(out_->child_data.size(), 1);

after LoadList succeeds without an error

[pitrou]

Yes, you're right. Will do.

[taepper]

This is because when null_count == -1, it will be inferred from the unvalidated data? Can we add a (non-fuzzing based) test for this directly?

[pitrou]

This is because when null_count == -1, it will be inferred from the unvalidated data?

Yes.

Can we add a (non-fuzzing based) test for this directly?

Well, for the test to be relevant, it would have to create invalid IPC data (a null bitmap that's too short, for example). Otherwise calling GetNullCount() would succeed anyway.

[taepper]

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 as I am not very familiar with IPC code):

// 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 :)

[pitrou]

Yes, I'd rather rely on the fuzz-generated file (or any other "bad data" file) than adding such maintenance burden to the unit test suite.

[pitrou]

Thanks for the review @taepper !