Segmentation fault

[Munksgaard]

@wenkokke pointed out that the following code not only panics, but actually segfaults:

extern crate session_types;

fn main () {
    use session_types::*;

    fn client(c: Chan<(), Send<(), Eps>>) {
        c.send(()).close();
    }

    fn drop_server(_c: Chan<(), Recv<(), Eps>>) {}

    connect(drop_server, client);
}

That's scary and should be fixed.

@Manishearth I have a feeling that this is caused by 0f25ccb7c3bc9f65fa8eaf538233e8fe344a189a. When I replace the close implementation with return () I no longer get a segmentation fault. Are destructor bombs still something that we want in our code, or has something changed in Rust so that they now segfault instead of just notifying the users about unfinished channels?

[Munksgaard]

cc @laumann

[laumann]

It's not a bug, it's a feature

• Someone

:stuck_out_tongue:

But seriously, it sounds like it's easier to fix than I initially thought, this segfault was haunting me a bit, thanks for looking into this @Munksgaard!

What's the behaviour if you swap around drop(sender);drop(receiver);?

[Munksgaard]

What's the behaviour if you swap around drop(sender);drop(receiver);?

Still the same, unfortunately.

[Manishearth]

Nothing's changed in rust, and this code should be safe except if the destructor of something that's still buffered up in the channel panics. Which shouldn't happen as channels should be empty at this stage.

Does using MaybeUninit instead of mem::uninitialized fix the issue?

[Munksgaard]

With the snippet below I still get Segmentation fault (core dumped)

-        let mut sender = unsafe { mem::uninitialized() };
-        let mut receiver = unsafe { mem::uninitialized() };
+        let mut sender = unsafe { mem::MaybeUninit::uninitialized().into_initialized() };
+        let mut receiver = unsafe { mem::MaybeUninit::uninitialized().into_initialized() };

[Manishearth]

Oh, MaybeUninit's APIs aren't great for this. Hmm.

[Manishearth]

I can't repro the segfault, but I can see if I can clean up the destructor code.

[Manishearth]

diff --git a/src/lib.rs b/src/lib.rs
index 30455d1..6053da1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -60,7 +60,7 @@
 //! }
 //! ```
 #![cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
-
+#![feature(maybe_uninit)]
 extern crate crossbeam_channel;

 use std::marker;
@@ -185,11 +185,14 @@ impl<E, P> Drop for Chan<E, P> {

 impl<E> Chan<E, Eps> {
     /// Close a channel. Should always be used at the end of your program.
-    pub fn close(mut self) {
+    pub fn close(self) {
         // This method cleans up the channel without running the panicky destructor
         // In essence, it calls the drop glue bypassing the `Drop::drop` method
         use std::mem;

+        let mut this = mem::MaybeUninit::uninitialized();
+        let this = this.set(self);
+
         // Create some dummy values to place the real things inside
         // This is safe because nobody will read these
         // mem::swap uses a similar technique (also paired with `forget()`)
@@ -199,16 +202,11 @@ impl<E> Chan<E, Eps> {
         // Extract the internal sender/receiver so that we can drop them
         // We cannot drop directly since moving out of a type
         // that implements `Drop` is disallowed
-        mem::swap(&mut self.0, &mut sender);
-        mem::swap(&mut self.1, &mut receiver);
+        mem::swap(&mut this.0, &mut sender);
+        mem::swap(&mut this.1, &mut receiver);

         drop(sender);
         drop(receiver); // drop them
-
-        // Ensure Chan destructors don't run so that we don't panic
-        // This also ensures that the uninitialized values don't get
-        // read at any point
-        mem::forget(self);
     }
 }

is this better?

[Manishearth]

(yes, it uses a rust feature, there's a crate for this that's nicer)

[Manishearth]

diff --git a/src/lib.rs b/src/lib.rs
index 30455d1..7c8aaf2 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -60,7 +60,7 @@
 //! }
 //! ```
 #![cfg_attr(feature = "cargo-clippy", allow(type_complexity))]
-
+#![feature(maybe_uninit)]
 extern crate crossbeam_channel;

 use std::marker;
@@ -185,30 +185,20 @@ impl<E, P> Drop for Chan<E, P> {

 impl<E> Chan<E, Eps> {
     /// Close a channel. Should always be used at the end of your program.
-    pub fn close(mut self) {
+    pub fn close(self) {
         // This method cleans up the channel without running the panicky destructor
         // In essence, it calls the drop glue bypassing the `Drop::drop` method
-        use std::mem;
+        use std::{mem, ptr};

-        // Create some dummy values to place the real things inside
-        // This is safe because nobody will read these
-        // mem::swap uses a similar technique (also paired with `forget()`)
-        let mut sender = unsafe { mem::uninitialized() };
-        let mut receiver = unsafe { mem::uninitialized() };
+        let mut this = mem::MaybeUninit::uninitialized();
+        this.set(self);
+        let this = this.as_mut_ptr();

-        // Extract the internal sender/receiver so that we can drop them
-        // We cannot drop directly since moving out of a type
-        // that implements `Drop` is disallowed
-        mem::swap(&mut self.0, &mut sender);
-        mem::swap(&mut self.1, &mut receiver);
+        let sender = unsafe { ptr::read(&(*this).0 as *const _) };
+        let receiver = unsafe { ptr::read(&(*this).1 as *const _) };

         drop(sender);
         drop(receiver); // drop them
-
-        // Ensure Chan destructors don't run so that we don't panic
-        // This also ensures that the uninitialized values don't get
-        // read at any point
-        mem::forget(self);
     }
 }

and this is a better version that doesn't use uninitialized at all.

[Munksgaard]

Same thing I'm afraid, have you tried it on the segfault branch that I just pushed?

[Manishearth]

Yep, though your test panics

[Manishearth]

Oh wait no I'm getting the segfault too, I just didn't see it with the panic message

[Manishearth]

Oh, the unsafety isn't because of the destructor bomb stuff. It's because Chan thinks it contains Box<u8>s, which it tries to free. These should be raw pointers.

[Manishearth]

Fixed it. PR coming soon

[Manishearth]

Fixed in https://github.com/Munksgaard/session-types/pull/58