huggingface / huggingface/candle

CUDA `copy_strided_src` copies past the end of an offset source view, silently corrupting `slice_scatter` (CPU disagrees)

Open Beginner friendly
#3,940 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
21k
Forks
1.8k
Avg merge
16h 42m
Merged PRs (30d)
25

Description

## Summary

On the CUDA backend, `copy_strided_src` sizes its copy from the **whole storage lengths** rather than from the source view. When the source is an offset view whose storage continues past the view (anything produced by `narrow`, which is stride-contiguous but starts partway into its parent), and the destination has room, it copies too many elements.

`Tensor::slice_scatter` is the visible casualty: scattering an offset source into position `p` also overwrites the elements **after** `p` with whatever follows the source in its own storage. No error is raised, and the CPU backend gives a different (correct) answer for the same call.

Observed on 0.11.0 and on current `main` (`d5fee525`); the code below is unchanged between them.

## The discrepancy

```rust
// candle-core/src/cpu_backend/mod.rs:902 -- copies exactly the view's length
StridedBlocks::SingleBlock { start_offset, len } =>
dst[dst_offset..dst_offset + len]
.copy_from_slice(&src[start_offset..start_offset + len])
```

```rust
// candle-core/src/cuda_backend/mod.rs:1208 -- derives the length from storage
fn slice_src_and_dst<'a, T>(
src: &'a CudaSlice,
src_l: &Layout,
dst: &'a mut CudaSlice,
dst_offset: usize,
) -> (cudarc::driver::CudaView<'a, T>, cudarc::driver::CudaViewMut<'a, T>) {
let src_offset = src_l.start_offset();
let to_copy = dst
.len()
.saturating_sub(dst_offset)
.min(src.len().saturating_sub(src_offset));
let src = src.slice(src_offset..src_offset + to_copy);
let dst = dst.slice_mut(dst_offset..dst_offset + to_copy);
(src, dst)
}
```

`to_copy` should be `src_l.shape().elem_count()`. It only equals that by accident, when the destination is no larger than the source view. That happens to hold for `Tensor::copy` / `try_clone`, which is presumably why this has gone unnoticed. It does not hold for `slice_scatter0`, where `dst` is the whole output and `dst_offset` points into it.

## Reproduction

```rust
use candle_core::{Device, Tensor};

fn main() -> candle_core::Result<()> {
for device in [Device::Cpu, Device::new_cuda(0)?] {
// Recipient [1, 4, 3].
let base = Tensor::new(
&[[[0f32, 1., 2.], [3., 4., 5.], [6., 7., 8.], [9., 10., 11.]]],
&device,
)?;
// Donor [4, 3]; take row 2 as a view, so the view starts at offset 6
// and its storage still holds rows 3 onwards.
let donor = Tensor::new(
&[
[90f32, 91., 92.],
[93., 94., 95.],
[96., 97., 98.],
[99., 100., 101.],
],
&device,
)?;
let row = donor.narrow(0, 2, 1)?.unsqueeze(0)?; // [1, 1, 3]

let out = base.slice_scatter(&row, 1, 2)?;
println!("{device:?}: {:?}", out.flatten_all()?.to_vec1::()?);
}
Ok(())
}
```

Only position 2 was asked for, so both devices should print

```
[0, 1, 2, 3, 4, 5, 96, 97, 98, 9, 10, 11]
```

Actual:

```
Cpu : [0, 1, 2, 3, 4, 5, 96, 97, 98, 9, 10, 11] correct
Cuda(CudaDevice(DeviceId(1))): [0, 1, 2, 3, 4, 5, 96, 97, 98, 99, 100, 101] position 3 clobbered
```

Position 3 has been overwritten with the donor's row 3, which the caller never referenced. Making the source own its storage (`Tensor::new`, or any op that allocates by `elem_count`) hides the bug; `.contiguous()` does **not**, because a `narrow` view is already contiguous and keeps its parent's storage.

`Tensor::cat` over the same narrowed pieces was tested alongside and is correct on both backends, so this appears reachable through `slice_scatter`'s use of the helper rather than through every caller of it.

## Suggested fix

```rust
let to_copy = src_l.shape().elem_count();
```

keeping the clamp against `dst.len() - dst_offset` if a defensive bound is still wanted. `slice_src_and_dst` is only reached from the `src_l.is_contiguous()` branch of `copy_strided_src`, where `elem_count` is exactly the number of elements the caller means to move.

## How it surfaced

Downstream, an activation-patching intervention overwrites one sequence position of a `[batch, seq_len, hidden]` residual stream with a row taken from another forward pass. That donor row is naturally a `narrow` view into a captured activation. On CUDA the patch also overwrote every position after the patch site.

The symptom was not a crash but a plausible number: a causal-tracing table reporting 100% recovery at every layer for every token position but the last, including the final layer, where patching a non-final position cannot affect the logits at all. Unit tests passed on CPU **and** CUDA, because they built the source with `Tensor::new`; only a donor row taken as a view reproduces it.

## Related

This is the mirror image of #3874, where the CPU backend read a view as though it were the whole storage and the GPU backends were correct. Here CUDA does it and the CPU is correct. It sits with the other recent view/offset fixes (#3735 / #3736, #3893 / #3894, #3853, #3836), which is the argument for fixing the shared helper rather than each call site.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in candle-core/src/cuda_backend/mod.rs at slice_src_and_dst and trace its call from copy_strided_src; compare its view handling with candle-core/src/cpu_backend/mod.rs. Run the supplied narrow-view slice_scatter reproduction on CPU and CUDA, then add or update regression coverage so CUDA matches CPU and leaves elements after the target position unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.