unsafe review: Potential (not actual) dangling pointers after inflate/deflate

[Manishearth]

This is mostly an unsafe code quality issue, there is no actual UB here.

https://github.com/rust-lang/flate2-rs/blob/f62ff42615861f1d890160c5f77647466505eac0/src/ffi/c.rs#L209-L222

We call mz_inflate() here with self.inner.stream_wrapper here (and later in similar code, mz_deflate()

This code sets some fields of self.raw to point to the input and output parameters. These can have an arbitrarily short lifetimes; they can live shorter than self. In such a case, we'll have dangling pointers after decompress() is called since we hold on to them in self.inner.stream_wrapper.

Now, self.inner.stream_wrapper stores raw pointers; and it's fine for them to be dangling as long as we do not read from them. In this code self.inner.stream_wrapper is only accessed in future decompress() calls and in reset() (which presumably does not read these buffers). So it's safe, but a defense in depth fix would be clear the *_in and *_out fields after calling inflate, or at least documenting these invariants very strongly.

[Byron]

Thanks for the review!

Maybe, just maybe, this is related this gitoxide issue where there is a double-free or corruption. My thought here is that some zlib implementations might actually do something to these pointers even though it seems somewhat far-fetched that it would do anything to input and output pointers that it doesn't own.

In any case, nulling these pointers after using them should probably do the trick.

PS: I set the "Help Wanted" label if you are interested to fix it. Otherwise I could probably also attempt a fix and invite you for review.

[Manishearth]

Ah, good note. I don't think there's unsoundness caused by this currently but if there's a preexisting known bug I do think we should try and fix this sooner rather than later.

I prepared a patch: https://github.com/rust-lang/flate2-rs/pull/380