Voultapher / self_cell

Safe-to-use proc-macro-free self-referential structs in stable Rust.
Apache License 2.0
242 stars 15 forks source link

Fix drop order of JoinedCell fields #21

Closed steffahn closed 3 years ago

steffahn commented 3 years ago

Closes #20

Voultapher commented 3 years ago

Let me know if you agree with https://github.com/Voultapher/self_cell/pull/21/commits/2ca0660884e55886505633e33f37ea5280f1d8ec

Voultapher commented 3 years ago

Also do you think switching to ManuallyDrop would be worth it? We have pointers anyway, so the use seems limited to me. And I suspect this simple pointer based code is quite fast to compile.

steffahn commented 3 years ago

2ca0660 Seems like a valid approach as well.

I’m not sure if it’d be superflous, but I’d feel even better if the code used addr_of_mut!((*ptr).field) instead of the &mut (*ptr).field. I’m just being paranoid that &mut (*ptr).field might require that the whole *ptr is supposed to be valid, even though a different field is already dropped. (Probably not, but I’m not sure and the change wouldn’t hurt anyway I guess?)

steffahn commented 3 years ago

Note that even without 2ca0660, drop order of fields is clearly specified.

But with the addr_of_mut, I like it even better than the previous drop_in_place(joined_ptr.as_ptr());, because in my mind that one could also try to get mutable access to the whole *joined_ptr.as_ptr() first, which would invalidate the shared reference to owner that dependent might contain.

In other words: I’m not quite sure if it makes a difference but if any version is sound then the current one with drop_in_place(addr_of_mut!((*joined_ptr.as_ptr()).dependent)); drop_in_place(addr_of_mut!((*joined_ptr.as_ptr()).owner)); should be sound, too.

steffahn commented 3 years ago

I, too, don’t really think using ManuallyDrop adds anything useful.

steffahn commented 3 years ago

Squashed the commits a bit.

steffahn commented 3 years ago

Added another commit with more changes. I’m more confident that these operations are sound with addr_of_mut than before. In particular, the UnsafeSelfCell::borrow_mut seemed problematic because it returned a mutable reference to the whole JoinedCell even though owner must not be borrowed mutably while dependent can hold a reference to it.

steffahn commented 3 years ago

Another usage of addr_of_mut added. I’m not sure if &mut (*self.joined_ptr.as_ptr()).owner is allowed when the dependent field contains uninitialized data. Better safe than sorry.

steffahn commented 3 years ago

Did some more reading, turns out the addr_of[_mut] usage is unnecessary in most cases, but 2 things were legitimate AFAICT:

steffahn commented 3 years ago

Reduced usage of addr_of[_mut] to those cases now :-), squashed again