fix(gguf): error instead of panic on malformed tokenizer metadata - #2307
Open
Awshesh12 wants to merge 1 commit into
Open
fix(gguf): error instead of panic on malformed tokenizer metadata#2307Awshesh12 wants to merge 1 commit into
Awshesh12 wants to merge 1 commit into
Conversation
The GGUF tokenizer conversion eagerly `.unwrap()`/`.expect()`ed several values that come straight from untrusted GGUF metadata, so a malformed file aborted the process with a panic instead of returning a clean error: - `bpe_tokenizer` parsed each `merges` entry with `splitn(2, ' ').collect_tuple().expect(...)`, panicking on any merge that is not a space-separated pair. - `convert_gguf_to_hf_tokenizer` read `tokenizer.ggml.token_type` with `.to_vec().unwrap()` / `.to_i32().unwrap()`, panicking when the key was absent, not an array, or held a non-integer value. Propagate these as errors via `?` so a bad file fails gracefully, matching the existing bounds-checking of the special token ids. Add unit tests covering the malformed and well-formed BPE merge cases.
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 23 4454 3116 790 548 CSS 3 282 252 6 24 CUDA 119 23681 19217 1704 2760 Dockerfile 1 35 19 9 7 HTML 2 27 27 0 0 JavaScript 3 577 562 12 3 Jinja2 7 694 656 5 33 JSON 27 15864 15861 0 3 Makefile 1 18 16 0 2 MDX 36 5882 0 4315 1567 Metal Shading Lan| 37 14287 11284 1136 1867 PowerShell 1 627 546 28 53 Python 149 11885 9849 473 1563 Shell 2 989 781 109 99 Plain Text 3 3723 0 2413 1310 TOML 28 1370 1187 43 140 TypeScript 11 1641 1404 66 171 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 3 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 272 11336 0 8431 2905 |- BASH 22 282 208 43 31 |- Dockerfile 2 5 5 0 0 |- JSON 6 289 289 0 0 |- PowerShell 1 1 1 0 0 |- Python 135 7239 6021 310 908 |- Rust 62 3820 2838 388 594 |- TOML 7 77 65 0 12 (Total) 23049 9427 9172 4450 ───────────────────────────────────────────────────────────────────────────────── Rust 665 295894 263526 5815 26553 |- Markdown 413 9762 452 8127 1183 (Total) 305656 263978 13942 27736 ───────────────────────────────────────────────────────────────────────────────── Svelte 19 1969 1826 51 92 |- CSS 1 4 4 0 0 |- JavaScript 19 921 767 25 129 (Total) 2894 2597 76 221 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1415 417964 341028 34347 42589 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
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
1 participant
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
The GGUF → HF tokenizer conversion eagerly
.unwrap()/.expect()ed several values that come straight from untrusted GGUF metadata, so a malformed file panics (aborts the whole process) instead of returning a recoverable error. This continues the hardening started in #2282 (special-token id bounds) and #2286 (chat-template panic), which already guard the indexing of special tokens — but two sibling panic sites in the same file remained:bpe_tokenizerparses eachmergesentry withsplitn(2, ' ').collect_tuple().expect("Failed to convert split into 2-tuple"),so any merge that is not a space-separated pair panics.
convert_gguf_to_hf_tokenizerreadstokenizer.ggml.token_typewith.to_vec().unwrap()/.to_i32().unwrap(), panicking when the value ismissing, not an array, or holds a non-integer.
Changes
?(BPE merges useok_or_elsewith a descriptive message naming the offending merge;token_typepropagates the underlyingcandle_coreerrors).bpe_malformed_merge_errors_without_panic,bpe_well_formed_merge_builds) that construct aPropsGGUFdirectly — no model download needed.Testing
Both new tests pass; behavior for well-formed GGUF files is unchanged.