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.

flate2-rs/src/ffi/c.rs

Lines 209 to 222 in f62ff42

	fn decompress(
	&mut self,
	input: &[u8],
	output: &mut [u8],
	flush: FlushDecompress,
	) -> Result<Status, DecompressError> {
	let raw = &mut *self.inner.stream_wrapper;
	raw.msg = ptr::null_mut();
	raw.next_in = input.as_ptr() as *mut u8;
	raw.avail_in = cmp::min(input.len(), c_uint::MAX as usize) as c_uint;
	raw.next_out = output.as_mut_ptr();
	raw.avail_out = cmp::min(output.len(), c_uint::MAX as usize) as c_uint;
	let rc = unsafe { mz_inflate(raw, flush as c_int) };

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: #380