diff --git a/common/imatrix-loader.cpp b/common/imatrix-loader.cpp index efe9aecee..71d3b500f 100644 --- a/common/imatrix-loader.cpp +++ b/common/imatrix-loader.cpp @@ -102,7 +102,8 @@ bool common_imatrix_load(const std::string & fname, common_imatrix & imatrix) { const int64_t chunk_count_key = gguf_find_key(ctx_gguf, LLM_KV_IMATRIX_CHUNK_COUNT); const int64_t chunk_size_key = gguf_find_key(ctx_gguf, LLM_KV_IMATRIX_CHUNK_SIZE); - if (datasets_key != -1 && gguf_get_arr_type(ctx_gguf, datasets_key) == GGUF_TYPE_STRING) { + if (datasets_key != -1 && gguf_get_kv_type(ctx_gguf, datasets_key) == GGUF_TYPE_ARRAY && + gguf_get_arr_type(ctx_gguf, datasets_key) == GGUF_TYPE_STRING) { const int64_t n = gguf_get_arr_n(ctx_gguf, datasets_key); imatrix.datasets.reserve(imatrix.datasets.size() + n); for (int64_t i = 0; i < n; ++i) { @@ -143,6 +144,13 @@ bool common_imatrix_load(const std::string & fname, common_imatrix & imatrix) { return false; } + if (in_sum2->type != GGML_TYPE_F32 || counts->type != GGML_TYPE_F32) { + LOG_ERR("%s: sums and counts for %s must be F32\n", __func__, name.c_str()); + gguf_free(ctx_gguf); + ggml_free(ctx); + return false; + } + auto & e = imatrix.entries[name]; const int64_t nval = ggml_nelements(in_sum2); diff --git a/examples/convert-llama2c-to-ggml/convert-llama2c-to-ggml.cpp b/examples/convert-llama2c-to-ggml/convert-llama2c-to-ggml.cpp index 702bc74be..3513c9d10 100644 --- a/examples/convert-llama2c-to-ggml/convert-llama2c-to-ggml.cpp +++ b/examples/convert-llama2c-to-ggml/convert-llama2c-to-ggml.cpp @@ -549,20 +549,34 @@ static void load_vocab(const char * filename, const Config * config, struct my_l const int token_idx = gguf_find_key(ctx, KV_TOKENIZER_LIST); GGML_ASSERT(token_idx >= 0); - - const int score_idx = gguf_find_key(ctx, KV_TOKENIZER_SCORES); - GGML_ASSERT(score_idx >= 0); - const float * scores = (const float * ) gguf_get_arr_data(ctx, score_idx); - - const int toktype_idx = gguf_find_key(ctx, KV_TOKENIZER_TOKEN_TYPE); - GGML_ASSERT(toktype_idx >= 0); - const int * toktypes = (const int * ) gguf_get_arr_data(ctx, toktype_idx); + if (gguf_get_kv_type(ctx, token_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, token_idx) != GGUF_TYPE_STRING) { + die_fmt("invalid gguf type for %s", KV_TOKENIZER_LIST); + } const uint32_t n_vocab = gguf_get_arr_n(ctx, token_idx); if (n_vocab != static_cast(config->vocab_size)) { die_fmt("vocab size mismatch: (gguf) %u != (llama2c) %d", n_vocab, config->vocab_size); } + const int score_idx = gguf_find_key(ctx, KV_TOKENIZER_SCORES); + GGML_ASSERT(score_idx >= 0); + if (gguf_get_kv_type(ctx, score_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, score_idx) != GGUF_TYPE_FLOAT32 || + gguf_get_arr_n(ctx, score_idx) < n_vocab) { + die_fmt("invalid gguf type or size for %s", KV_TOKENIZER_SCORES); + } + const float * scores = (const float * ) gguf_get_arr_data(ctx, score_idx); + + const int toktype_idx = gguf_find_key(ctx, KV_TOKENIZER_TOKEN_TYPE); + GGML_ASSERT(toktype_idx >= 0); + if (gguf_get_kv_type(ctx, toktype_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, toktype_idx) != GGUF_TYPE_INT32 || + gguf_get_arr_n(ctx, toktype_idx) < n_vocab) { + die_fmt("invalid gguf type or size for %s", KV_TOKENIZER_TOKEN_TYPE); + } + const int * toktypes = (const int * ) gguf_get_arr_data(ctx, toktype_idx); + vocab->id_to_token.resize(n_vocab); for (uint32_t i = 0; i < n_vocab; i++) { diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index b9372ddda..a17c11d7c 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -46,6 +46,8 @@ Mandatory on every review; any finding here is **blocking**. Rule of thumb: GGUF - **Sizes/counts from tensor dims:** validate before allocating. Products like `ne[i]*nb[i]`/nbytes can overflow on crafted dims into an undersized alloc then heap overflow. Overflow checks must run BEFORE the arithmetic they guard - padding/alignment macros wrap to 0 near `SIZE_MAX`, so a guard after the pad passes. - **GGUF strings/arrays:** cap declared lengths and element counts before using them to size a loop or buffer; validate element type and length before casting an array to a pointer or reading fixed indices (`[i+1]`, `[0..2]`). +- **Element-type confusion:** casting `gguf_get_arr_data()` or `tensor->data` to `float *`/`int32_t *` needs an element-type check first (`gguf_get_kv_type() == GGUF_TYPE_ARRAY` then `gguf_get_arr_type()`; `type == GGML_TYPE_F32` for tensors). A `UINT8` array or `I8` tensor passes every length check, then gets read 4 bytes per element - a nearby length check is not a type check. +- **Loaders:** `GGML_ASSERT` on a file-derived value aborts the process; throw instead where the caller already catches (vocab, model loader, clip). - **File-supplied counts indexing fixed arrays:** bound any count (e.g. layer/block count into a `LLAMA_MAX_*` array) before indexing; watch checks that only fire when an optional key is present. - **Declared vs actual array length:** check the declared length of a GGUF array against the count actually read, not just against a buffer size. - **Bounds comparisons:** flag narrowing casts (`size_t`->`int32_t`) and signed/unsigned mixing that can bypass a length check and copy past a buffer. diff --git a/src/llama-model-loader.cpp b/src/llama-model-loader.cpp index 0ee63abfb..1ca698704 100644 --- a/src/llama-model-loader.cpp +++ b/src/llama-model-loader.cpp @@ -316,15 +316,19 @@ namespace GGUFMeta { struct GGUFMeta::ArrayInfo arr_info = GGUFMeta::GKV::get_kv(ctx, kid); + bool type_ok = false; switch (arr_info.gt) { case GGUF_TYPE_UINT32: - case GGUF_TYPE_INT32: GGML_ASSERT((std::is_same::value) || - (std::is_same::value)); break; - case GGUF_TYPE_FLOAT32: GGML_ASSERT((std::is_same::value)); break; - case GGUF_TYPE_STRING: GGML_ASSERT((std::is_same::value)); break; + case GGUF_TYPE_INT32: type_ok = (std::is_same::value) || + (std::is_same::value); break; + case GGUF_TYPE_FLOAT32: type_ok = (std::is_same::value); break; + case GGUF_TYPE_STRING: type_ok = (std::is_same::value); break; default: throw std::runtime_error(format("%s is not a string/float32/uint32/int32 array", key.c_str())); } + if (!type_ok) { + throw std::runtime_error(format("%s has wrong array element type %s", key.c_str(), gguf_type_name(arr_info.gt))); + } if constexpr (std::is_same::value) { const size_t n_items = gguf_get_arr_n(ctx, kid); @@ -357,16 +361,20 @@ namespace GGUFMeta { struct GGUFMeta::ArrayInfo arr_info = GGUFMeta::GKV::get_kv(ctx, kid); + bool type_ok = false; switch (arr_info.gt) { case GGUF_TYPE_BOOL: case GGUF_TYPE_UINT32: - case GGUF_TYPE_INT32: GGML_ASSERT((std::is_same::value) || - (std::is_same::value)); break; - case GGUF_TYPE_FLOAT32: GGML_ASSERT((std::is_same::value)); break; - case GGUF_TYPE_STRING: GGML_ASSERT((std::is_same::value)); break; + case GGUF_TYPE_INT32: type_ok = (std::is_same::value) || + (std::is_same::value); break; + case GGUF_TYPE_FLOAT32: type_ok = (std::is_same::value); break; + case GGUF_TYPE_STRING: type_ok = (std::is_same::value); break; default: throw std::runtime_error(format("%s is not a string/float32/uint32/int32 array", key.c_str())); } + if (!type_ok) { + throw std::runtime_error(format("%s has wrong array element type %s", key.c_str(), gguf_type_name(arr_info.gt))); + } if (arr_info.length > N_MAX) { throw std::runtime_error(format("array length %u for key %s exceeds max %u", (uint32_t) arr_info.length, key.c_str(), (uint32_t) N_MAX)); diff --git a/src/llama-vocab.cpp b/src/llama-vocab.cpp index 4a01dfd4c..c1b447f3f 100644 --- a/src/llama-vocab.cpp +++ b/src/llama-vocab.cpp @@ -1989,6 +1989,10 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { // Kimi-K2 doesn't need merges, skip LLAMA_LOG_INFO("%s: Kimi-K2 tokenizer detected, skipping BPE merges\n", __func__); } else { + if (gguf_get_kv_type(ctx, merges_keyidx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, merges_keyidx) != GGUF_TYPE_STRING) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_MERGES).c_str())); + } const int n_merges = gguf_get_arr_n(ctx, merges_keyidx); for (int i = 0; i < n_merges; i++) { const std::string word = gguf_get_arr_str(ctx, merges_keyidx, i); @@ -2028,8 +2032,13 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { const int precompiled_charsmap_keyidx = gguf_find_key(ctx, kv(LLM_KV_TOKENIZER_PRECOMPILED_CHARSMAP).c_str()); if (precompiled_charsmap_keyidx != -1) { + if (gguf_get_kv_type(ctx, precompiled_charsmap_keyidx) != GGUF_TYPE_ARRAY) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_PRECOMPILED_CHARSMAP).c_str())); + } const gguf_type pc_type = gguf_get_arr_type(ctx, precompiled_charsmap_keyidx); - GGML_ASSERT(pc_type == GGUF_TYPE_INT8 || pc_type == GGUF_TYPE_UINT8); + if (pc_type != GGUF_TYPE_INT8 && pc_type != GGUF_TYPE_UINT8) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_PRECOMPILED_CHARSMAP).c_str())); + } const size_t n_precompiled_charsmap = gguf_get_arr_n(ctx, precompiled_charsmap_keyidx); const char * pc = (const char *) gguf_get_arr_data(ctx, precompiled_charsmap_keyidx); @@ -2081,6 +2090,10 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { throw std::runtime_error("cannot find tokenizer merges in model file\n"); } { + if (gguf_get_kv_type(ctx, merges_keyidx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, merges_keyidx) != GGUF_TYPE_STRING) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_MERGES).c_str())); + } const int n_merges = gguf_get_arr_n(ctx, merges_keyidx); for (int i = 0; i < n_merges; i++) { const std::string word = gguf_get_arr_str(ctx, merges_keyidx, i); @@ -2407,11 +2420,20 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { throw std::runtime_error("cannot find tokenizer vocab in model file\n"); } + if (gguf_get_kv_type(ctx, token_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, token_idx) != GGUF_TYPE_STRING) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_LIST).c_str())); + } + const uint32_t n_tokens = gguf_get_arr_n(ctx, token_idx); const float * scores = nullptr; const int score_idx = gguf_find_key(ctx, kv(LLM_KV_TOKENIZER_SCORES).c_str()); if (score_idx != -1) { + if (gguf_get_kv_type(ctx, score_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, score_idx) != GGUF_TYPE_FLOAT32) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_SCORES).c_str())); + } const uint32_t n_scores = gguf_get_arr_n(ctx, score_idx); if (n_scores < n_tokens) { throw std::runtime_error("Index out of array bounds for scores (" + std::to_string(n_scores) + " < " + std::to_string(n_tokens) + ")\n"); @@ -2422,6 +2444,10 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { const int * toktypes = nullptr; const int toktype_idx = gguf_find_key(ctx, kv(LLM_KV_TOKENIZER_TOKEN_TYPE).c_str()); if (toktype_idx != -1) { + if (gguf_get_kv_type(ctx, toktype_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, toktype_idx) != GGUF_TYPE_INT32) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_TOKEN_TYPE).c_str())); + } const uint32_t n_toktypes = gguf_get_arr_n(ctx, toktype_idx); if (n_toktypes < n_tokens) { throw std::runtime_error("Index out of array bounds for toktypes (" + std::to_string(n_toktypes) + " < " + std::to_string(n_tokens) + ")\n"); @@ -2584,6 +2610,10 @@ void llama_vocab::impl::load(llama_model_loader & ml, const LLM_KV & kv) { { const int suppress_idx = gguf_find_key(ctx, kv(LLM_KV_TOKENIZER_SUPPRESS_TOKENS).c_str()); if (suppress_idx != -1) { + if (gguf_get_kv_type(ctx, suppress_idx) != GGUF_TYPE_ARRAY || + gguf_get_arr_type(ctx, suppress_idx) != GGUF_TYPE_INT32) { + throw std::runtime_error(format("invalid gguf type for %s", kv(LLM_KV_TOKENIZER_SUPPRESS_TOKENS).c_str())); + } const int n = gguf_get_arr_n(ctx, suppress_idx); const int32_t * data = (const int32_t *) gguf_get_arr_data(ctx, suppress_idx); // drop out-of-range ids diff --git a/tools/mtmd/clip.cpp b/tools/mtmd/clip.cpp index 7530ed613..b9dd5e845 100644 --- a/tools/mtmd/clip.cpp +++ b/tools/mtmd/clip.cpp @@ -3746,6 +3746,9 @@ struct clip_model_loader { } return; } + if (gguf_get_kv_type(ctx_gguf.get(), i) != GGUF_TYPE_ARRAY) { + throw std::runtime_error(string_format("%s: key '%s' is not an array\n", __func__, key.c_str())); + } const auto type = gguf_get_arr_type(ctx_gguf.get(), i); if (type != GGUF_TYPE_FLOAT32) { throw std::runtime_error(string_format("%s: array '%s' has type %d, expected %d (GGUF_TYPE_FLOAT32)\n", __func__, key.c_str(), type, GGUF_TYPE_FLOAT32)); @@ -3780,6 +3783,9 @@ struct clip_model_loader { } return; } + if (gguf_get_kv_type(ctx_gguf.get(), i) != GGUF_TYPE_ARRAY) { + throw std::runtime_error(string_format("%s: key '%s' is not an array\n", __func__, key.c_str())); + } const auto type = gguf_get_arr_type(ctx_gguf.get(), i); if (type != GGUF_TYPE_INT32) { throw std::runtime_error(string_format("%s: array '%s' has type %d, expected %d (GGUF_TYPE_INT32)\n", __func__, key.c_str(), type, GGUF_TYPE_INT32));