huggingface / huggingface/candle

Tensor::from_slice / from_raw_buffer return Ok when the declared shape does not match the storage element count

Open
#3,534 1 comment 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

`candle_core::Tensor::from_slice` (and `from_raw_buffer`, which routes through `from_slice` internally) does not validate that the declared `shape` matches the actual storage element count. The `ShapeWithOneHole` trait is designed for this purpose (the `into_shape(self, el_count)` method takes element count as a parameter), but the blanket impl ignores it:

```rust
// candle-core/src/shape.rs:474
impl> ShapeWithOneHole for S {
fn into_shape(self, _el_count: usize) -> Result {
Ok(self.into()) // <- _el_count parameter is ignored
}
}
```

As a result, `Tensor::from_slice(&[0.0], vec![10], &Device::Cpu)` returns `Ok` even though the storage has 1 element and the shape declares 10. The resulting tensor reports `shape.elem_count() == 10` while its actual storage holds 1 `f32`. Subsequent reads or iterations against the shape then read past the storage, surfacing as a `Vec` slice panic.

This is a contract bug in `from_slice` rather than an ONNX-specific issue. It is reachable from ordinary user code (e.g. constructing a tensor from any in-memory buffer with caller-supplied shape) and from any loader frontend that passes externally-derived shape and bytes through this API. `candle-onnx` is the simplest concrete trigger we have, but the surface is broader.

The current trigger via safe `Vec` indexing yields a panic, not undefined behavior. However, the lying tensor itself is constructed silently and any consumer that uses `get_unchecked`, `from_raw_parts`, or other shape-derived unsafe slicing would turn this into a true out-of-bounds read.

### Minimal reproducer (no ONNX, no external input)

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

fn main() {
let storage = vec![0.0f32]; // 1 element
let lying_shape = vec![1usize << 32]; // declares 2^32 elements
let t = Tensor::from_slice(&storage, lying_shape.as_slice(), &Device::Cpu).unwrap();
println!("declared elem_count = {}", t.shape().elem_count());
// declared elem_count = 4294967296

let _ = t.to_vec1::();
// panicked at candle-core/src/tensor.rs:1942:39:
// range end index 4294967296 out of range for slice of length 1
}
```

Smaller mismatches behave the same way. `Tensor::from_slice(&[0.0], vec![10], &Device::Cpu)` returns `Ok`, then `to_vec1::()` panics with `range end index 10 out of range for slice of length 1`. The bug is the missing contract check, not a magic threshold.

`from_raw_buffer` reaches the same lying-tensor state:

```rust
let raw = vec![0u8; 4]; // 1 f32 worth of bytes
let t = Tensor::from_raw_buffer(&raw, DType::F32, &[1usize << 30], &Device::Cpu).unwrap();
// declared elem_count = 1073741824, backed by 1 element
```

### ONNX trigger path (one concrete frontend)

`candle-onnx/src/eval.rs:192` `get_tensor` constructs the dim vector from `TensorProto.dims` and passes it to `Tensor::from_slice` without checking it against the embedded data buffer:

```rust
pub fn get_tensor(t: &onnx::TensorProto, name: &str) -> Result {
let dims: Vec = t.dims.iter().map(|&x| x as usize).collect();
...
Tensor::from_slice(&t.int64_data, dims.as_slice(), &Device::Cpu)
```

So an `.onnx` model that declares an initializer with `dims = [1 << 32]` and 1 actual element produces a lying tensor inside the loaded model. Downstream eval ops then trip the same OOB-slice panic when they iterate based on the declared shape. A 155-byte ONNX file with a `OneHot` node consuming such an initializer demonstrates this end-to-end:

```python
import onnx
from onnx import helper, TensorProto

indices = helper.make_tensor("indices", TensorProto.INT64, [1 << 32], [0]) # shape claims 2^32, storage has 1
depth = helper.make_tensor("depth", TensorProto.INT64, [], [1 << 32])
values = helper.make_tensor("values", TensorProto.FLOAT, [2], [0.0, 1.0])

node = helper.make_node("OneHot",
inputs=["indices", "depth", "values"],
outputs=["out"], axis=-1)

graph = onnx.GraphProto()
graph.name = "evil"
graph.initializer.extend([indices, depth, values])
graph.node.append(node)
out = helper.make_tensor_value_info("out", TensorProto.FLOAT, None)
graph.output.append(out)

model = helper.make_model(graph, opset_imports=[helper.make_opsetid("", 11)])
model.ir_version = 7
open("/tmp/evil.onnx", "wb").write(model.SerializeToString())
# total: 155 bytes
```

```rust
let model = candle_onnx::read_file("/tmp/evil.onnx")?;
let _ = candle_onnx::simple_eval(&model, std::collections::HashMap::new());
// panicked at candle-core/src/tensor.rs:1942:39
```

This is one path; any future loader that constructs tensors from caller-controlled (shape, bytes) tuples is exposed to the same root cause until the contract check is added in candle-core.

### Related observation in candle-onnx

While locating a concrete trigger, an intra-file inconsistency in `candle-onnx/src/eval.rs` showed up: the internal `get` impl for attribute tensors at L112 rejects negative `dims` entries (`if dim < 0 { bail!(...) }`), but `get_tensor` at L192 does the same `i64 -> usize` cast with no such check. Both eventually call into `Tensor::from_slice` / `from_raw_buffer` with the resulting `dims`, so the root candle-core fix is the load-bearing one, but the L192 path is worth aligning with L112 for the same defensive shape.

### Suggested fix

Two layers:

1. `candle-core` (root cause): the blanket `ShapeWithOneHole for S where S: Into` should validate the shape product matches `el_count`, or the impl should be removed and replaced with explicit-shape impls that accept a `&[D]` length argument. A non-breaking option is to add the check behind a `debug_assert` first, then promote to a hard error.

```rust
impl> ShapeWithOneHole for S {
fn into_shape(self, el_count: usize) -> Result {
let shape: Shape = self.into();
if shape.elem_count() != el_count {
crate::bail!(
"shape {shape:?} declares {} elements but storage has {el_count}",
shape.elem_count()
);
}
Ok(shape)
}
}
```

2. `candle-onnx` (additional defense): the intra-file inconsistency at `eval.rs:112` vs `eval.rs:192` should be resolved. The internal `get` impl rejects negative dims; `get_tensor` should do the same. Apply the same shape-element check before calling `Tensor::from_slice`.

Regression tests: the 155-byte PoC above should return `Err`, not panic. Additionally, a unit test should assert that `Tensor::from_slice(&[0.0], vec![10, 10], &Device::Cpu)` returns `Err` (1 element vs 100 declared).

### CWE

- CWE-1284 (Improper Validation of Specified Quantity in Input)
- CWE-125 (Out-of-bounds Read; latent. surfaced as panic by safe `Vec` indexing in this trigger path)

### CVE
If a CVE assignment is in scope here, happy to coordinate.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at candle-core/src/shape.rs:474 and trace how ShapeWithOneHole::into_shape is used by Tensor::from_slice and from_raw_buffer. Add regression coverage for mismatched storage and shape counts, then inspect candle-onnx/src/eval.rs:112 and :192 and run the 155-byte PoC to confirm malformed input returns Err rather than panicking.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
machine-learning, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.