Repository navigation
fix(view): checked shape products and dense strides in StridedArray constructors (#205) - #346
Merged
Merged
Conversation
…onstructors (#205) The allocating StridedArray constructors computed dims.iter().product() unchecked and col_major_strides/row_major_strides multiplied unchecked (and cast extents to isize with as). In a release build [1 << 62, 4] wrapped to 0 elements, so the array kept an empty Vec behind a [1 << 62, 4] layout and view()/get dereferenced past it. The len() accessors had the same product. - New try_col_major, try_row_major, try_from_fn_col_major, try_from_fn_row_major, try_col_major_from_buffer, try_col_major_uninit and try_col_major_from_buffer_uninit return StridedError::OffsetOverflow when the element count or a stride does not fit isize. f is not called on error. - The existing constructors keep their signatures (0.x patch-compatible) and delegate to them, panicking with a message instead of wrapping; documented under # Panics. - col_major_strides/row_major_strides panic on overflow of a non-empty shape; a shape with a zero extent saturates instead, since it is never accessed. - len() counts a zero extent first and panics instead of wrapping when only a broadcast layout's count overflows usize. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Repository rules reviewRepository rules review (7053f9d1a35906a0c5593d0802851f63b5f779ef...77bf594871f67e5251ac47ab65b49caae21fcc8b) Verdict: pass Findings: - [warn] llm-skipped (External LLM Review) <unknown>: External LLM review was skipped External LLM review is permanently disabled; deterministic checks only. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #205.
Problem
StridedArray's allocating constructors computeddims.iter().product()without overflow checks.col_major_strides/row_major_stridesmultiplied without checks and cast extents withas isize. In a release build,[1 << 62, 4]wrapped to 0 elements: the array held an emptyVecbehind a[1 << 62, 4]layout, andview()/getread past it. Thelen()accessors (StridedView,StridedViewMut,StridedArray) used the same unchecked product.Change (strided-view only, no changed signatures, so this can ship as a patch)
try_*constructors.try_col_major,try_row_major,try_from_fn_col_major,try_from_fn_row_major,try_col_major_from_buffer, plus the unsafetry_col_major_uninitandtry_col_major_from_buffer_uninit. They returnStridedError::OffsetOverflowwhen the element count or a stride does not fitisize.fis not called on error. I reused the existingOffsetOverflowvariant becauseStridedErroris not#[non_exhaustive], so a new variant would be a breaking change.try_*versions. They now panic with a message instead of wrapping, documented under# Panics. Under REPOSITORY_RULES "Public Boundary Safety" they should really returnResult, but that changes their signatures. I suggest deprecating them, or switching them toResult, in the next minor release.col_major_strides/row_major_strides. Built with checked multiplication andisize::try_from. They panic only when a non-empty shape overflows. A shape with a zero extent saturates instead of panicking, because none of its elements are ever accessed. For example,[1 << 62, 4, 0]stays constructible as an empty array. Values for every shape that does not overflow are unchanged.len(). A zero extent now short-circuits the product, so[usize::MAX, 2, 0]returns 0; before, debug builds overflowed. It panics only when the element count of a broadcast (stride-0) layout exceedsusize.Tests
try_*constructor is checked against five overflowing shapes:[1<<62, 4]wraps to 0;[1<<62, 2]overflows onlyisize;[2, 1<<62];[isize::MAX+2, 1]overflows theas isizeconversion;[usize::MAX, usize::MAX].[1<<62, 4, 0]and[0, usize::MAX, usize::MAX].col_majorandcol_major_strides.[3, 0, 5]keeps its existing values, and the saturating cases are checked.len(). Of a zero-sized broadcast view, and of an overflowing one.The five tests that do not use the new API fail on
origin/mainand pass with this change.Verification
All builds ran under
nice -n 19 taskset -c 56-63with-j 8. broccoli has only CPUs 0-63.cargo fmt --all -- --check: OKcargo test --workspace: 1106 passed, 0 failedcargo clippy -p strided-view --all-targets: no new warnings in the changed code (the existingmanual_containswarnings are untouched)🤖 Generated with Claude Code