renet
/
renet
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix potential unsoundness in SocketRef::into_inner()/drop(). 

This didn't ever actually cause UB because the drop implementation
should have compiled to a no-op under all conditions; `finish` is
a part of a patchset that never got in. (Even if it didn't, the code
should have been dynamically sound, under all but the most strict
interpretations of Rust's aliasing requirements.)

But it's still a problem, at least because there is a warning for
this potential unsoundness that will become an error in the future.
So everything else aside, we can't use this pattern going forward.

For now, remove the Drop impl, exploiting the fact that `finish`
is a no-op. Going forward this will likely require unsafe code, or
some fundamental change in the way SocketRef is implemented, but
I could not come up with any way to do it. :(
v0.7.x
whitequark 2019-06-22 10:52:39 +00:00
parent b5d5023ac6
commit 2902ca00c0
1 changed files with 15 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
src/socket/ref_.rs
View File

@ -32,8 +32,7 @@ impl<'a> Session for TcpSocket<'a> {}
///
/// Allows the network stack to efficiently determine if the socket state was changed in any way.
pub struct Ref<'a, T: Session + 'a> {
socket: &'a mut T,
consumed: bool,
socket: &'a mut T
}
impl<'a, T: Session + 'a> Ref<'a, T> {
@ -43,7 +42,7 @@ impl<'a, T: Session + 'a> Ref<'a, T> {
///
/// [into_inner]: #method.into_inner
pub fn new(socket: &'a mut T) -> Self {
Ref { socket, consumed: false }
Ref { socket }
}
/// Unwrap a smart pointer to a socket.
@ -56,8 +55,7 @@ impl<'a, T: Session + 'a> Ref<'a, T> {
/// be sure to call [new] afterwards.
///
/// [new]: #method.new
pub fn into_inner(mut ref_: Self) -> &'a mut T {
ref_.consumed = true;
pub fn into_inner(ref_: Self) -> &'a mut T {
ref_.socket
}
}
@ -76,10 +74,15 @@ impl<'a, T: Session> DerefMut for Ref<'a, T> {
}
}
impl<'a, T: Session> Drop for Ref<'a, T> {
fn drop(&mut self) {
if !self.consumed {
Session::finish(self.socket);
}
}
}
// FIXME: `Session::finish` currently does not do anything, but if it did, this would be a problem,
// because `SocketRef::into_inner` would have to use unsafe code, and there's currently no unsafe
// code in smoltcp at all (other than the `phy` module). The reason it would need unsafe code is
// that it is currently an error to destructure a value that implements Drop (or move out its
// fields in any other way), so it'd have to be transmuted away. This is a deficiency in Rust:
// it is always safe to ignore the Drop impl during destructuring.
//
// impl<'a, T: Session> Drop for Ref<'a, T> {
// fn drop(&mut self) {
// Session::finish(self.socket);
// }
// }