Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unsoundness of peek_ahead in iter.rs #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hkBst
Copy link
Contributor

@hkBst hkBst commented Nov 19, 2024

This fixes the inadequate safety checks, by checking the index n against the length of the byte array. Then there is no need to use wrapping_add instead of add since we now never point outside the array (since we checked the index against length already).

Potentially, there is some performance impact here, but the benchmarks are not very clear about the direction. Perhaps there is none to speak of?

@seanmonstar
Copy link
Owner

Checking in Godbolt, it seems like this produces less optimized assembly code. Could we just mark the function unsafe and pass the requirement onto the caller?

@hkBst
Copy link
Contributor Author

hkBst commented Dec 10, 2024

I'd love to do that, but I don't see how it would help:

The single caller lib::parse_method does:

  • a safe get for 4 bytes match bytes.peek_n::<[u8; 4]>(4) {
  • then wants to look at the 5th byte Some(POST) if bytes.peek_ahead(4) == Some(b' ') => {
  • then unsafely advances 5 bytes bytes.advance(5);

So, we need to know that the 5th byte exists, which means we need to check against the length of the Bytes somehow.

@seanmonstar
Copy link
Owner

Since we already know it had 4 bytes, then using cursor.add(5) is safe, since it's either another byte, or the end of the slice. And the docs for ptr::add() say that vec.as_ptr().add(vec.len()) is always safe (which is one byte off the end.)

@hkBst
Copy link
Contributor Author

hkBst commented Dec 11, 2024

Right, I suppose that allows us to simplify the in-bounds check to just a comparison with bytes.end(), but it would also lead to a baroque safety requirement and make peek_ahead an extremely single-use abstraction. I'll try and whip something up.

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