Skip to content

Prevent Rows row index overflow#9817

Merged
alamb merged 1 commit intoapache:mainfrom
alamb:codex/rows-row-index-overflow
Apr 25, 2026
Merged

Prevent Rows row index overflow#9817
alamb merged 1 commit intoapache:mainfrom
alamb:codex/rows-row-index-overflow

Conversation

@alamb
Copy link
Copy Markdown
Contributor

@alamb alamb commented Apr 25, 2026

Which issue does this PR close?

  • None.

Rationale for this change

Rows used unchecked usize arithmetic when validating a requested row index. In optimized builds, very large indexes could wrap the bounds check before reaching the unchecked row access path.

What changes are included in this PR?

This adds checked arithmetic for row index validation and reuses it for both Rows::row and Rows::row_len.

Are these changes tested?

Yes. This adds regression coverage for overflowing row indexes.

Are there any user-facing changes?

Invalid row indexes that overflow during bounds validation now panic consistently. There are no API changes.

@github-actions github-actions Bot added the arrow Changes to the arrow crate label Apr 25, 2026
@alamb alamb marked this pull request as ready for review April 25, 2026 03:30
@alamb
Copy link
Copy Markdown
Contributor Author

alamb commented Apr 25, 2026

Thanks @Dandandan

@alamb alamb merged commit 479ad5b into apache:main Apr 25, 2026
13 checks passed
Comment thread arrow-row/src/lib.rs
fn checked_row_end(&self, row: usize) -> usize {
row.checked_add(1)
.filter(|end| *end < self.offsets.len())
.expect("row index out of bounds")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR just replaces an existing assert! panic with a new explicit panic that cannot be optimized away?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In rust assert! is not optimized away in release builds (only debug_assert! is)

The change here also also checks for overflow when doing row + 1 (using checked_add). Without that, if you pass in usize::MAX it will wrap around to 0, which I think will pass the assert and not trigger the panic

I do think trying to access memory at offset uisze::MAX will then most likely then cause a SIGSEGV, but it seemed better to check the corner case anyway

Copy link
Copy Markdown
Contributor

@scovich scovich Apr 27, 2026

Choose a reason for hiding this comment

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

I do think trying to access memory at offset uisze::MAX will then most likely then cause a SIGSEGV, but it seemed better to check the corner case anyway

Yeah, Vec specifically documents a panic if you try to grow it beyond isize::MAX entries. Not sure if the same applies to slices ion general or if that's a vec-specific thing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, it does seem to be a general limitation:
https://doc.rust-lang.org/std/primitive.pointer.html#method.offset

Allocations can never be larger than isize::MAX bytes

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

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants