Validate GGUF tensor dimensions - #4378
Open
roshaninfordham wants to merge 1 commit into
Open
Conversation
roshaninfordham
added a commit
to roshaninfordham/opensource-contributions
that referenced
this pull request
Aug 23, 2026
A portfolio of pull requests is a list of links; what is worth showing is the reasoning. Each contribution gets a writeup with the same five sections: what was broken and its bug class, how it was reproduced with the evidence pasted verbatim, what changed and what was deliberately left out, how it was verified with real counts, and what transferred to the next problem. Exactly two things are hand-maintained: those writeups, and the project list in data/projects.yml. Everything else is pulled live -- pull request state, size, commits, review comments, days to merge, repository language, stars, npm version, monthly downloads. A number written by hand is wrong the day after it is written. A scheduled workflow re-syncs every six hours and commits only on a real change. The search is scoped with is:public. A local token with repo scope can see private repositories, and this data file is committed to a public repo. Contributions to projects I do not maintain are counted separately from my own repositories, and only the former appear on the landing page. Combining them would report a larger number that anyone clicking through would discount. Charts are standalone SVG with no dependencies, since GitHub markdown strips styles and scripts, and every colour has to carry on both themes. AGENTS.md documents the purpose, the structure, the writeup format, and the rules for any agent working here. First writeup: ml-explore/mlx#4378, a heap out-of-bounds write in the GGUF quantized loader.
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.
Fixes #4244 (thanks @perparimmjeku for the report).
The bug
GGUF stores tensor dimensions as 64-bit values, while MLX represents dimensions with
ShapeElem, which isint32_t.Previously,
get_shape()copied GGUF dimensions directly into the MLX shape, allowing a 64-bit dimension to narrow to a different 32-bit value. In the quantized loader, this could make the allocation size smaller than the number of elements subsequently processed, resulting in a heap-buffer-overflow during tensor extraction.The dimension product could also wrap in 64-bit arithmetic, causing the MLX shape and the tensor's element count to describe different sizes.
Reproducing it
The regression tests cover:
{32}as a valid quantized tensor dimension.(1ull << 32) + 32to test a dimension larger than theShapeElemrange.1ull << 31to test a value that would become negative when represented as signedint32_t.{96, 384307168202282326ull}to produce a dimension product that wraps in 64-bit arithmetic.{}to test a quantized tensor with no dimensions.On unmodified
main, the crafted dimension-product case produces:The allocation size is derived from a 32-bit accumulator, while the extraction loop operates on the full shape-derived element count. This allows the loop to write beyond the allocated buffer.
The same underlying issue is present in the Q4_0 and Q4_1 extraction paths.
The fix
mlx/io/gguf.cpp<limits>.tensor_error()to centralize tensor validation errors.get_shape()now validates every GGUF dimension against the maximum representableShapeElemvalue before narrowing.uint64_tmaximum divided by the next dimension.tensor_error()for the existing tensor file-boundary validation.The validation is performed before narrowing or multiplying, so malformed dimensions cannot silently produce a different MLX shape or a wrapped element count.
mlx/io/gguf_quants.cppstd::accumulate()initial values from1tosize_t{1}.The
std::accumulate()change is important because its accumulator type is deduced from the initial value, not from the binary operation. With the literal1, the accumulator is anint, even though the operation isstd::multiplies<size_t>. This causes intermediate products to be converted back to 32-bitint. Usingsize_t{1}keeps the accumulation in the intended width.I did not add allocator-failure checks in this change. Once GGUF dimensions are validated, an allocation of genuinely enormous size requires a correspondingly enormous input file; handling general out-of-memory conditions is outside the scope of this fix.
Why not compare against
tensor.num_weights?tensor.num_weightsis the 64-bit product maintained by gguflib, but comparing a newly calculated product against it would not solve the representation problem.MLX ultimately stores dimensions as
ShapeElem, which isint32_t, so every individual dimension must first be proven representable in that type. The multiplication must then be proven safe before calculating the product.The fix therefore validates the GGUF inputs before narrowing and before multiplication, rather than relying on a potentially narrowed shape or a wrapped product.
Prior art
This PR addresses a different issue: malformed tensor dimensions and dimension-product arithmetic can cause the MLX shape and element count to disagree, leading to unsafe allocation and indexing behavior in the quantized loader.
Testing
Added
TEST_CASE("test gguf tensor dimension validation")intests/load_tests.cpp, covering six cases.Built CPU-only with AddressSanitizer:
On unmodified
main, the new dimension-product test triggers the heap-buffer-overflow shown above.With this change, the whole C++ suite passes under ASAN:
Python load and quantized tests:
uvx pre-commit run --allis clean.The reporter's four PoC files are also rejected by the loader with:
Checklist
Put an
xin the boxes that apply.pre-commit run --all-filesto format my code / installed pre-commit prior to committing changes