Repository navigation
fix(ruvllm): refuse malformed GGUF alignment and tensor ranges - #1204
Merged
Merged
Conversation
A zero `general.alignment` no longer panics, and an oversized one no longer wraps the data offset in release builds; both are now an error when the file is opened. Tensor ranges are checked when a file is opened and again on every owned read, so a tensor whose offset or size runs past the end of the file, or whose offset arithmetic overflows, returns an error instead of panicking or reading the wrong bytes. Positive alignments, including ones that are not a power of two, are read as before. Refs ruvnet#1202
Contributor
Author
CI at this head (
|
Owner
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GgufFilenow returns an error for the malformed files described in #1202 instead of panicking or reading the wrong bytes. All changes are incrates/ruvllm/src/gguf/mod.rs.general.alignmentis converted tousizewith a checked conversion, and the offset rounding returnsRuvLLMError::Modelfor a zero alignment or when rounding overflows. Before this change, a zero alignment caused a division by zero, andu64::MAXoverflowed in debug builds and wrapped the data offset to 0 in release builds.openand in bothopen_mmapvariants. The owned reads inread_tensor_bytescheck again before allocating or slicing.Behaviour on the files from #1202
The example program from #1202, built at this commit with default features (debug and release give the same output):
openopen_mmapModel error: GGUF alignment must be nonzerou64::MAXModel error: GGUF alignment overflowModel error: GGUF tensor range outside file: test.weight (4224..4228, length 132)[1.0][1.0][1.0][1.0]On
main(see #1202), alignment 0 panics;u64::MAXpanics in debug builds and, in release builds, opens and returns the GGUF magic as the tensor's value; and the offset past the end panics throughopen_mmap(throughopenit already failed with a read error when the tensor was loaded).Tests
Linux x86_64, rustc 1.98.0, at this commit:
rustfmt --edition 2021 --check crates/ruvllm/src/gguf/mod.rscargo clippy --locked -p ruvllm --all-targets -- -W warningsgguf/mod.rs(none before either)cargo test --locked -p ruvllm --lib gguf::tests::--release--features mmap--release --features mmapSix new tests write small GGUF files and check, through both
openandopen_mmap: valid files with the default alignment, an explicit 32 and a non-power-of-two 3 still load; zero andu64::MAXalignments are refused; tensor offsets past the end of the file are refused at open and again on read; and tensor offsets and sizes whose arithmetic overflows are refused on read. Each of the seven new checks was disabled on its own (removed, or replaced with unchecked arithmetic) and its test failed; with the check restored the test passed.Notes
general.alignmentto be a non-zero power of two stored as a u32 (ggml/src/gguf.cpp, lines 622 to 636 at f2cc7282). I kept the existing behaviour of accepting any positive alignment, so no file that opens today is newly refused for its alignment. I can add the power-of-two rule if you prefer to match llama.cpp.tensor_data(the borrowed accessor) still slices without its own check. Tensor info read from the file is now checked when the file is opened. Info that a caller builds or modifies afterwards is not.Refs #1202