GH-50351 : [C++] KeyValueMetadata::DeleteMany method crashes

[dkp116]

Rationale for this change

Investigated the two usages of DeleteMany in the codebase to verify whether there could be an issue caused by incorrect or duplicate indexes being passed.

There are two locations where DeleteMany is called:

1. cpp/src/arrow/c/bridge.cc

DeleteMany(metadata_.extension_name_index, metadata_.extension_serialized_index)

The indexes are populated while iterating through metadata keys:

if (keys[i] == kExtensionTypeKeyName) {
  decoded.extension_name = values[i];
  decoded.extension_name_index = i;
} else if (keys[i] == kExtensionMetadataKeyName) {
  decoded.extension_serialized = values[i];
  decoded.extension_serialized_index = i;
}

Here due to the if and else if statement there is not a case where they are the same value.

2. cpp/src/arrow/ipc/metadata_internal.cc

metadata->DeleteMany({name_index, data_index});

Here, the indexes are obtained using:

name_index = metadata->FindKey(kExtensionTypeKeyName);
data_index = metadata->FindKey(kExtensionMetadataKeyName);

FindKey traverses the keys_ array maintained by KeyValueMetadata and returns the index of the matching key. Since the two searched keys are different:

cpp/src/arrow/extension_type.cc

extern const char kExtensionTypeKeyName[] = "ARROW:extension:name";
extern const char kExtensionMetadataKeyName[] = "ARROW:extension:metadata";

the returned indexes refer to two different metadata entries.

What changes are included in this PR?

This change documents/investigates the behavior of the existing DeleteMany, and a log is produced when DeleteMany is accessing an invalid memory address instead of throwing with this error:
/usr/include/c++/16.1.1/bits/stl_vector.h:1253: constexpr std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator [with _Tp = std::__cxx11::basic_string; _Alloc = std::allocator<std::__cxx11::basic_string >;
reference = std::__cxx11::basic_string&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.

Note: I have not prevented the use of the same index in DeleteMany as for example DeleteMany(1,1) will still give an output, given a valid key and array vector so no functionality has been changed, just a more readable log has been added.

Are these changes tested?

A test has been added to cpp/src/arrow/util/key_value_metadata_test.cc which checks to see the correct error is shown when an invalid memory address is being accessed

Are there any user-facing changes?

No user-facing changes.

• GitHub Issue: [C++] KeyValueMetadata::DeleteMany method crashes with duplicate 0 indexes #50351

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[Reranko05]

Could DeleteMany also detect duplicate indices when the duplicate is the last index, e.g. DeleteMany({6, 6}) for metadata with 7 entries? Could you add a regression test for this case to verify this behaviour like this

  {
    KeyValueMetadata metadata(keys, values);
    std::string expected_error_message =
        "Index error: KeyValueMetadata::DeleteMany: duplicate index 6 in indices to "
        "delete";

    ASSERT_RAISES_WITH_MESSAGE(IndexError, expected_error_message,
                               metadata.DeleteMany({6, 6}));
  }

[Reranko05]

Could we validate the indices are within bounds before checking for duplicates? The current validation could produce a misleading error message when an out-of-bounds index collides with the sentinel value, and add a regression test for the same :)

[Reranko05]

LGTM :)
cc @pitrou