search

Frommi / miniz_oxide

Rust replacement for miniz
MIT License
179 stars 49 forks source link

Expand comment on `unsafe` in `read_u16_le()` #46

Closed Shnatsel closed 5 years ago

Shnatsel commented 5 years ago

I've spent an hour figuring out if this function is sound or not, might as well document it so that future readers don't have to.

oyvindln commented 5 years ago

Yeah was a bit unsure about that myself. No idea really if slices in theory can be bigger than isize::max (even though I don't know any computer that has that much memory space.)

It's been a while since we wrote this so I wonder some of the unsafe usage could be avoided without any speed penalty now.

Shnatsel commented 5 years ago

Yes, they can. It is documented here: https://doc.rust-lang.org/std/primitive.pointer.html#method.offset

u16::from_le_bytes might help do this safely. However, I'm not really worried about this unsafe block; it's the one in flush() that I'm scared of, and sadly I'm not qualified to check if it's sound or not.

Shnatsel commented 5 years ago

FWIW, miniz_oxide now has ~6000 downloads per day on crates.io. For reference, flate2 has ~9000 downloads/day. This makes miniz_oxide one of the more popular Rust projects and the most popular DEFLATE implementation.

I've started auditing it like I've done with the other two DEFLATE implementations, but found that the level of complexity here is beyond my abilities to reason about.

oyvindln commented 5 years ago

it's the one in flush() that I'm scared of, and sadly I'm not qualified to check if it's sound or not.

I think it might be best to just avoid unsafe there, and change it later if we can make sure it's sound and it has any positive performance impact.