From c7246a5395d17651bf0e4fba27dadc69189afaa9 Mon Sep 17 00:00:00 2001 From: x14ngch3n Date: Thu, 13 Aug 2026 00:01:09 +0800 Subject: [PATCH 1/5] Bound GGUF metadata string/array values against the file mapping The tensor load path validates offset and byte size against the mmap'd file (check_tensor_in_file, #4179). The metadata path did not: set_mx_value_from_gguf read val->string.len / val->array.len straight from the file and passed them to std::string / array construction, so a crafted STRING or ARRAY metadata value could claim a length far larger than the file and force a read past the mapping (out-of-bounds read, SEGV / potential memory disclosure). gguf_get_key() performs no bounds checking of its own, and mlx does not use gguflib's bounded gguf_do_with_value walk, so nothing else caught this. Distinct from #4136/#4179 (tensor data offset), #3436 (gguflib asserts), and CVE-2025-62609. Add check_metadata_value_in_file() mirroring check_tensor_in_file, and bound each STRING and ARRAY metadata value (including each element of a string array) against the mapping before any copy. Lengths that would narrow badly to int are rejected before the static_cast. Reproduced under AddressSanitizer on main (4 MB over-read on a ~50-byte file at gguf.cpp:128 STRING and :155 ARRAY); after this change the same PoCs throw cleanly and a normal save/load round trip still succeeds. Co-Authored-By: Claude --- mlx/io/gguf.cpp | 98 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-) diff --git a/mlx/io/gguf.cpp b/mlx/io/gguf.cpp index 40cca573e5..d9cbf9b453 100644 --- a/mlx/io/gguf.cpp +++ b/mlx/io/gguf.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include "mlx/io/gguf.h" @@ -87,6 +88,29 @@ std::tuple extract_tensor_data(gguf_tensor* tensor) { return {buffer, float16}; } +// gguf_get_key() returns a pointer (val) into the mmap'd file but performs no +// bounds checking, and the metadata value lengths are read straight from the +// file. Without this guard a crafted STRING/ARRAY metadata value can claim a +// length far larger than the file and force std::string / array construction to +// read past the mapping (out-of-bounds read). The tensor path is bounded by +// check_tensor_in_file(); the metadata path needs the same invariant. +// +// `value_bytes` is the number of bytes the value occupies in the mapping +// (header + payload, computed per branch below). We ensure the whole region +// lies within [0, ctx->size). +void check_metadata_value_in_file( + const gguf_ctx* ctx, + const gguf_value* val, + size_t value_bytes) { + auto base = reinterpret_cast(val); + auto end = ctx->data + ctx->size; + if (base < ctx->data || base > end || value_bytes > static_cast(end - base)) { + throw std::runtime_error( + "[load_gguf] Metadata value extends past the end of the file. " + "Perhaps an incomplete download or corrupt file?"); + } +} + void set_mx_value_from_gguf( gguf_ctx* ctx, uint32_t type, @@ -123,21 +147,76 @@ void set_mx_value_from_gguf( case GGUF_VALUE_TYPE_BOOL: value = array(val->boolval, bool_); break; - case GGUF_VALUE_TYPE_STRING: - value = - std::string(val->string.string, static_cast(val->string.len)); + case GGUF_VALUE_TYPE_STRING: { + // Guard against an attacker-controlled length that exceeds the mapping. + // gguf_string is { uint64_t len; char string[] }; the static_cast + // below also narrows, so reject lengths that do not fit in an int. + uint64_t len = val->string.len; + if (len > static_cast(std::numeric_limits::max()) || + len > static_cast(ctx->size)) { + throw std::runtime_error( + "[load_gguf] String metadata value length exceeds file size."); + } + check_metadata_value_in_file( + ctx, val, sizeof(gguf_string) + static_cast(len)); + value = std::string(val->string.string, static_cast(len)); break; + } case GGUF_VALUE_TYPE_FLOAT64: value = array(val->float64, float32); break; case GGUF_VALUE_TYPE_ARRAY: { ctx->off += gguf_array_header_size; // Skip header char* data = reinterpret_cast(val) + gguf_array_header_size; - auto size = static_cast(val->array.len); + // The array length and element type are attacker-controlled. Bound the + // element count and the payload region against the file mapping before + // any array/string construction reads from `data`. + uint64_t arr_len = val->array.len; if (val->array.type == GGUF_VALUE_TYPE_ARRAY) { throw std::invalid_argument( "[load_gguf] Only supports loading 1-layer of nested arrays."); } + // Element byte size for the declared element type (0 = string/unknown, + // handled per-branch below). Avoids narrowing uint64 length to int + // before checking it fits the mapping. + size_t elt_size = 0; + switch (val->array.type) { + case GGUF_VALUE_TYPE_UINT8: + case GGUF_VALUE_TYPE_INT8: + case GGUF_VALUE_TYPE_BOOL: + elt_size = 1; + break; + case GGUF_VALUE_TYPE_UINT16: + case GGUF_VALUE_TYPE_INT16: + elt_size = 2; + break; + case GGUF_VALUE_TYPE_UINT32: + case GGUF_VALUE_TYPE_INT32: + case GGUF_VALUE_TYPE_FLOAT32: + elt_size = 4; + break; + case GGUF_VALUE_TYPE_UINT64: + case GGUF_VALUE_TYPE_INT64: + case GGUF_VALUE_TYPE_FLOAT64: + elt_size = 8; + break; + default: // GGUF_VALUE_TYPE_STRING and anything else: handled below + break; + } + if (elt_size > 0) { + // Overflow-safe payload size check: arr_len * elt_size <= remaining. + uint64_t remaining = static_cast(ctx->size) - + static_cast(reinterpret_cast(data) - ctx->data); + if (arr_len > remaining / elt_size) { + throw std::runtime_error( + "[load_gguf] Array metadata value extends past the end of the file."); + } + if (arr_len > static_cast(std::numeric_limits::max())) { + throw std::runtime_error( + "[load_gguf] Array metadata value length is too large."); + } + } + auto size = static_cast(arr_len); switch (val->array.type) { case GGUF_VALUE_TYPE_UINT8: value = array(reinterpret_cast(data), {size}, uint8); @@ -172,7 +251,18 @@ void set_mx_value_from_gguf( case GGUF_VALUE_TYPE_STRING: { std::vector strs(size); for (auto& str : strs) { + // Each element's length is attacker-controlled; bound it against + // the mapping before reading. + check_metadata_value_in_file(ctx, reinterpret_cast(data), 0); auto str_val = reinterpret_cast(data); + uint64_t slen = str_val->len; + if (slen > static_cast(std::numeric_limits::max())) { + throw std::runtime_error( + "[load_gguf] String array element length is too large."); + } + check_metadata_value_in_file( + ctx, reinterpret_cast(data), + sizeof(gguf_string) + static_cast(slen)); data += (str_val->len + sizeof(gguf_string)); str = std::string(str_val->string, static_cast(str_val->len)); ctx->off += (str_val->len + sizeof(gguf_string)); From ff577301b75f17586341dfd57f0716f97547b8b8 Mon Sep 17 00:00:00 2001 From: Xiang Chen Date: Thu, 13 Aug 2026 11:22:58 +0800 Subject: [PATCH 2/5] Centralize metadata bounds check in load_metadata Move check_metadata_value_in_file() out of set_mx_value_from_gguf and call it once per key in load_metadata(), mirroring check_tensor_in_file() on the tensor path (#4179). set_mx_value_from_gguf is back to reading value lengths straight from the file; all STRING/ARRAY bounds checking (fixed scalars, length-prefixed strings, fixed-size arrays, and each element of a string array) now lives in a single validator invoked before the value is consumed. Lengths that would not fit in the int the downstream array() / std::string constructors take are rejected there too. Adds "test gguf metadata value validation" covering valid empty/small strings plus OOB string, far-past-end string, fixed-size array, and string array element cases (ASAN, -O1). Co-Authored-By: Claude --- mlx/io/gguf.cpp | 219 ++++++++++++++++++++++++++----------------- tests/load_tests.cpp | 108 +++++++++++++++++++++ 2 files changed, 240 insertions(+), 87 deletions(-) diff --git a/mlx/io/gguf.cpp b/mlx/io/gguf.cpp index d9cbf9b453..90d7b82407 100644 --- a/mlx/io/gguf.cpp +++ b/mlx/io/gguf.cpp @@ -88,27 +88,137 @@ std::tuple extract_tensor_data(gguf_tensor* tensor) { return {buffer, float16}; } -// gguf_get_key() returns a pointer (val) into the mmap'd file but performs no -// bounds checking, and the metadata value lengths are read straight from the -// file. Without this guard a crafted STRING/ARRAY metadata value can claim a -// length far larger than the file and force std::string / array construction to -// read past the mapping (out-of-bounds read). The tensor path is bounded by -// check_tensor_in_file(); the metadata path needs the same invariant. -// -// `value_bytes` is the number of bytes the value occupies in the mapping -// (header + payload, computed per branch below). We ensure the whole region -// lies within [0, ctx->size). +// Mirror check_tensor_in_file(): gguf_get_key() leaves key.val / ctx->off +// pointing at the value but performs no bounds checking, and the value lengths +// are read straight from the file. Bound the whole value (and, for arrays, +// every element) against the mmap'd file once per key in load_metadata(), +// before set_mx_value_from_gguf consumes it. Lengths that would not fit in the +// int the downstream array() / std::string constructors take are rejected here. void check_metadata_value_in_file( const gguf_ctx* ctx, - const gguf_value* val, - size_t value_bytes) { - auto base = reinterpret_cast(val); + uint32_t type, + const gguf_value* val) { auto end = ctx->data + ctx->size; - if (base < ctx->data || base > end || value_bytes > static_cast(end - base)) { - throw std::runtime_error( - "[load_gguf] Metadata value extends past the end of the file. " - "Perhaps an incomplete download or corrupt file?"); + // Bytes available from a pointer up to the end of the mapping; 0 if the + // pointer lies outside [ctx->data, end]. + auto avail = [&](const uint8_t* p) -> size_t { + return (p < ctx->data || p > end) ? 0 : static_cast(end - p); + }; + auto base = reinterpret_cast(val); + + size_t fixed = 0; + switch (type) { + case GGUF_VALUE_TYPE_BOOL: + case GGUF_VALUE_TYPE_UINT8: + case GGUF_VALUE_TYPE_INT8: + fixed = 1; + break; + case GGUF_VALUE_TYPE_UINT16: + case GGUF_VALUE_TYPE_INT16: + fixed = 2; + break; + case GGUF_VALUE_TYPE_UINT32: + case GGUF_VALUE_TYPE_INT32: + case GGUF_VALUE_TYPE_FLOAT32: + fixed = 4; + break; + case GGUF_VALUE_TYPE_UINT64: + case GGUF_VALUE_TYPE_INT64: + case GGUF_VALUE_TYPE_FLOAT64: + fixed = 8; + break; + default: + break; + } + if (fixed) { + if (fixed > avail(base)) { + throw std::runtime_error( + "[load_gguf] Metadata value extends past the end of the file. " + "Perhaps an incomplete download or corrupt file?"); + } + return; + } + + // gguf_string = { uint64_t len; char string[] }. + if (type == GGUF_VALUE_TYPE_STRING) { + if (sizeof(uint64_t) > avail(base) || + val->string.len > static_cast(std::numeric_limits::max()) || + sizeof(uint64_t) + val->string.len > avail(base)) { + throw std::runtime_error( + "[load_gguf] String metadata value extends past the end of the file."); + } + return; + } + + // Array header = { uint32_t type; uint64_t len; } (gguf_array_header_size), + // followed by the elements. + if (type == GGUF_VALUE_TYPE_ARRAY) { + if (gguf_array_header_size > avail(base)) { + throw std::runtime_error( + "[load_gguf] Metadata value extends past the end of the file. " + "Perhaps an incomplete download or corrupt file?"); + } + if (val->array.len > static_cast(std::numeric_limits::max())) { + throw std::runtime_error( + "[load_gguf] Array metadata value length is too large."); + } + + size_t elt_size = 0; + switch (val->array.type) { + case GGUF_VALUE_TYPE_BOOL: + case GGUF_VALUE_TYPE_UINT8: + case GGUF_VALUE_TYPE_INT8: + elt_size = 1; + break; + case GGUF_VALUE_TYPE_UINT16: + case GGUF_VALUE_TYPE_INT16: + elt_size = 2; + break; + case GGUF_VALUE_TYPE_UINT32: + case GGUF_VALUE_TYPE_INT32: + case GGUF_VALUE_TYPE_FLOAT32: + elt_size = 4; + break; + case GGUF_VALUE_TYPE_UINT64: + case GGUF_VALUE_TYPE_INT64: + case GGUF_VALUE_TYPE_FLOAT64: + elt_size = 8; + break; + default: + break; + } + const uint8_t* elt = base + gguf_array_header_size; + if (elt_size) { + if (val->array.len > avail(elt) / elt_size) { + throw std::runtime_error( + "[load_gguf] Array metadata value extends past the end of the file."); + } + return; + } + // String array: each element is a length-prefixed string, so walk them. + if (val->array.type == GGUF_VALUE_TYPE_STRING) { + const uint8_t* p = elt; + for (uint64_t i = 0; i < val->array.len; i++) { + if (sizeof(uint64_t) > avail(p)) { + throw std::runtime_error( + "[load_gguf] Array metadata value extends past the end of the file."); + } + uint64_t slen = reinterpret_cast(p)->len; + if (slen > static_cast(std::numeric_limits::max()) || + sizeof(uint64_t) + slen > avail(p)) { + throw std::runtime_error( + "[load_gguf] Array metadata value extends past the end of the file."); + } + p += sizeof(uint64_t) + slen; + } + return; + } + // Unsupported element type (e.g. nested array): header is bounded; the + // consumer rejects the format without reading the payload. + return; } + + throw std::runtime_error("[load_gguf] Received unexpected type."); } void set_mx_value_from_gguf( @@ -147,76 +257,21 @@ void set_mx_value_from_gguf( case GGUF_VALUE_TYPE_BOOL: value = array(val->boolval, bool_); break; - case GGUF_VALUE_TYPE_STRING: { - // Guard against an attacker-controlled length that exceeds the mapping. - // gguf_string is { uint64_t len; char string[] }; the static_cast - // below also narrows, so reject lengths that do not fit in an int. - uint64_t len = val->string.len; - if (len > static_cast(std::numeric_limits::max()) || - len > static_cast(ctx->size)) { - throw std::runtime_error( - "[load_gguf] String metadata value length exceeds file size."); - } - check_metadata_value_in_file( - ctx, val, sizeof(gguf_string) + static_cast(len)); - value = std::string(val->string.string, static_cast(len)); + case GGUF_VALUE_TYPE_STRING: + value = + std::string(val->string.string, static_cast(val->string.len)); break; - } case GGUF_VALUE_TYPE_FLOAT64: value = array(val->float64, float32); break; case GGUF_VALUE_TYPE_ARRAY: { ctx->off += gguf_array_header_size; // Skip header char* data = reinterpret_cast(val) + gguf_array_header_size; - // The array length and element type are attacker-controlled. Bound the - // element count and the payload region against the file mapping before - // any array/string construction reads from `data`. - uint64_t arr_len = val->array.len; + auto size = static_cast(val->array.len); if (val->array.type == GGUF_VALUE_TYPE_ARRAY) { throw std::invalid_argument( "[load_gguf] Only supports loading 1-layer of nested arrays."); } - // Element byte size for the declared element type (0 = string/unknown, - // handled per-branch below). Avoids narrowing uint64 length to int - // before checking it fits the mapping. - size_t elt_size = 0; - switch (val->array.type) { - case GGUF_VALUE_TYPE_UINT8: - case GGUF_VALUE_TYPE_INT8: - case GGUF_VALUE_TYPE_BOOL: - elt_size = 1; - break; - case GGUF_VALUE_TYPE_UINT16: - case GGUF_VALUE_TYPE_INT16: - elt_size = 2; - break; - case GGUF_VALUE_TYPE_UINT32: - case GGUF_VALUE_TYPE_INT32: - case GGUF_VALUE_TYPE_FLOAT32: - elt_size = 4; - break; - case GGUF_VALUE_TYPE_UINT64: - case GGUF_VALUE_TYPE_INT64: - case GGUF_VALUE_TYPE_FLOAT64: - elt_size = 8; - break; - default: // GGUF_VALUE_TYPE_STRING and anything else: handled below - break; - } - if (elt_size > 0) { - // Overflow-safe payload size check: arr_len * elt_size <= remaining. - uint64_t remaining = static_cast(ctx->size) - - static_cast(reinterpret_cast(data) - ctx->data); - if (arr_len > remaining / elt_size) { - throw std::runtime_error( - "[load_gguf] Array metadata value extends past the end of the file."); - } - if (arr_len > static_cast(std::numeric_limits::max())) { - throw std::runtime_error( - "[load_gguf] Array metadata value length is too large."); - } - } - auto size = static_cast(arr_len); switch (val->array.type) { case GGUF_VALUE_TYPE_UINT8: value = array(reinterpret_cast(data), {size}, uint8); @@ -251,18 +306,7 @@ void set_mx_value_from_gguf( case GGUF_VALUE_TYPE_STRING: { std::vector strs(size); for (auto& str : strs) { - // Each element's length is attacker-controlled; bound it against - // the mapping before reading. - check_metadata_value_in_file(ctx, reinterpret_cast(data), 0); auto str_val = reinterpret_cast(data); - uint64_t slen = str_val->len; - if (slen > static_cast(std::numeric_limits::max())) { - throw std::runtime_error( - "[load_gguf] String array element length is too large."); - } - check_metadata_value_in_file( - ctx, reinterpret_cast(data), - sizeof(gguf_string) + static_cast(slen)); data += (str_val->len + sizeof(gguf_string)); str = std::string(str_val->string, static_cast(str_val->len)); ctx->off += (str_val->len + sizeof(gguf_string)); @@ -296,6 +340,7 @@ std::unordered_map load_metadata(gguf_ctx* ctx) { while (gguf_get_key(ctx, &key)) { std::string key_name = std::string(key.name, key.namelen); auto& val = metadata.insert({key_name, GGUFMetaData{}}).first->second; + check_metadata_value_in_file(ctx, key.type, key.val); set_mx_value_from_gguf(ctx, key.type, key.val, val); } return metadata; diff --git a/tests/load_tests.cpp b/tests/load_tests.cpp index 8974919476..f844f26359 100644 --- a/tests/load_tests.cpp +++ b/tests/load_tests.cpp @@ -257,6 +257,114 @@ TEST_CASE("test gguf tensor data offset validation") { } } +// Writes a metadata-only GGUF (no tensors) whose metadata KV section is +// `kv_section` verbatim, so a caller can encode values whose lengths exceed the +// file to exercise check_metadata_value_in_file(). `kv_count` must match the +// number of KV pairs encoded in `kv_section`. +void write_raw_gguf_metadata( + const std::string& path, + uint64_t kv_count, + const std::vector& kv_section) { + std::ofstream out(path, std::ios::binary); + auto u32 = [&out](uint32_t v) { + out.write(reinterpret_cast(&v), 4); + }; + auto u64 = [&out](uint64_t v) { + out.write(reinterpret_cast(&v), 8); + }; + out.write("GGUF", 4); + u32(3); // version + u64(0); // tensor_count + u64(kv_count); // metadata_kv_count + out.write(kv_section.data(), kv_section.size()); +} + +TEST_CASE("test gguf metadata value validation") { + // A STRING/ARRAY metadata value claiming a length larger than the file must + // be rejected rather than read past the end of the mapping. See PR #4212. + + auto append_string_kv = [](std::vector& b, + const std::string& key, + uint64_t claimed_len, + bool write_payload) { + auto put = [&](const void* p, size_t n) { + b.insert(b.end(), static_cast(p), static_cast(p) + n); + }; + uint64_t klen = key.size(); + put(&klen, 8); + put(key.data(), key.size()); + uint32_t vt = 8; // GGUF_VALUE_TYPE_STRING + put(&vt, 4); + put(&claimed_len, 8); + if (write_payload) { + b.insert(b.end(), claimed_len, '\0'); + } + }; + + auto append_array_kv = [](std::vector& b, + const std::string& key, + uint32_t elt_type, + uint64_t claimed_len) { + auto put = [&](const void* p, size_t n) { + b.insert(b.end(), static_cast(p), static_cast(p) + n); + }; + uint64_t klen = key.size(); + put(&klen, 8); + put(key.data(), key.size()); + uint32_t vt = 9; // GGUF_VALUE_TYPE_ARRAY + put(&vt, 4); + put(&elt_type, 4); + put(&claimed_len, 8); + }; + + SUBCASE("valid empty and small strings load") { + std::vector kv; + append_string_kv(kv, "empty", 0, false); + append_string_kv(kv, "small", 5, true); + std::string file_path = get_temp_file("test_gguf_meta_ok.gguf"); + write_raw_gguf_metadata(file_path, 2, kv); + auto [weights, metadata] = load_gguf(file_path); + CHECK(weights.empty()); + CHECK(std::get(metadata.at("empty")) == ""); + CHECK(std::get(metadata.at("small")) == std::string(5, '\0')); + } + + SUBCASE("string length extends past the end of the file") { + // Claims 100 bytes of payload, none of which are present. + std::vector kv; + append_string_kv(kv, "s", 100, false); + std::string file_path = get_temp_file("test_gguf_meta_str_past.gguf"); + write_raw_gguf_metadata(file_path, 1, kv); + CHECK_THROWS_AS(load_gguf(file_path), std::runtime_error); + } + + SUBCASE("string length far past the end of the file") { + std::vector kv; + append_string_kv(kv, "s", 1ull << 40, false); + std::string file_path = get_temp_file("test_gguf_meta_str_far.gguf"); + write_raw_gguf_metadata(file_path, 1, kv); + CHECK_THROWS_AS(load_gguf(file_path), std::runtime_error); + } + + SUBCASE("fixed-size array length extends past the end of the file") { + // GGUF_VALUE_TYPE_UINT8 = 0; claims 2^40 elements, none present. + std::vector kv; + append_array_kv(kv, "a", 0, 1ull << 40); + std::string file_path = get_temp_file("test_gguf_meta_arr_past.gguf"); + write_raw_gguf_metadata(file_path, 1, kv); + CHECK_THROWS_AS(load_gguf(file_path), std::runtime_error); + } + + SUBCASE("string array element length extends past the end of the file") { + // GGUF_VALUE_TYPE_STRING = 8; two elements, neither present. + std::vector kv; + append_array_kv(kv, "a", 8, 2); + std::string file_path = get_temp_file("test_gguf_meta_strarr_past.gguf"); + write_raw_gguf_metadata(file_path, 1, kv); + CHECK_THROWS_AS(load_gguf(file_path), std::runtime_error); + } +} + TEST_CASE("test gguf metadata") { std::string file_path = get_temp_file("test_arr.gguf"); using dict = std::unordered_map; From 8d7899d17f7d33fab3199e472285c5303488e86d Mon Sep 17 00:00:00 2001 From: Xiang Chen Date: Tue, 18 Aug 2026 00:54:49 +0800 Subject: [PATCH 3/5] Deduplicate metadata bounds check, drop redundant int_max string test - Extract gguf_value_type_size() shared by scalar and array element paths - Share string validation between single strings and string-array walks - Drop the string int_max checks: std::string now takes the uint64_t length directly instead of narrowing through int - Keep the array len int_max check: the downstream array() shape is int-valued Co-Authored-By: Claude --- mlx/io/gguf.cpp | 135 +++++++++++++++++++----------------------------- 1 file changed, 53 insertions(+), 82 deletions(-) diff --git a/mlx/io/gguf.cpp b/mlx/io/gguf.cpp index 90d7b82407..6c1a1663bb 100644 --- a/mlx/io/gguf.cpp +++ b/mlx/io/gguf.cpp @@ -88,65 +88,73 @@ std::tuple extract_tensor_data(gguf_tensor* tensor) { return {buffer, float16}; } -// Mirror check_tensor_in_file(): gguf_get_key() leaves key.val / ctx->off -// pointing at the value but performs no bounds checking, and the value lengths -// are read straight from the file. Bound the whole value (and, for arrays, -// every element) against the mmap'd file once per key in load_metadata(), -// before set_mx_value_from_gguf consumes it. Lengths that would not fit in the -// int the downstream array() / std::string constructors take are rejected here. -void check_metadata_value_in_file( - const gguf_ctx* ctx, - uint32_t type, - const gguf_value* val) { - auto end = ctx->data + ctx->size; - // Bytes available from a pointer up to the end of the mapping; 0 if the - // pointer lies outside [ctx->data, end]. - auto avail = [&](const uint8_t* p) -> size_t { - return (p < ctx->data || p > end) ? 0 : static_cast(end - p); - }; - auto base = reinterpret_cast(val); - - size_t fixed = 0; +// gguf_get_key() leaves key.val / ctx->off pointing at the value but performs +// no bounds checking, and the value lengths are read straight from the file. +// Bound the whole value (and, for arrays, every element) against the mmap'd +// file once per key in load_metadata(), before set_mx_value_from_gguf +// consumes it. Array lengths are also capped at int max because the +// downstream array() shape is int-valued. +size_t gguf_value_type_size(uint32_t type) { switch (type) { case GGUF_VALUE_TYPE_BOOL: case GGUF_VALUE_TYPE_UINT8: case GGUF_VALUE_TYPE_INT8: - fixed = 1; - break; + return 1; case GGUF_VALUE_TYPE_UINT16: case GGUF_VALUE_TYPE_INT16: - fixed = 2; - break; + return 2; case GGUF_VALUE_TYPE_UINT32: case GGUF_VALUE_TYPE_INT32: case GGUF_VALUE_TYPE_FLOAT32: - fixed = 4; - break; + return 4; case GGUF_VALUE_TYPE_UINT64: case GGUF_VALUE_TYPE_INT64: case GGUF_VALUE_TYPE_FLOAT64: - fixed = 8; - break; + return 8; default: - break; + return 0; } +} + +void check_metadata_value_in_file( + const gguf_ctx* ctx, + uint32_t type, + const gguf_value* val) { + auto end = ctx->data + ctx->size; + // Bytes available from a pointer up to the end of the mapping; 0 if the + // pointer lies outside [ctx->data, end]. + auto avail = [&](const uint8_t* p) -> size_t { + return (p < ctx->data || p > end) ? 0 : static_cast(end - p); + }; + auto base = reinterpret_cast(val); + auto fail = [](const char* what) { + throw std::runtime_error(std::string("[load_gguf] ") + what + + " Perhaps an incomplete download or corrupt file?"); + }; + + size_t fixed = gguf_value_type_size(type); if (fixed) { if (fixed > avail(base)) { - throw std::runtime_error( - "[load_gguf] Metadata value extends past the end of the file. " - "Perhaps an incomplete download or corrupt file?"); + fail("Metadata value extends past the end of the file."); } return; } - // gguf_string = { uint64_t len; char string[] }. + // gguf_string = { uint64_t len; char string[] }. Validate and return a + // pointer past the string. + auto check_string = [&](const uint8_t* p) -> const uint8_t* { + uint64_t len = reinterpret_cast(p)->len; + if (sizeof(uint64_t) + len > avail(p)) { + fail("String metadata value extends past the end of the file."); + } + return p + sizeof(uint64_t) + len; + }; + if (type == GGUF_VALUE_TYPE_STRING) { - if (sizeof(uint64_t) > avail(base) || - val->string.len > static_cast(std::numeric_limits::max()) || - sizeof(uint64_t) + val->string.len > avail(base)) { - throw std::runtime_error( - "[load_gguf] String metadata value extends past the end of the file."); + if (sizeof(uint64_t) > avail(base)) { + fail("String metadata value extends past the end of the file."); } + check_string(base); return; } @@ -154,44 +162,16 @@ void check_metadata_value_in_file( // followed by the elements. if (type == GGUF_VALUE_TYPE_ARRAY) { if (gguf_array_header_size > avail(base)) { - throw std::runtime_error( - "[load_gguf] Metadata value extends past the end of the file. " - "Perhaps an incomplete download or corrupt file?"); + fail("Metadata value extends past the end of the file."); } if (val->array.len > static_cast(std::numeric_limits::max())) { - throw std::runtime_error( - "[load_gguf] Array metadata value length is too large."); - } - - size_t elt_size = 0; - switch (val->array.type) { - case GGUF_VALUE_TYPE_BOOL: - case GGUF_VALUE_TYPE_UINT8: - case GGUF_VALUE_TYPE_INT8: - elt_size = 1; - break; - case GGUF_VALUE_TYPE_UINT16: - case GGUF_VALUE_TYPE_INT16: - elt_size = 2; - break; - case GGUF_VALUE_TYPE_UINT32: - case GGUF_VALUE_TYPE_INT32: - case GGUF_VALUE_TYPE_FLOAT32: - elt_size = 4; - break; - case GGUF_VALUE_TYPE_UINT64: - case GGUF_VALUE_TYPE_INT64: - case GGUF_VALUE_TYPE_FLOAT64: - elt_size = 8; - break; - default: - break; + fail("Array metadata value length is too large."); } const uint8_t* elt = base + gguf_array_header_size; + size_t elt_size = gguf_value_type_size(val->array.type); if (elt_size) { if (val->array.len > avail(elt) / elt_size) { - throw std::runtime_error( - "[load_gguf] Array metadata value extends past the end of the file."); + fail("Array metadata value extends past the end of the file."); } return; } @@ -200,18 +180,10 @@ void check_metadata_value_in_file( const uint8_t* p = elt; for (uint64_t i = 0; i < val->array.len; i++) { if (sizeof(uint64_t) > avail(p)) { - throw std::runtime_error( - "[load_gguf] Array metadata value extends past the end of the file."); + fail("Array metadata value extends past the end of the file."); } - uint64_t slen = reinterpret_cast(p)->len; - if (slen > static_cast(std::numeric_limits::max()) || - sizeof(uint64_t) + slen > avail(p)) { - throw std::runtime_error( - "[load_gguf] Array metadata value extends past the end of the file."); - } - p += sizeof(uint64_t) + slen; + p = check_string(p); } - return; } // Unsupported element type (e.g. nested array): header is bounded; the // consumer rejects the format without reading the payload. @@ -258,8 +230,7 @@ void set_mx_value_from_gguf( value = array(val->boolval, bool_); break; case GGUF_VALUE_TYPE_STRING: - value = - std::string(val->string.string, static_cast(val->string.len)); + value = std::string(val->string.string, val->string.len); break; case GGUF_VALUE_TYPE_FLOAT64: value = array(val->float64, float32); @@ -308,7 +279,7 @@ void set_mx_value_from_gguf( for (auto& str : strs) { auto str_val = reinterpret_cast(data); data += (str_val->len + sizeof(gguf_string)); - str = std::string(str_val->string, static_cast(str_val->len)); + str = std::string(str_val->string, str_val->len); ctx->off += (str_val->len + sizeof(gguf_string)); } value = std::move(strs); From 4e3bfb4c2f88d352756ded944ce21c0c86814722 Mon Sep 17 00:00:00 2001 From: Cheng Date: Tue, 18 Aug 2026 19:22:52 +0900 Subject: [PATCH 4/5] nit --- mlx/io/gguf.cpp | 203 ++++++++++++++++++++----------------------- tests/load_tests.cpp | 10 ++- 2 files changed, 100 insertions(+), 113 deletions(-) diff --git a/mlx/io/gguf.cpp b/mlx/io/gguf.cpp index 6c1a1663bb..6c27c46987 100644 --- a/mlx/io/gguf.cpp +++ b/mlx/io/gguf.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include "mlx/io/gguf.h" @@ -88,111 +87,6 @@ std::tuple extract_tensor_data(gguf_tensor* tensor) { return {buffer, float16}; } -// gguf_get_key() leaves key.val / ctx->off pointing at the value but performs -// no bounds checking, and the value lengths are read straight from the file. -// Bound the whole value (and, for arrays, every element) against the mmap'd -// file once per key in load_metadata(), before set_mx_value_from_gguf -// consumes it. Array lengths are also capped at int max because the -// downstream array() shape is int-valued. -size_t gguf_value_type_size(uint32_t type) { - switch (type) { - case GGUF_VALUE_TYPE_BOOL: - case GGUF_VALUE_TYPE_UINT8: - case GGUF_VALUE_TYPE_INT8: - return 1; - case GGUF_VALUE_TYPE_UINT16: - case GGUF_VALUE_TYPE_INT16: - return 2; - case GGUF_VALUE_TYPE_UINT32: - case GGUF_VALUE_TYPE_INT32: - case GGUF_VALUE_TYPE_FLOAT32: - return 4; - case GGUF_VALUE_TYPE_UINT64: - case GGUF_VALUE_TYPE_INT64: - case GGUF_VALUE_TYPE_FLOAT64: - return 8; - default: - return 0; - } -} - -void check_metadata_value_in_file( - const gguf_ctx* ctx, - uint32_t type, - const gguf_value* val) { - auto end = ctx->data + ctx->size; - // Bytes available from a pointer up to the end of the mapping; 0 if the - // pointer lies outside [ctx->data, end]. - auto avail = [&](const uint8_t* p) -> size_t { - return (p < ctx->data || p > end) ? 0 : static_cast(end - p); - }; - auto base = reinterpret_cast(val); - auto fail = [](const char* what) { - throw std::runtime_error(std::string("[load_gguf] ") + what + - " Perhaps an incomplete download or corrupt file?"); - }; - - size_t fixed = gguf_value_type_size(type); - if (fixed) { - if (fixed > avail(base)) { - fail("Metadata value extends past the end of the file."); - } - return; - } - - // gguf_string = { uint64_t len; char string[] }. Validate and return a - // pointer past the string. - auto check_string = [&](const uint8_t* p) -> const uint8_t* { - uint64_t len = reinterpret_cast(p)->len; - if (sizeof(uint64_t) + len > avail(p)) { - fail("String metadata value extends past the end of the file."); - } - return p + sizeof(uint64_t) + len; - }; - - if (type == GGUF_VALUE_TYPE_STRING) { - if (sizeof(uint64_t) > avail(base)) { - fail("String metadata value extends past the end of the file."); - } - check_string(base); - return; - } - - // Array header = { uint32_t type; uint64_t len; } (gguf_array_header_size), - // followed by the elements. - if (type == GGUF_VALUE_TYPE_ARRAY) { - if (gguf_array_header_size > avail(base)) { - fail("Metadata value extends past the end of the file."); - } - if (val->array.len > static_cast(std::numeric_limits::max())) { - fail("Array metadata value length is too large."); - } - const uint8_t* elt = base + gguf_array_header_size; - size_t elt_size = gguf_value_type_size(val->array.type); - if (elt_size) { - if (val->array.len > avail(elt) / elt_size) { - fail("Array metadata value extends past the end of the file."); - } - return; - } - // String array: each element is a length-prefixed string, so walk them. - if (val->array.type == GGUF_VALUE_TYPE_STRING) { - const uint8_t* p = elt; - for (uint64_t i = 0; i < val->array.len; i++) { - if (sizeof(uint64_t) > avail(p)) { - fail("Array metadata value extends past the end of the file."); - } - p = check_string(p); - } - } - // Unsupported element type (e.g. nested array): header is bounded; the - // consumer rejects the format without reading the payload. - return; - } - - throw std::runtime_error("[load_gguf] Received unexpected type."); -} - void set_mx_value_from_gguf( gguf_ctx* ctx, uint32_t type, @@ -305,22 +199,109 @@ void set_mx_value_from_gguf( } } +inline size_t gguf_value_type_size(uint32_t type) { + switch (type) { + case GGUF_VALUE_TYPE_BOOL: + case GGUF_VALUE_TYPE_UINT8: + case GGUF_VALUE_TYPE_INT8: + return 1; + case GGUF_VALUE_TYPE_UINT16: + case GGUF_VALUE_TYPE_INT16: + return 2; + case GGUF_VALUE_TYPE_UINT32: + case GGUF_VALUE_TYPE_INT32: + case GGUF_VALUE_TYPE_FLOAT32: + return 4; + case GGUF_VALUE_TYPE_UINT64: + case GGUF_VALUE_TYPE_INT64: + case GGUF_VALUE_TYPE_FLOAT64: + return 8; + default: + return 0; + } +} + +void check_metadata_value_in_file( + const gguf_ctx* ctx, + uint32_t type, + const gguf_value* val) { + auto end = ctx->data + ctx->size; + // Bytes available from a pointer up to the end of the mapping; 0 if the + // pointer lies outside [ctx->data, end]. + auto avail = [&](const uint8_t* p) -> size_t { + return (p < ctx->data || p > end) ? 0 : static_cast(end - p); + }; + auto base = reinterpret_cast(val); + auto fail = [](const char* what) { + std::ostringstream msg; + msg << "[load_gguf] " << what + << " Perhaps an incomplete download or corrupt file?"; + throw std::runtime_error(msg.str()); + }; + + size_t fixed = gguf_value_type_size(type); + if (fixed) { + if (fixed > avail(base)) { + fail("Metadata value extends past the end of the file."); + } + return; + } + + auto check_string = [&](const uint8_t* p) -> const uint8_t* { + uint64_t len = reinterpret_cast(p)->len; + if (sizeof(uint64_t) + len > avail(p)) { + fail("String metadata value extends past the end of the file."); + } + return p + sizeof(uint64_t) + len; + }; + + if (type == GGUF_VALUE_TYPE_STRING) { + if (sizeof(uint64_t) > avail(base)) { + fail("String metadata value extends past the end of the file."); + } + check_string(base); + return; + } + + if (type == GGUF_VALUE_TYPE_ARRAY) { + if (gguf_array_header_size > avail(base)) { + fail("Metadata value extends past the end of the file."); + } + const uint8_t* elt = base + gguf_array_header_size; + size_t elt_size = gguf_value_type_size(val->array.type); + if (elt_size) { + if (val->array.len > avail(elt) / elt_size) { + fail("Array metadata value extends past the end of the file."); + } + return; + } + if (val->array.type == GGUF_VALUE_TYPE_STRING) { + const uint8_t* p = elt; + for (uint64_t i = 0; i < val->array.len; i++) { + if (sizeof(uint64_t) > avail(p)) { + fail("Array metadata value extends past the end of the file."); + } + p = check_string(p); + } + } + return; + } + + throw std::runtime_error("[load_gguf] Received unexpected type."); +} + std::unordered_map load_metadata(gguf_ctx* ctx) { std::unordered_map metadata; gguf_key key; while (gguf_get_key(ctx, &key)) { + check_metadata_value_in_file(ctx, key.type, key.val); std::string key_name = std::string(key.name, key.namelen); auto& val = metadata.insert({key_name, GGUFMetaData{}}).first->second; - check_metadata_value_in_file(ctx, key.type, key.val); set_mx_value_from_gguf(ctx, key.type, key.val, val); } return metadata; } -// gguflib computes weights_data as ctx->data + ctx->data_off + the tensor's -// offset field in unsigned arithmetic, without comparing the result against the -// mapping, so a crafted offset can point outside the file or -- if the addition -// wraps -- back inside it at the wrong bytes. void check_tensor_in_file(const gguf_ctx* ctx, const gguf_tensor& tensor) { auto fail = [&tensor](const std::string& what) { std::ostringstream msg; diff --git a/tests/load_tests.cpp b/tests/load_tests.cpp index f844f26359..6ef7bc276e 100644 --- a/tests/load_tests.cpp +++ b/tests/load_tests.cpp @@ -288,7 +288,10 @@ TEST_CASE("test gguf metadata value validation") { uint64_t claimed_len, bool write_payload) { auto put = [&](const void* p, size_t n) { - b.insert(b.end(), static_cast(p), static_cast(p) + n); + b.insert( + b.end(), + static_cast(p), + static_cast(p) + n); }; uint64_t klen = key.size(); put(&klen, 8); @@ -306,7 +309,10 @@ TEST_CASE("test gguf metadata value validation") { uint32_t elt_type, uint64_t claimed_len) { auto put = [&](const void* p, size_t n) { - b.insert(b.end(), static_cast(p), static_cast(p) + n); + b.insert( + b.end(), + static_cast(p), + static_cast(p) + n); }; uint64_t klen = key.size(); put(&klen, 8); From af220c17ad0cc074b4aec2ae0ce6f291d193e245 Mon Sep 17 00:00:00 2001 From: Cheng Date: Wed, 19 Aug 2026 07:52:02 +0900 Subject: [PATCH 5/5] update_bypass_list is annoying giving every pr a pass --- .github/workflows/release.yml | 2 +- .github/workflows/update_bypass_list.yml | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d10493efff..b53f003ff4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,7 @@ on: required: false type: boolean schedule: - - cron: 33 6 * * 1-5 + - cron: 33 6 * * * # In jobs we must use |*publish| instead of |inputs.publish| because we can not # set default value for workflow_dispatch inputs reliably. diff --git a/.github/workflows/update_bypass_list.yml b/.github/workflows/update_bypass_list.yml index a129c51ce2..b9b71eee8b 100644 --- a/.github/workflows/update_bypass_list.yml +++ b/.github/workflows/update_bypass_list.yml @@ -5,8 +5,9 @@ on: workflow_dispatch: pull_request_target: types: - - opened - closed + schedule: + - cron: 33 6 * * * permissions: contents: write