Skip to content

GH-50351: [C++] Fix KeyValueMetadata::DeleteMany crash with duplicate…#50376

Open
AdvancedUno wants to merge 1 commit into
apache:mainfrom
AdvancedUno:gh-50351-deletemany-duplicate-indices
Open

GH-50351: [C++] Fix KeyValueMetadata::DeleteMany crash with duplicate…#50376
AdvancedUno wants to merge 1 commit into
apache:mainfrom
AdvancedUno:gh-50351-deletemany-duplicate-indices

Conversation

@AdvancedUno

Copy link
Copy Markdown

Rationale for this change

KeyValueMetadata::DeleteMany crashes (out-of-bounds write) when the indices vector
contains duplicates, e.g. DeleteMany({0, 0}). The compaction loop increments its shift
counter once per entry, so duplicates cause writes at negative offsets.

Additionally, out-of-range indices were only checked by DCHECK, so in release builds
DeleteMany({size}) silently deleted the last element and negative indices were UB.

What changes are included in this PR?

  • DeleteMany now validates indices after sorting and returns Status::IndexError for any index out of [0, size), matching the behavior and message format of Delete. Metadata is left unmodified on error.
  • Duplicate indices are removed with std::unique before compaction, so deleting the same index twice is equivalent to deleting it once.

Are these changes tested?

Yes, I added cases to KeyValueMetadataTest.Delete covering duplicate indices ({0,0}, {1,3,1,3,1}), out-of-bounds positive and negative indices (exact error messages), and that metadata is unchanged after a failed call.

Are there any user-facing changes?

Yes, minor behavioral fix: DeleteMany now returns IndexError for out-of-range indices
instead of crashing (duplicates) or silently corrupting state (release builds). No API change.

@pitrou

pitrou commented Jul 6, 2026

Copy link
Copy Markdown
Member

I don't think we should so such extensive validation of input arguments here. Rather, we should document the pre-conditions when calling DeleteMany.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants