Skip to content

quantized: fix UAF in as_t_slice when called with Cow::Owned - #3493

Merged
ivarflakstad merged 4 commits into
huggingface:mainfrom
tomsanbear:fix/quantized-from-data-cow-owned-uaf
Jun 18, 2026
Merged

ivarflakstad merged 4 commits into
huggingface:mainfrom
tomsanbear:fix/quantized-from-data-cow-owned-uaf

Conversation

@tomsanbear

@tomsanbear tomsanbear commented Apr 25, 2026 •

Copy link
Copy Markdown
Collaborator

What's broken

as_t_slice (candle-core/src/quantized/mod.rs:43) takes Cow<'_, [u8]> by value and returns a &[T] whose elided lifetime is tied to that input. With Cow::Owned, the inner Vec drops at the helper's exit and the returned slice dangles. Both pub fn from_data paths route through it (QStorage::from_data, GgmlDType::from_data), so they UAF on every backend — CPU's .to_vec() and Metal/CUDA's GPU upload both read the dangling slice.

The borrow checker doesn't catch it because Cow<'a, T>'s lifetime parameter only describes the borrow inside the Borrowed variant; for Owned it's unconstrained.

No in-tree caller currently triggers this (the GGUF loaders bypass these APIs entirely), but they're pub, so any downstream user reaching for Cow::Owned silently corrupts their model weights.

How I ran into this

While extending Metal kernel test coverage post-#3477, I wrote a CPU-vs-Metal dequant equivalence test that quantized on CPU, copied the packed bytes into a Vec<u8>, wrapped them in Cow::Owned, and reloaded onto Metal via QStorage::from_data. The two dequant outputs disagreed by max |Δ| = 1.0625. Tracing showed the GPU buffer held freed memory by the time of the upload — switching to Cow::Borrowed(&bytes) made the test pass bit-equivalently. Same shape on the CPU branch's .to_vec().

Fix

as_t_slice now takes &[u8]. Both pub fn from_data keep their Cow<'_, [u8]> parameter — they bind let data: &[u8] = &data; and pass the borrow through, which keeps the inner Vec alive in the public function's frame for the whole call.

No public signature changes. No allocs added, no memcpys added. Cow::Borrowed paths compile identically.

Tests

Two new tests in candle-core/tests/quantized_tests.rs:

  1. from_data_dequant_matches_canonical_when_caller_passes_cow_owned — dispatched via test_device! to CPU + CUDA + Metal. Quantizes on CPU, round-trips packed bytes through QStorage::from_data(Cow::Owned(bytes), device, …), asserts dequant matches the canonical CPU dequant. Pre-fix _cpu and _metal fail with max |Δ| ranging from 1.0 to ~5e5 across runs (depends on what the allocator hands the freed page). Post-fix: green on both. CUDA path is fixed by the same change but I couldn't run it locally.

  2. from_data_with_cow_owned_must_not_read_freed_memory_under_miri — #[cfg(miri)]-gated. Run with cargo +nightly miri test -p candle-core --test quantized_tests. On unpatched candle Miri reports:

    error: Undefined Behavior: constructing invalid value:
    encountered a dangling reference (use-after-free)
        --> candle-core/src/quantized/mod.rs:348:36
         |
     348 |   Self::Q4_0 => Box::new(as_t_slice::<BlockQ4_0>(data).to_vec()),
         |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
    

    With the fix: passes.

cargo test --features metal -p candle-core is fully green (294 tests).

Notes

  • A cleaner alternative is to drop Cow from from_data's public signature entirely (fn from_data(data: &[u8], …)). Same correctness, semver-breaking. Happy to do that as a follow-up if preferred.
  • The behavioral test is reliable on macOS Apple Silicon (100% repro across many runs) but could be flaky on other allocators. The Miri test is the deterministic backstop.

`as_t_slice` (candle-core/src/quantized/mod.rs:43) took `Cow<'_, [u8]>`
by value and returned a `&[T]` whose elided lifetime was tied to that
input. With `Cow::Owned`, the inner Vec dropped at the helper's exit
and the returned slice dangled. Both `pub fn from_data` paths route
through it (`QStorage::from_data`, `GgmlDType::from_data`), so they
UAF on every backend — CPU's `.to_vec()` and Metal/CUDA's GPU upload
both read the dangling slice.

Make `as_t_slice` take `&[u8]`. Both `pub fn from_data` keep their
`Cow<'_, [u8]>` parameter — they bind `let data: &[u8] = &data;` and
pass the borrow through, which keeps the inner Vec alive in the public
function's frame. No public signature changes; no allocs or memcpys
added.

Found while extending Metal kernel test coverage post-huggingface#3477: a
CPU-vs-Metal dequant equivalence test using `Cow::Owned` produced
`max |Δ| = 1.0625` on Q4_0; switching to `Cow::Borrowed(&bytes)` made
the divergence vanish.

Adds two regression tests in candle-core/tests/quantized_tests.rs:

