Skip to content

GH-50054: [C++][IPC] Validate indices buffer size in ReadSparseCOOIndex#50055

Merged
rok merged 3 commits into
apache:mainfrom
metsw24-max:ipc-sparse-coo-index-bounds
Jun 30, 2026
Merged

GH-50054: [C++][IPC] Validate indices buffer size in ReadSparseCOOIndex#50055
rok merged 3 commits into
apache:mainfrom
metsw24-max:ipc-sparse-coo-index-bounds

Conversation

@metsw24-max

@metsw24-max metsw24-max commented May 27, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

ReadSparseCOOIndex builds the indices Tensor of shape {non_zero_length, ndim} over indicesBuffer with no size check, unlike the sibling ReadSparseCSXIndex. A crafted COO message with a large non_zero_length and a small buffer produces an index tensor that overruns its buffer, giving an out-of-bounds read when the sparse tensor is consumed (for example, converted to dense). A non_zero_length near INT64_MAX also overflows the non_zero_length * ndim * byte_width size product.

What changes are included in this PR?

A minimum-size guard on indicesBuffer in ReadSparseCOOIndex, mirroring the existing check in ReadSparseCSXIndex and using MultiplyWithOverflow for the size computation.

Are these changes tested?

The guard is a no-op for the valid inputs covered by the round-trip tests in cpp/src/arrow/ipc/tensor_test.cc; the rejection and overflow behaviour was checked against a standalone reproducer of the size computation.

Are there any user-facing changes?

Reading a malformed sparse COO tensor message now returns Status::Invalid instead of constructing an out-of-bounds index tensor.

This PR contains a "Critical Fix". A malformed IPC SparseTensor (COO) message could trigger an out-of-bounds read when the resulting sparse tensor is consumed.

@github-actions

Copy link
Copy Markdown

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

@metsw24-max

Copy link
Copy Markdown
Contributor Author

gentle ping

@pitrou

pitrou commented Jun 25, 2026

Copy link
Copy Markdown
Member

@rok Can you take a look at this?

Comment thread cpp/src/arrow/ipc/reader.cc Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jun 30, 2026
@rok rok requested a review from pitrou as a code owner June 30, 2026 12:11
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 30, 2026
Comment thread cpp/src/arrow/ipc/reader.cc Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 30, 2026

@rok rok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense.

@rok rok merged commit a6c2018 into apache:main Jun 30, 2026
58 of 59 checks passed
@rok rok removed the awaiting changes Awaiting changes label Jun 30, 2026
@github-actions github-actions Bot added the awaiting merge Awaiting merge label Jun 30, 2026
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit a6c2018.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 21 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants