Skip to content

Fix panic in DecodeStream::step due to incorrect index usage#1699

Merged
Narsil merged 3 commits intohuggingface:mainfrom
n0gu-furiosa:fix-decode-stream-overflow
Jan 9, 2025
Merged

Fix panic in DecodeStream::step due to incorrect index usage#1699
Narsil merged 3 commits intohuggingface:mainfrom
n0gu-furiosa:fix-decode-stream-overflow

Conversation

@n0gu-furiosa
Copy link
Copy Markdown
Contributor

When calling DecodeStream::step multiple times, it eventually panics with attempt to subtract with overflow in the following lines of code:

let new_prefix_index = ids.len() - *prefix_index;
*ids = ids.drain(*read_index..).collect();

The panic can be easily reproduced, and I have added a test case to demonstrate the issue.

Upon inspecting the code, I found that the shrinking of the token buffer references read_index instead of prefix_index. This PR corrects the issue by using the correct index.

However, this change makes read_index unused, so I am not entirely certain if it aligns with the intended logic of the original implementation. Please let me know if further adjustments or clarifications are needed, or if there is additional context regarding the intended use of read_index.

@irexyc irexyc mentioned this pull request Dec 26, 2024
@Narsil
Copy link
Copy Markdown
Contributor

Narsil commented Jan 9, 2025

However, this change makes read_index unused, so I am not entirely certain if it aligns with the intended logic of the original implementation. Please let me know if further adjustments or clarifications are needed, or if there is additional context regarding the intended use of read_index.

I'm very confused by my own implementation re-reading it. Your fix is definitely correct and we can remove than internal pointer.
The prefix/read difference stems from another implementation I had which tried to keep all "side effects contained (in the read segment part, which isn't ever compared) and keep the prefix segment distinct.
However in this implementation because we recalculate the prefix *prefix = tokenizer.decode(..) then we re-encapsulate the side effect which is simpler.

What's weird is that I remember having only that in my first iteration of this. I have no idea why I re-added half-brokenly that read_index.

Thanks a lot for the fix.

Note: I will rewrite your unit test. They are good but I find them not really linearly readable and also panicking within tests is perfectly fine (it's a failure).

@Narsil Narsil merged commit 862d1a3 into huggingface:main Jan 9, 2025
@n0gu-furiosa n0gu-furiosa deleted the fix-decode-stream-overflow branch January 10, 2025 01:36
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.

2 participants