- `from_data_dequant_matches_canonical_when_caller_passes_cow_owned`,
  dispatched via `test_device!` to CPU + CUDA + Metal. Pre-fix `_cpu`
  and `_metal` failed with `max |Δ|` ranging from 1.0 to ~5e5 across
  runs; post-fix both pass.
- `from_data_with_cow_owned_must_not_read_freed_memory_under_miri`,
  `#[cfg(miri)]`-gated. Run with
  `cargo +nightly miri test -p candle-core --test quantized_tests`.
  On unpatched candle Miri reports "Undefined Behavior: dangling
  reference (use-after-free)" at the `.to_vec()` callsite; with the
  fix it passes.

@ivarflakstad ivarflakstad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

If we could make the comments more concise / keep only the what we need that would be excellent 👍

Per @ivarflakstad on huggingface#3493: keep only what we need. The signature
change and the behavioral regression test pin the contract; the
PR description carries the UAF analysis. Drop the Miri-only test
and hoist `use std::borrow::Cow` to the top of the test file.
@tomsanbear

Copy link
Copy Markdown
Collaborator Author

Great catch!

If we could make the comments more concise / keep only the what we need that would be excellent 👍

Thanks! Updated!

@ivarflakstad ivarflakstad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👌

@ivarflakstad
ivarflakstad merged commit c1e6756 into huggingface:main Jun 18, 2026
11 checks passed
FerrisMind pushed a commit to FerrisMind/candle that referenced this pull request Jun 23, 2026
…gingface#3493)

* quantized: fix UAF in `as_t_slice` when called with `Cow::Owned`

* quantized: drop explanatory comments and Miri test

---------

Co-authored-by: ivarflakstad <69173633+ivarflakstad@users.noreply.github.com>
michaeltrefry added a commit to SceneWorks/candle-llm that referenced this pull request Jul 2, 2026
… fix) (#46)

Lockstep bump with candle-gen. Upstream PR huggingface/candle#3493 fixes the
use-after-free in as_t_slice/QStorage::from_data(Cow::Owned) that candle-gen's
sc-9085 quant spike hit (silent garbage quantized weights). The 4-commit range
also carries three Metal-only SDPA fixes; no API changes, candle-kernels is
byte-identical across the range. CPU test suite green at the new rev.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
michaeltrefry added a commit to SceneWorks/candle-gen that referenced this pull request Jul 2, 2026
…(upstream Cow::Owned UAF fix) (#222)

Bump the workspace candle pin to pick up upstream huggingface/candle#3493
(commit c1e6756a89), the use-after-free fix in as_t_slice /
QStorage::from_data(Cow::Owned...) that the sc-9085 quant spike hit: the
helper took the Cow by value and returned a slice into its dropped buffer,
silently corrupting quantized weights. Our Cow::Borrowed workaround stays
(zero-cost), but the footgun is gone at the source.

The 4-commit range (65ecb58c..c1e6756a89) is the fix plus three Metal-only
SDPA fixes: no API changes, and candle-kernels/ is byte-identical across it,
so the sc-7544 vendored fatbin fork needs NO re-vendor (VENDORED.md updated
to record the new rev and why).

candle-llm re-pinned d0ba3e6 -> 3d9fdf0 in lockstep (candle-llm #46 carries
the matching candle bump) so cargo unifies ONE candle-core across the graph;
the intermediate d0ba3e6..3d9fdf0 candle-llm churn is the sc-8528 KV-quant
backout + docs, and candle-gen-joycaption links no candle-llm symbols.

Validation: full CUDA gate (scripts/check-cuda.ps1) green at the new revs on
sm_120 - 115 suites, 0 failures, including cuda_quant_smoke (the Blackwell
quant-fatbin guard). candle-llm's own CPU suite green in #46. The gate ran
against the pre-squash branch commit 1b3bc2f; the squash-merge 3d9fdf0 has
the IDENTICAL git tree (543b33bf), so the result carries over exactly.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
SorenDreano pushed a commit to SorenDreano/candle that referenced this pull request Jul 9, 2026
…gingface#3493)

* quantized: fix UAF in `as_t_slice` when called with `Cow::Owned`

* quantized: drop explanatory comments and Miri test

---------

Co-authored-by: ivarflakstad <69173633+ivarflakstad@users.noreply.github.com>
Abhinav5132 pushed a commit to Abhinav5132/candle that referenced this pull request Jul 24, 2026
…gingface#3493)

* quantized: fix UAF in `as_t_slice` when called with `Cow::Owned`

* quantized: drop explanatory comments and Miri test

---------

Co-authored-by: ivarflakstad <69173633+ivarflakstad@users.noreply.github.com>
achmadk pushed a commit to achmadk/candle that referenced this pull request Sep 13, 2026
…gingface#3493)

* quantized: fix UAF in `as_t_slice` when called with `Cow::Owned`

* quantized: drop explanatory comments and Miri test

---------

Co-authored-by: ivarflakstad <69173633+ivarflakstad@users.noreply.github.com>
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.

2 participants