Skip to content

b64ct: subtle overflow bug #205

New issue

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

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

Already on GitHub? # to your account

Closed
newpavlov opened this issue Jan 13, 2021 · 6 comments · Fixed by #212
Closed

b64ct: subtle overflow bug #205

newpavlov opened this issue Jan 13, 2021 · 6 comments · Fixed by #212

Comments

@newpavlov
Copy link
Member

newpavlov commented Jan 13, 2021

While working on the in-place decoding function, I noticed a subtle overflow bug. Currently decoded_len is implemented as:

pub const fn decoded_len(bytes: &str) -> usize {
    (bytes.len() * 3) / 4
}

For lengths greater than usize::MAX/3 it will produce incorrect results. Obviously in practice it's really unlikely that someone will work with more than gigabyte B64-encoded strings and on 64-bit platforms it will be practically impossible to trigger this bug. But nevertheless it's a clear bug.

The easiest solution would be to cast usize to u128 and back, but it's quite inelegant solution and it looks like a bit hard for compiler to properly infer properties of a resulting value... Another solution would be to use two branches for values bigger than usize::MAX/3 and not. And the last solution will be to use an alternative formula which would produce identical results.

The same applies to encoded_len as well, since it contains bytes.len() * 4.

@tarcieri
Copy link
Member

Good catch!

Perhaps as a simple solution we can configure a maximum allowed length.

@newpavlov
Copy link
Member Author

newpavlov commented Jan 13, 2021

Hm, for some reason compiler is unable to remove panic in this code. Even replacing usize::MAX/3 with 1 does not work... :(

UPD: Reported as rust-lang/rust#80963.

@tarcieri
Copy link
Member

Is there some reason you're not using get? This is panic-free:

https://rust.godbolt.org/z/jEbzGn

@newpavlov
Copy link
Member Author

It's a simplified, minimal example. The idea is that I plan to loop over the buf in overlapping chunks (with period 3 and 4), so since compiler can not optimize such simple case, there is no hope for a more advanced code to be panic-free. I guess we can use get inside the loop and bubble Nones to Err(_), but while it will be indeed panic-free, such solution looks quite dirty to me.

@tarcieri
Copy link
Member

Aah, okay, well in that case how about bounds checking the slicing operation like this?

https://rust.godbolt.org/z/Tj1Mz7

@newpavlov
Copy link
Member Author

newpavlov commented Jan 13, 2021

I am not sure it will help with the loop which I want to write. Consider this code: https://rust.godbolt.org/z/8eK49M I want to take buf[4*chunk..][..4] and buf[3*chunk..][..3] slices each iteration, but compiler is unable to even eliminate panics for buf[4*chunk..] and buf[3*chunk..]. Trying to add various additional members to the first branch does not help. And we haven't even got to processing of the buffer tail.

Changing the loop to:

for i in (0..buf.len()).step_by(4) {
    &buf[i..];
}

does not help either (either way, such loop can not be used for code which I have in mind).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants