Skip to content

Fix panic in Strip decoder on short all-content tokens - #2239

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

Fix panic in Strip decoder on short all-content tokens#2239
v-code01 wants to merge 1 commit into
huggingface:mainfrom
v-code01:fix/strip-decoder-panic

Conversation

@v-code01

Copy link
Copy Markdown

Problem

Strip::decode_chain's trailing-strip loop panics on short tokens made up of the content char:

let mut stop_cut = chars.len();
for i in 0..self.stop {                 // not bounded by chars.len()
    let index = chars.len() - i - 1;    // underflows once i >= chars.len()
    if chars[index] == self.content {
        stop_cut = index;
        continue;
    } else { break; }
}
let new_token: String = chars[start_cut..stop_cut].iter().collect();

self.stop is a raw config value. A token that is entirely content and shorter than stop keeps decrementing index past 0:

  • debug: panicked at strip.rs: attempt to subtract with overflow
  • release: index out of bounds: the len is 1 but the index is 18446744073709551615

Separately, when the leading and trailing windows overlap on a short token, start_cut can exceed stop_cut, so the final chars[start_cut..stop_cut] panics on a reversed slice range (slice index starts at 2 but ends at 1).

Both are reachable through Tokenizer::decode for any tokenizer whose decoder is a Strip with stop >= 1. Decoders aren't wrapped in catch_unwind, so one such token crashes the whole decode/decode_batch.

Fix

Bound the trailing loop with self.stop.min(chars.len()) and clamp the range with stop_cut.max(start_cut) so an over-stripped token collapses to an empty string. Existing valid inputs are unaffected (the current tests still pass).

Tests

Added regression tests for the short all-content token (Strip::new('_', 0, 2) on ["_"], and a mixed batch) and the overlapping start/stop windows (Strip::new('H', 2, 1) on ["HH"]). Both panic on the pre-fix code and pass with the fix.

cc @ArthurZucker @Narsil

Strip::decode_chain's trailing-strip loop ran `for i in 0..self.stop` and
indexed `chars.len() - i - 1` without bounding `i` by the token length.
`self.stop` is a raw config value, so a token made up entirely of the
`content` char and shorter than `stop` kept decrementing past 0: in debug
this panics with "attempt to subtract with overflow", in release the `usize`
wraps to a huge value and `chars[index]` panics with an out-of-bounds index.

Separately, when the leading and trailing windows overlap on a short token,
`start_cut` can exceed `stop_cut`, so `chars[start_cut..stop_cut]` panics on a
reversed slice range.

Both are reachable through `Tokenizer::decode` for any tokenizer configured
with a Strip decoder that has `stop >= 1`, and decoders are not wrapped in
catch_unwind, so a single such token crashes the whole decode/decode_batch call.

Bound the trailing loop with `self.stop.min(chars.len())` and clamp the final
range with `stop_cut.max(start_cut)` so an over-stripped token collapses to an
empty string. Existing valid inputs are unaffected.

Adds regression tests for the short all-content token and the overlapping
start/stop windows.
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