quantized: validate the raw GGML byte slice before reinterpreting it as blocks - #3963
apollo-2006 wants to merge 1 commit into
Conversation
…as blocks
`from_raw_data` turned a `&[u8]` into a `&[T]` with `slice::from_raw_parts` and
no checks at all:
let raw_data_ptr = raw_data.as_ptr();
let n_blocks = size_in_bytes / std::mem::size_of::<T>();
let data = unsafe { std::slice::from_raw_parts(raw_data_ptr as *const T, n_blocks) };
Two separate preconditions were missing, and `qtensor_from_ggml` is public and
safe, so both are reachable from safe code.
Length. `size_in_bytes` is derived from `dims`, never compared against
`raw_data.len()`. A caller passing a buffer shorter than the dims imply gets a
slice that runs past the end of the allocation:
error: Undefined Behavior: constructing invalid value of type
&[BlockQ4_0]: encountered a dangling reference (going beyond the bounds
of its allocation)
Alignment. A `&[u8]` carries 1-byte alignment while `&[T]` requires
`align_of::<T>()`. In a debug build this already aborts on the standard library
precondition check, so an unaligned caller crashes rather than silently
misbehaving. A zero-element tensor hits the same abort, because an empty
`Vec<u8>` has a dangling pointer of 0x1.
The length case is a genuine error, so it now bails. The alignment case is not:
what alignment a `Vec<u8>` ends up with is decided by the allocator, not the
caller, so rejecting an unaligned buffer would reject valid data. Under Miri,
which gives a `Vec<u8>` only the 1-byte alignment it actually guarantees, an
assertion rejects an ordinary well-formed tensor. So the slice is borrowed when
the buffer happens to be aligned and copied into an aligned `Vec<T>` when it is
not, which also removes the zero-element abort.
Copying is sound for these types for the same reason `GgmlType::zeros` is: the
ggml block types are plain data.
Adds regression tests. Without the fix, the unaligned and empty cases abort and
the truncated case is silently accepted; the aligned case is unaffected.
|
@ivarflakstad flagging one thing up front, since it is easy to read this PR as not doing what #3815 asked for. The issue proposes mirroring the assertions in Which leaves the part I cannot decide on my own. Do you want as_t_slice softened in this PR while the reasoning is in front of you, or kept as a follow-up so this one stays scoped to ggml_file.rs? |
Fixes #3815.
from_raw_dataincandle-core/src/quantized/ggml_file.rsturned a&[u8]into a&[T]with no checks:Two separate preconditions were missing.
qtensor_from_ggmlis public and safe, so both are reachable from safe code.1. Length is never checked (not in the issue)
size_in_bytesis computed fromdimsand never compared againstraw_data.len(). A buffer shorter than the dims imply produces a slice running past the end of the allocation. Miri:This one is a plain out-of-bounds read, so it is now rejected with an error.
2. Alignment, as reported
A
&[u8]carries 1-byte alignment while&[T]requiresalign_of::<T>(). Worth adding to the issue: this does not just fail under Miri. A debug build aborts on the standard library's own precondition check, so an unaligned caller crashes outright:A zero-element tensor hits the same abort today, because an empty
Vec<u8>has a dangling pointer of0x1, which is not aligned for the block type.Why this does not assert the alignment
The issue suggests mirroring the assertions in
quantized/mod.rs::as_t_slice. I started there, and it is not safe to do: the alignment aVec<u8>ends up with is decided by the allocator, not the caller, so rejecting an unaligned buffer rejects valid data.Miri makes this concrete, since it gives a
Vec<u8>only the 1-byte alignment it actually guarantees. With an assertion in place, an ordinary well-formed tensor is rejected:and in the same run a deliberately offset buffer came out aligned and was accepted. The alignment of a byte buffer simply is not something to branch correctness on.
So the slice is borrowed when the buffer happens to be aligned and copied into an aligned
Vec<T>when it is not. That is always correct, costs nothing on the usual path where the allocator over-aligns, and removes the zero-element abort as a side effect.Copying is sound for these types for the same reason
GgmlType::zerosis: the ggml block types are plain data.Worth flagging separately:
as_t_slicehas the same latent fragility, since itsassert_eq!on alignment would panic on valid data under such an allocator. I left it alone to keep this focused, happy to follow up if you want it changed.Tests
candle-core/tests/ggml_file_tests.rs, asserting on loaded contents rather than on which branch is taken, so they hold under either allocator. Against the unfixed code:unaligned_data_loads_correctlyaccepts_empty_tensorrejects_truncated_dataaligned_data_loads_correctly(control)Verified:
cargo test -p candle-core232 passed 0 failed,cargo clippy -p candle-core --all-targets -- -D warningsclean,cargo fmt --checkclean, andcargo +nightly miri test -p candle-core --test ggml_file_testsreports no undefined behaviour.