Skip to content

Keep gguflib input-validation asserts active in release builds#3436

Open
qflen wants to merge 2 commits intoml-explore:mainfrom
qflen:fix/gguf-enable-asserts
Open

Keep gguflib input-validation asserts active in release builds#3436
qflen wants to merge 2 commits intoml-explore:mainfrom
qflen:fix/gguf-enable-asserts

Conversation

@qflen
Copy link
Copy Markdown

@qflen qflen commented Apr 21, 2026

Summary

Fixes the GGUF out-of-bounds read reported in #3358, using the CMake-based approach suggested in the #3359 review thread (force-enable all gguflib asserts) rather than disabling them.

Why

gguflib relies on assert() to validate untrusted tensor headers: notably assert(tensor->ndim <= GGUF_TENSOR_MAX_DIM) in gguflib.c. Release builds define NDEBUG, so that assert compiles to a no-op and a crafted GGUF with ndim > 8 lets gguflib write past the fixed 8-element dim[] array into offset / bsize / weights_data / the caller's stack frame, after which MLX's get_shape() reads the same range and downstream code dereferences the attacker-controlled weights_data pointer.

The earlier PR #3359 attempted to address this with target_compile_definitions(gguflib PRIVATE NDEBUG), which defines NDEBUG instead of undefining it — so the asserts remained disabled. This PR uses -UNDEBUG to keep them live.

Changes

  • mlx/io/CMakeLists.txt: target_compile_options(gguflib PRIVATE -UNDEBUG) so gguflib's existing input-validation asserts stay in every build configuration.
  • mlx/io/gguf.cpp: defensive ndim > GGUF_TENSOR_MAX_DIM check in get_shape() as defense-in-depth for packagers that link a system gguflib or otherwise strip asserts.
  • tests/load_tests.cpp + tests/CMakeLists.txt: new TEST_CASE exercising the defensive get_shape() check; guarded by MLX_BUILD_GGUF and pulls the gguflib include path in via FetchContent_GetProperties.

Notes

  • The test covers the defensive MLX-side check directly (constructs a gguf_tensor with ndim = GGUF_TENSOR_MAX_DIM + 1 and expects std::runtime_error). The primary fix — gguflib's own assert() firing on malformed input — can't be covered by a doctest CHECK_* because it triggers abort(). I can add a death-style or subprocess test on request.
  • Upstream fix in Replace assert() with runtime check for tensor ndim bounds antirez/gguf-tools#22 is still the right long-term path; this change just ensures MLX isn't exposed while that sits.

Checklist

  • Read the CONTRIBUTING document.
  • Ran pre-commit run --all-files against the four changed files; clang-format and cmake-format passed.
  • Added a unit test for the defensive get_shape() bounds check in tests/load_tests.cpp. The primary gguflib abort() path is not covered — see Notes.
  • No user-facing documentation changes; MLX has no public docs page on GGUF internals that this touches.

gguflib.c relies on assert() to validate untrusted GGUF input
(e.g. tensor ndim <= GGUF_TENSOR_MAX_DIM). In release builds those
asserts are compiled out by -DNDEBUG, leaving MLX to parse and
dereference the corrupted tensor struct — get_shape() then reads past
the fixed 8-element dim[] array, and downstream code uses clobbered
offset/weights_data fields.

- Force -UNDEBUG on the gguflib target so the asserts stay live in
  every build configuration.
- Add a defensive bounds check in get_shape() so the MLX side also
  refuses oversized ndim if the assert is ever disabled by a packager
  or replaced by a system gguflib.

Closes part of ml-explore#3358.
Copy link
Copy Markdown
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Thanks for continuing the work! I think you are doing the right fix in this PR.

Comment thread mlx/io/gguf.cpp Outdated
qflen added a commit to qflen/mlx that referenced this pull request Apr 24, 2026
zcbenz pointed out on ml-explore#3436 that the defense-in-depth ndim bounds
check in get_shape() is unnecessary: with -UNDEBUG keeping gguflib's
own assert live, a tensor with ndim > GGUF_TENSOR_MAX_DIM aborts
inside gguflib before get_shape() is ever called. Remove the check
and its unit test (which was the only reason tests/ needed to pull
in the gguflib include path).
@qflen qflen force-pushed the fix/gguf-enable-asserts branch 3 times, most recently from cd8b6d3 to 0de7fb2 Compare April 24, 2026 00:37
Copy link
Copy Markdown
Collaborator

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Thanks!

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