Validate tile_view() extent against the parent tile - #1750
Conversation
tile_view() range-checked the origin but never checked that offset + shape fits inside the source tile. Because the view just forms an aliased pointer into the parent storage, an extent reaching past the end of an axis was accepted and silently read, or wrote through, the shared memory that follows. The slice form cannot hit this since its extents are clamped to the parent. Constant offsets and extents are now checked at code-gen time, matching the existing origin check. Runtime values still fall through to the debug-only bounds check. Fixes NVIDIA#1745 Signed-off-by: nileshpatil6 <technil6436@gmail.com>
📝 WalkthroughWalkthrough
Changestile_view bounds validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds code-generation-time validation that constant explicit
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking correction needed for the misleading claim that runtime offsets receive a debug-only bounds check. The constant extent validation and regression tests cover the stated fix, while the only accepted issue is inaccurate commentary about protection on the unchanged runtime-offset path. Files Needing Attention: warp/_src/builtins.py Important Files Changed
Reviews (1): Last reviewed commit: "Validate tile_view() extent against the ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # constant offsets can be checked here; runtime offsets fall through to the | ||
| # debug-only bounds check. |
There was a problem hiding this comment.
The new comment says unresolved runtime offsets fall through to a debug-only bounds check, but native tile_view directly forms the aliased pointer without such a guard. This obscures the existing runtime-offset validation gap and can mislead future maintenance of this safety check.
Knowledge Base Used: Codegen and Execution Pipeline
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@changelog/1745.fixed.md`:
- Around line 1-3: Update the changelog entry to state that constant
offset-plus-shape combinations are validated at code-generation time, while
preserving that origin validation already existed; avoid wording that implies
the new check validates origins or offsets alone.
In `@warp/_src/builtins.py`:
- Around line 4242-4247: Update the explicit shape validation loop around shape,
offset, and parent_shape to reject any integer extent less than or equal to zero
before checking const + extent against the parent dimension. Preserve the
existing handling of non-integer values and upper-bound rejection, and add a
regression test covering zero and negative explicit extents passed through the
tile path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 40c0ac53-a15a-4e85-8c01-bbbbe71ca123
📒 Files selected for processing (3)
changelog/1745.fixed.mdwarp/_src/builtins.pywarp/tests/tile/test_tile_view.py
| Reject `wp.tile_view()` calls whose explicit `shape` reaches past the end of the source tile. The origin was already | ||
| range-checked, but `offset + shape` was not, so a view could silently alias the shared memory following its parent. | ||
| Constant offsets are now validated at code-gen time with the same message style as the existing origin check. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify that the new check validates offset + shape.
Line 3 says that constant offsets are now validated, but the origin was already range-checked. The new behavior validates constant offset-and-shape combinations. Update the wording to avoid implying that origin validation was added.
Proposed wording
-Constant offsets are now validated at code-gen time with the same message style as the existing origin check.
+Constant offset-and-shape combinations are now validated at code-gen time with the same message style as the existing origin check.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Reject `wp.tile_view()` calls whose explicit `shape` reaches past the end of the source tile. The origin was already | |
| range-checked, but `offset + shape` was not, so a view could silently alias the shared memory following its parent. | |
| Constant offsets are now validated at code-gen time with the same message style as the existing origin check. | |
| Reject `wp.tile_view()` calls whose explicit `shape` reaches past the end of the source tile. The origin was already | |
| range-checked, but `offset + shape` was not, so a view could silently alias the shared memory following its parent. | |
| Constant offset-and-shape combinations are now validated at code-gen time with the same message style as the existing origin check. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@changelog/1745.fixed.md` around lines 1 - 3, Update the changelog entry to
state that constant offset-plus-shape combinations are validated at
code-generation time, while preserving that origin validation already existed;
avoid wording that implies the new check validates origins or offsets alone.
| for dim, extent in enumerate(shape): | ||
| entry = offset[dim] if dim < len(offset) else 0 | ||
| const = entry.constant if isinstance(entry, Var) else entry | ||
| if not isinstance(const, int) or not isinstance(extent, int): | ||
| continue | ||
| if const + extent > parent_shape[dim]: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-positive explicit extents.
This check only rejects const + extent > parent_shape[dim]. It accepts zero and negative extents. The slice path already rejects zero-length axes at Line 4216, and the explicit shape= path passes these values to tile(...) below. Reject extent <= 0 before the upper-bound check and add a regression test.
Proposed fix
for dim, extent in enumerate(shape):
entry = offset[dim] if dim < len(offset) else 0
const = entry.constant if isinstance(entry, Var) else entry
- if not isinstance(const, int) or not isinstance(extent, int):
+ if not isinstance(extent, int):
continue
+ if extent <= 0:
+ raise ValueError(f"tile_view() shape dimension {dim} must be positive, got {extent}.")
+ if not isinstance(const, int):
+ continue
if const + extent > parent_shape[dim]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for dim, extent in enumerate(shape): | |
| entry = offset[dim] if dim < len(offset) else 0 | |
| const = entry.constant if isinstance(entry, Var) else entry | |
| if not isinstance(const, int) or not isinstance(extent, int): | |
| continue | |
| if const + extent > parent_shape[dim]: | |
| for dim, extent in enumerate(shape): | |
| entry = offset[dim] if dim < len(offset) else 0 | |
| const = entry.constant if isinstance(entry, Var) else entry | |
| if not isinstance(extent, int): | |
| continue | |
| if extent <= 0: | |
| raise ValueError(f"tile_view() shape dimension {dim} must be positive, got {extent}.") | |
| if not isinstance(const, int): | |
| continue | |
| if const + extent > parent_shape[dim]: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@warp/_src/builtins.py` around lines 4242 - 4247, Update the explicit shape
validation loop around shape, offset, and parent_shape to reject any integer
extent less than or equal to zero before checking const + extent against the
parent dimension. Preserve the existing handling of non-integer values and
upper-bound rejection, and add a regression test covering zero and negative
explicit extents passed through the tile path.
1 participant
Fixes #1745.
tile_view()already range-checks the origin, but nothing checked thatoffset + shapeactually fits in the source tile. Since the view just forms an aliased pointer into the parent storage, an extent that runs past the end of an axis was accepted and silently read, or wrote through, whatever shared memory follows. The slice form can't hit this because its extents get clamped to the parent, so it only affects the explicitshape=path.The check goes right after the existing rank check and mirrors the origin check just above it, same
Var.constantunwrapping and same message style. Only constant offsets and extents can be resolved at code-gen time; runtime values fall through to the debug-only bounds check as before.Testing
Added
test_tile_view_shape_oob_rejectednext to the existingtest_tile_view_offset_oob_rejected, covering a 1D overrun, a 2D overrun on the last axis, and a positive control where the view exactly reaches the end of every axis.Run on an RTX 3050 laptop GPU, so both cpu and cuda:0 were exercised:
test_tile_view: 96 passedtest_tile: 126 passedtest_tile_load: 30 passedtest_tile_reduce: 70 passedtest_tile_shared_memory: 32 passedtest_tile_mathdx: 6 passedruff checkandruff format --checkclean on both changed filesI also confirmed the new test actually catches the bug rather than just passing: reverting only the
builtins.pychange makes both overrun cases fail with "not raised" on cpu and cuda:0, since the out-of-bounds view is accepted today.One note on the positive control, it spells the shape with literals rather than
TILE_M - 1/TILE_N - 2. Arithmetic on module-level constants isn't folded at code-gen time, so the extent comes through unresolved andtile()fails onself.size *= s. That's pre-existing and unrelated to this change, it reproduces on an unpatched tree, but it did seem worth mentioning since it's a sharp edge if anyone writes a similar test.Summary by CodeRabbit
Bug Fixes
Tests