Skip to content

Validate negative offsets in wp.copy#1553

Open
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix-copy-negative-offsets
Open

Validate negative offsets in wp.copy#1553
fallintoplace wants to merge 1 commit into
NVIDIA:mainfrom
fallintoplace:fix-copy-negative-offsets

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jun 12, 2026

Copy link
Copy Markdown

Summary

wp.copy() now rejects negative source and destination offsets before converting them to byte offsets or adding them to raw array pointers. Previously, negative offsets could pass the upper-bound checks and produce pointers before the start of the array allocation.

Validation

  • Built Warp locally with CPU support: python build_lib.py --no-cuda --no-use-libmathdx -j 8
  • Ran python -m unittest warp.tests.test_copy.TestCopy.test_copy_negative_offsets_cpu
  • Ran python -m unittest warp.tests.test_copy.TestCopy.test_copy_adjoint_cpu
  • Ran uvx ruff@0.15.9 check warp/_src/context.py warp/tests/test_copy.py
  • Ran uvx ruff@0.15.9 format --check warp/_src/context.py warp/tests/test_copy.py

Summary by CodeRabbit

  • Bug Fixes
    • Improved input validation in copy operations to explicitly reject negative offset values with clear error messages before processing begins.

@copy-pr-bot

copy-pr-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Enterprise

Run ID: 688e5b62-af41-4ede-a5f9-b3741f58430a

📥 Commits

Reviewing files that changed from the base of the PR and between 5df5550 and ec17029.

📒 Files selected for processing (2)
  • warp/_src/context.py
  • warp/tests/test_copy.py

📝 Walkthrough

Walkthrough

This PR adds input validation to the copy() function to reject negative src_offset and dest_offset values with explicit error messages, and introduces a test that verifies these errors are raised correctly across all configured devices.

Changes

Copy Offset Validation

Layer / File(s) Summary
Negative offset validation and tests
warp/_src/context.py, warp/tests/test_copy.py
copy() function now validates that both src_offset and dest_offset are non-negative, raising RuntimeError before proceeding with copy logic. A new test function verifies this validation raises the expected errors with specific messages, and the test is registered to run across all devices.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Validate negative offsets in wp.copy' directly and specifically describes the main change: adding validation for negative offsets in the wp.copy function.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
@fallintoplace fallintoplace force-pushed the fix-copy-negative-offsets branch from 316923b to ec17029 Compare June 12, 2026 17:17
@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds input validation to wp.copy() that rejects negative src_offset and dest_offset values before they can corrupt pointer arithmetic downstream. Without these checks, a negative offset would produce a byte-level pointer before the start of the array allocation and pass the existing upper-bound guards.

  • Two early-exit RuntimeError raises are added in warp/_src/context.py immediately after the array-type check, covering both the contiguous and non-contiguous copy paths.
  • A new test function test_copy_negative_offsets is registered for all available devices, verifying each offset independently via assertRaisesRegex.

Confidence Score: 4/5

Safe to merge; the change is a small, targeted defensive guard with no impact on the happy path.

Both modified files are straightforward. The two new checks in context.py are placed at the right point in the call flow — after the array-type guard and before any arithmetic on raw pointers — so they correctly protect both the contiguous memcpy path and, as a side-effect, the non-contiguous kernel path. The non-contiguous path has always silently dropped positive offset values too (that gap is pre-existing and out of scope for this PR). Tests cover negative src_offset and dest_offset independently and are registered across all available devices.

No files require special attention.

Important Files Changed

Filename Overview
warp/_src/context.py Adds two guard clauses rejecting negative src_offset/dest_offset before any arithmetic on raw array pointers; placement is correct and consistent with existing validation style.
warp/tests/test_copy.py Adds test_copy_negative_offsets covering negative src_offset and dest_offset independently across all registered devices; registration placement and pattern match existing adjoint test.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["wp.copy(dest, src, dest_offset, src_offset, count)"] --> B{src and dest\nare arrays?}
    B -- No --> C[RuntimeError:\nnot arrays]
    B -- Yes --> D{src_offset < 0?}
    D -- Yes --> E[RuntimeError:\nSource offset must be non-negative]
    D -- No --> F{dest_offset < 0?}
    F -- Yes --> G[RuntimeError:\nDestination offset must be non-negative]
    F -- No --> H{count <= 0?}
    H -- Yes --> I[count = src.size]
    H -- No --> J{count == 0?}
    I --> J
    J -- Yes --> K[return early]
    J -- No --> L{src.is_contiguous\nand dest.is_contiguous?}
    L -- Yes --> M[Compute byte offsets\nsrc_ptr = src.ptr + src_offset_in_bytes\ndst_ptr = dest.ptr + dst_offset_in_bytes]
    M --> N{Bounds checks\nin/out of range?}
    N -- Out of range --> O[RuntimeError:\nbounds exceeded]
    N -- In range --> P[memcpy via runtime core]
    L -- No --> Q[Non-contiguous kernel\noffsets not applied]
Loading

Comments Outside Diff (1)

  1. warp/_src/context.py, line 11280-11321 (link)

    P2 Non-contiguous path silently ignores offset parameters

    The non-contiguous copy path (reached when either array is strided, indexed, or fabric) never reads src_offset or dest_offset — it copies based on array shape and element size alone. As a result, passing any non-zero positive offset alongside non-contiguous arrays is silently ignored today; only the new negative-value guard (applied before both paths) will surface an error. A caller who passes src_offset=2 expecting a partial copy of a non-contiguous array will get a full-shape copy with no warning. This is a pre-existing gap, but worth documenting or guarding separately from the negative-offset fix.

Reviews (1): Last reviewed commit: "Validate copy offsets before pointer mat..." | Re-trigger Greptile

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant