GH-50311: [C++] `KeyValueMetadata::Delete` returns IndexError instead of crashing due to seg fault by OmBiradar · Pull Request #50322 · apache/arrow

OmBiradar · 2026-07-01T15:54:17Z

The Delete function now catches out of bound index and does not throw a segmentation fault.

Rationale for this change

The KeyValueMetadata::Delete(int64_t index) never checked for out of bounds value of index, & if index was out of bounds, then a seg fault was thrown by the program and it aborted.

What changes are included in this PR?

The KeyValueMetadata::Delete(int64_t index) now returns a IndexError for out of bounds values of index

Are these changes tested?

Yes, CI tests pass on my fork

Are there any user-facing changes?

No API changes

GitHub Issue: [C++] KeyValueMetadata gives seg fault while deleting a key #50311

github-actions · 2026-07-01T15:54:42Z

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

Copilot

Pull request overview

This PR fixes a crash in the C++ arrow::KeyValueMetadata::Delete(int64_t index) implementation by adding bounds checking so out-of-range indices return Status::IndexError instead of triggering undefined behavior.

Changes:

Added an out-of-bounds guard to KeyValueMetadata::Delete(int64_t) using ARROW_PREDICT_FALSE and std::cmp_greater_equal.
Returned Status::IndexError when index < 0 or index >= size.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

* The Delete function now catches out of bound index and does not throw a segmentation fault. Signed-off-by: OmBiradar <ombiradar04@gmail.com>

OmBiradar · 2026-07-02T06:20:41Z

The above has been a messy PR with me amending the commit mostly due to me being unsure about the exact error message.

pitrou · 2026-07-02T08:31:42Z

 }

 Status KeyValueMetadata::Delete(int64_t index) {
+  if (ARROW_PREDICT_FALSE(index < 0 || std::cmp_greater_equal(index, values_.size()))) {


Why std::cmp_greater_equal? Just use the standard comparison operator here.

The types are int64_t and size_t, so any operator would cast the int64_t to a size_t which retults in negative numbers being converted to extremely large values (>2^31). While even if this happens the logic would work for any case of negative values as index<0 is true, I think this is the general practice for these types of comparisons? based on what I know

The Arrow codebase generally casts explicitly, i.e. static_cast<int64_t>(values_.size())

For values_.size() > 2^31 it would overflow and return a -ve int64_t. Tho it's extremely impossible to have so many elments in the first place.

I was just trying to make it secure 😅

Should I use static_cast<int64_t>? for consistency with the project
@pitrou

Yes, please use that :)

I changed it to static_cast

pitrou · 2026-07-02T08:34:09Z

@OmBiradar This is not really a "critical fix" as this is mostly a API usage question (the caller should have checked index bounds accordingly), can you edit the PR description?

OmBiradar · 2026-07-02T08:39:50Z

@pitrou yes sure, did it. I was unsure about what critical fixes mean, my rational to add it was that a seg fault would be triggered and crash any program.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

OmBiradar · 2026-07-02T08:45:17Z

Forgot to update the tests

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Signed-off-by: OmBiradar <ombiradar04@gmail.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 1, 2026 15:54

OmBiradar requested a review from pitrou as a code owner July 1, 2026 15:54

github-actions Bot added the Component: C++ label Jul 1, 2026

Copilot started reviewing on behalf of OmBiradar July 1, 2026 15:54 View session

github-actions Bot added the awaiting review Awaiting review label Jul 1, 2026

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread cpp/src/arrow/util/key_value_metadata.cc Outdated

Comment thread cpp/src/arrow/util/key_value_metadata.cc

OmBiradar force-pushed the KeyValueMetadata-catch-out-of-bounds branch 2 times, most recently from c2efac9 to 046bb2e Compare July 2, 2026 06:07

Copilot AI review requested due to automatic review settings July 2, 2026 06:07

Copilot started reviewing on behalf of OmBiradar July 2, 2026 06:07 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread cpp/src/arrow/util/key_value_metadata.cc Outdated

Add Out of bounds IndexError for KeyValueMetadata::Delete

15fb3bf

* The Delete function now catches out of bound index and does not throw a segmentation fault. Signed-off-by: OmBiradar <ombiradar04@gmail.com>

OmBiradar force-pushed the KeyValueMetadata-catch-out-of-bounds branch from 046bb2e to 15fb3bf Compare July 2, 2026 06:16

pitrou reviewed Jul 2, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 2, 2026

Copilot AI review requested due to automatic review settings July 2, 2026 08:41

Copilot started reviewing on behalf of OmBiradar July 2, 2026 08:41 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread cpp/src/arrow/util/key_value_metadata_test.cc

Comment thread cpp/src/arrow/util/key_value_metadata_test.cc

OmBiradar force-pushed the KeyValueMetadata-catch-out-of-bounds branch from 5ecddee to 7acf912 Compare July 2, 2026 08:49

OmBiradar requested review from Copilot and pitrou July 2, 2026 08:50

OmBiradar force-pushed the KeyValueMetadata-catch-out-of-bounds branch from 7acf912 to 3eee41b Compare July 2, 2026 09:39

Copilot started reviewing on behalf of OmBiradar July 2, 2026 09:39 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread cpp/src/arrow/util/key_value_metadata.cc Outdated

Comment thread cpp/src/arrow/util/key_value_metadata_test.cc

Update KeyValueMetadata::Delete IndexError message

926c6c4

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

OmBiradar force-pushed the KeyValueMetadata-catch-out-of-bounds branch from 3eee41b to 926c6c4 Compare July 2, 2026 09:44

Use static_cast instead of std::cmp_greater_equal

a5f1bd9

Signed-off-by: OmBiradar <ombiradar04@gmail.com>

Copilot AI review requested due to automatic review settings July 2, 2026 11:03

Copilot started reviewing on behalf of OmBiradar July 2, 2026 11:03 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

pitrou approved these changes Jul 2, 2026

View reviewed changes

pitrou merged commit 98347d2 into apache:main Jul 2, 2026
61 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Jul 2, 2026

pitrou mentioned this pull request Jul 2, 2026

[C++] KeyValueMetadata gives seg fault while deleting a key #50311

Closed

Uh oh!

Conversation

OmBiradar commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

OmBiradar commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

OmBiradar Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

OmBiradar Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OmBiradar Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

OmBiradar Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pitrou commented Jul 2, 2026

Uh oh!

OmBiradar commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

OmBiradar commented Jul 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

OmBiradar commented Jul 1, 2026 •

edited

Loading

OmBiradar commented Jul 2, 2026 •

edited

Loading

OmBiradar Jul 2, 2026 •

edited

Loading

OmBiradar Jul 2, 2026 •

edited

Loading

OmBiradar Jul 2, 2026 •

edited

Loading

OmBiradar commented Jul 2, 2026 •

edited

Loading