rust-lang / flate2-rs

DEFLATE, gzip, and zlib bindings for Rust
https://docs.rs/flate2
Apache License 2.0
862 stars 159 forks source link

Store `StreamWrapper::inner` as a raw pointer #394

Closed icmccorm closed 5 months ago

icmccorm commented 6 months ago

Fixes #392.

Storing and accessing StreamWrapper as a raw pointer fixes a tree borrows violation and removes the need for implementations of Deref and DerefMut.

icmccorm commented 6 months ago

I'll follow up with better documentation—thanks for pointing me in the right direction!

Since this code deals with unsafe a lot, reviewing it isn't easy

My decision to remove Deref and DerefMut introduced unsafe everywhere that inner needs to be accessed, but the invariants related to inner only apply in certain situations. It can be unsound to mutably borrow inner, pass that borrow as a pointer into a foreign call, and then use the same borrow again, since it may have become invalid during the foreign call. Because of this, I decided it would be best to make accessing inner unsafe in any situation instead of providing implementations of Deref and DerefMut that are not always sound. I'm thinking of adding the following note everywhere that inner is borrowed.

/// SAFETY: The field `StreamWrapper::inner` is a pointer to a cyclic structure, so any mutable 
/// borrows against it can become invalidated by foreign function calls that mutate this 
/// structure. This borrow is safe, since it is not used after the call returns. 

Actually, now that I'm thinking of it, it might be sound to leave the implementation of Deref, but require unsafe for mutable borrows. I'll test that out and follow up with my next changes.

icmccorm commented 5 months ago

This should be ready to go, unless you'd like any other changes to documentation. I think Deref has the same potential soundness issues as DerefMut under Tree Borrows, so it's best to remove both traits.

Byron commented 5 months ago

Thanks @Nilstrieb for the additional review, and thanks again to @icmccorm for your contribution!