Skip to content

Do not panic in the Strip decoder on crafted config or short tokens#2154

Open
v-code01 wants to merge 1 commit into
huggingface:mainfrom
v-code01:strip-decoder-no-panic
Open

Do not panic in the Strip decoder on crafted config or short tokens#2154
v-code01 wants to merge 1 commit into
huggingface:mainfrom
v-code01:strip-decoder-no-panic

Conversation

@v-code01

@v-code01 v-code01 commented Jul 5, 2026

Copy link
Copy Markdown

What

Strip's content/start/stop fields are deserialized straight from the config (tokenizer.json) with no validation, and decode_chain trusts them, so a crafted decoder config (plus tokens from the model vocab) panics:

let mut stop_cut = chars.len();
for i in 0..self.stop {
    let index = chars.len() - i - 1;   // underflows when a token is shorter than `stop`
    ...
}
let new_token: String = chars[start_cut..stop_cut].iter().collect(); // panics on a reversed range
  • Underflowchars.len() - i - 1 underflows for a token shorter than stop: a panic in debug/overflow-checked builds, and an out-of-bounds index after wrapping in release. An empty token with stop >= 1 triggers it ("attempt to subtract with overflow").
  • Reversed range — when a token is made entirely of content characters, start_cut overruns stop_cut, so chars[start_cut..stop_cut] panics. E.g. Strip::new('H', 2, 2) decoding "HH"chars[2..0] ("slice index starts at 2 but ends at 0").

Fix

Bound the trailing loop by the token length (0..self.stop.min(chars.len())) and clamp stop_cut = stop_cut.max(start_cut) so the slice range is never reversed. Both crafted cases now decode to an empty string, consistent with "everything was stripped".

Tests

Adds does_not_panic_on_crafted_config covering the reversed-range and empty/short-token cases; the existing decode test is unchanged.

cc @ArthurZucker @McPatate

`Strip`'s `content`/`start`/`stop` fields are deserialized straight from
the config with no validation, and `decode_chain` trusts them:

- `let index = chars.len() - i - 1;` (i in `0..self.stop`) underflows when
  a token is shorter than `stop` — a panic in debug/overflow-checked
  builds, and an out-of-bounds index after wrapping in release. An empty
  token with `stop >= 1` triggers it.
- `chars[start_cut..stop_cut]` panics on a reversed range when a token is
  made entirely of `content` characters, so `start_cut` overruns
  `stop_cut` (e.g. content `'H'`, start 2, stop 2, token `"HH"` →
  `chars[2..0]`).

Bound the trailing loop by the token length and clamp `stop_cut` so the
slice range is never reversed; both crafted cases now decode to an empty
string. Adds a regression test.
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.

1 participant