Fix panic in Strip decoder on short all-content tokens - #2239
Open
v-code01 wants to merge 1 commit into
Open
Conversation
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.
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
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.
Problem
Strip::decode_chain's trailing-strip loop panics on short tokens made up of thecontentchar:self.stopis a raw config value. A token that is entirelycontentand shorter thanstopkeeps decrementingindexpast 0:panicked at strip.rs: attempt to subtract with overflowindex out of bounds: the len is 1 but the index is 18446744073709551615Separately, when the leading and trailing windows overlap on a short token,
start_cutcan exceedstop_cut, so the finalchars[start_cut..stop_cut]panics on a reversed slice range (slice index starts at 2 but ends at 1).Both are reachable through
Tokenizer::decodefor any tokenizer whose decoder is aStripwithstop >= 1. Decoders aren't wrapped incatch_unwind, so one such token crashes the wholedecode/decode_batch.Fix
Bound the trailing loop with
self.stop.min(chars.len())and clamp the range withstop_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