Skip to content

fix: avoid char-boundary panic in NBReader::try_read#172

Merged
epage merged 2 commits into
rust-cli:mainfrom
SAY-5:fix-try-read-char-boundary
May 14, 2026
Merged

fix: avoid char-boundary panic in NBReader::try_read#172
epage merged 2 commits into
rust-cli:mainfrom
SAY-5:fix-try-read-char-boundary

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented May 12, 2026

The internal buffer stores raw input bytes as latin-1 chars, so a byte >= 0x80 becomes a two-byte char and drain(..1) could land off a char boundary and panic. Drain the first char by its UTF-8 length instead.

Closes #32

Comment thread src/reader.rs Outdated
@SAY-5 SAY-5 force-pushed the fix-try-read-char-boundary branch from 298398d to 534e491 Compare May 13, 2026 00:13
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 13, 2026

Split into a test commit that shows the panic on multi-byte input followed by the fix, per the contrib guide.

@SAY-5 SAY-5 force-pushed the fix-try-read-char-boundary branch from 534e491 to f9e11ce Compare May 13, 2026 06:41
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 13, 2026

Restructured per the c-test guide: the test commit now documents the panic with should_panic, and the fix commit drops should_panic and asserts the decoded chars. Pushed.

Comment thread src/reader.rs Outdated
@SAY-5

This comment was marked as duplicate.

@SAY-5 SAY-5 force-pushed the fix-try-read-char-boundary branch from f9e11ce to dffa1eb Compare May 13, 2026 20:41
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 13, 2026

Force-pushed without the thread::sleep, both tests now use a deadline-bounded poll on try_read so they finish as soon as the reader thread has pumped the two bytes. test suite still passes locally.

@SAY-5 SAY-5 force-pushed the fix-try-read-char-boundary branch from dffa1eb to 740defa Compare May 13, 2026 20:59
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 13, 2026

Switched to a synchronous fixture that fills the buffer directly, no sleeps or polling.

@epage
Copy link
Copy Markdown
Contributor

epage commented May 14, 2026

Generally, I would be against breaking abstractions in tests but this might be the least bad option, so moving forward with it. We can always reevaluate in the future.

@epage epage merged commit 808af95 into rust-cli:main May 14, 2026
14 of 15 checks passed
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.

try_read panic

2 participants