huggingface / huggingface/candle

Shape::elem_count() silently wraps in release, producing a Tensor with corrupted metadata (dims disagree with backing storage)

Open
#3,816 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
Loading a GGUF file with attacker/model-controlled dimensions that overflow `usize` (e.g. `[2^32, 2^32]`) causes `Shape::elem_count()` to silently wrap to a small value while `Shape::dims()` continues to report the original (huge) dimensions. The result is a `Tensor`/`QTensor` whose reported shape and actual backing storage disagree — a silently corrupted tensor, not a crash and not (as far as I can tell) an out-of-bounds access.

Not filed as a security report: every code path I could find that actually touches the mismatched data is bounds-checked and panics before any memory access happens (see "Why it never becomes unsafe" below), so I believe this is contained. Filing as a correctness/robustness issue because the corrupted `Tensor` state itself is visible and surprising.

## Root cause
`src/shape.rs:131`
```rust
pub fn elem_count(&self) -> usize {
self.0.iter().product() // unchecked usize product
}
```
This wraps in `usize` with no overflow check. **The wrap only exists in release builds** — a debug build panics at the multiply (`attempt to multiply with overflow`), so this is a release-only defect that debug testing will never catch.

## Reproduction
A 384-byte GGUF file declaring a tensor with `dims = [4294967296, 4294967296]`:
```
declared dims = [4294967296, 4294967296]
Shape::elem_count() = 0 <- wrapped (2^32 * 2^32 mod 2^64 = 0)
QTensor accepted: storage_size_in_bytes = 0
dequantize() -> Ok
Tensor reports dims=[4294967296, 4294967296] but elem_count=0
```
The file is accepted, candle allocates 0 bytes, and hands back a `Tensor` claiming ~4.3 billion × 4.3 billion elements over an empty backing store. `dequantize()` returns `Ok` rather than an error.

The desync propagates through ordinary ops that all return `Ok` on the corrupted tensor: `flatten_all`, `t()`, `contiguous()`, `copy()`, `narrow()`, `reshape()`, `affine()`, `matmul()`. Sharpest case — `narrow()` followed by `contiguous()`:
```
base: dims=[2^32, 2^32] elem_count=0
narrowed: dims=[1, 2^32] elem_count=4294967296 <- no longer zero
contiguous() -> dims=[1, 4294967296] elem_count=4294967296
real storage len = 0 bytes
peak RSS ~1.9 MB <- no 16 GiB allocation ever happened
```
`narrow()` is the interesting step because it defeats the `elem_count() == 0` guard elsewhere: the narrowed view's element count no longer wraps to zero, even though the storage behind it is still empty.

## Why it never becomes unsafe (three independent reasons, so I'm not filing this as a vulnerability)
1. `elem_count()` is used for *both* buffer sizing and empty-tensor short-circuits (e.g. `tensor.rs:1516`: `if c_shape.elem_count() == 0 || k == 0 { return Tensor::zeros(...) }`), so the same wrap that corrupts the shape also routes most ops onto a no-op path. `matmul` never reaches gemm on the base (unwrapped-view) case.
2. Every CPU backend path that actually reads/writes the mismatched data panics on a Rust slice bounds check before any access — e.g. `narrowed.affine(...)` panics at `cpu_backend/utils.rs:331` with "range end index 4294967296 ... length 0", not a memory fault.
3. Confirmed with Miri (`-C overflow-checks=off` to force release semantics, `-Zmiri-disable-alignment-check` to isolate from an unrelated alignment issue I filed separately, #3815): no out-of-bounds access, no aliasing/provenance UB, across the read -> dequantize -> narrow -> contiguous -> flatten -> transpose -> matmul chain.

## Suggested fix
`checked_product` (or fold with `checked_mul`, returning a `Result`/erroring shape) in `Shape::elem_count()`, or reject a `Shape` at construction time whose dimension product overflows `usize`. Either closes the silent-corruption case; I don't think a panic-on-overflow is even necessary given containment above, but a `Result`-returning path would let GGUF loaders reject the file cleanly instead of accepting a tensor with impossible metadata.

Happy to share the crafted GGUF + a Miri test harness if useful.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at src/shape.rs:131 and trace callers of Shape::elem_count(), especially tensor.rs:1516 and the GGUF loading path. Review cpu_backend/utils.rs:331 and the supplied overflow reproduction to understand the current bounds-check behavior. Done means overflowing dimensions are rejected or reported as an error instead of producing a tensor whose metadata disagrees with its storage, with regression coverage for release semantics.

Written by the indexing model from the issue text.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.