Repository: cassandra-rs/cassandra-rs (language: rust)

Issue:
Mysterious build failure in test_early_drop_rust_futures
https://travis-ci.org/Metaswitch/cassandra-rs/builds/481257553 shows a build failure (build 249), which persists on retry. Build 248 https://travis-ci.org/Metaswitch/cassandra-rs/builds/481257551 which is a simple merge commit resulting in the same code succeeded, but retrying that build yields a failure too, so the pass must have been an anomaly.

The failure is:
```
test test_early_drop_rust_futures ... error: process didn't exit successfully: `/home/travis/build/Metaswitch/cassandra-rs/target/debug/deps/futures-875ccf2c5ef67442 --test-threads 1` (signal: 11, SIGSEGV: invalid memory reference)
```


Discussion / hints:
When testing on CentOS:
* The test works fine with Rust 1.31.1, but fails with Rust 1.32.0.
* The test works fine with Rust 1.32.0 if jemallocator is used instead of the default allocator.
Running under `valgrind` reveals that we have an invalid read while freeing the MutexGuard `let mut lock` in `notify_task` - which unlocks the mutex within `future_target`. The read is invalid because immediately prior, we freed the `future_target` as a consequence of freeing the `keep_alive` field of the `Awaiting` state of the future. Both of these frees occur on the `}` at the end of `notify_task`.

It is correct that we free both of these - the `keep_alive` field, as described in the comments, represents the foreign pointer from C into Rust, but that pointer has now been dropped by the C code; obviously when we exit a mutex block we must unlock the mutex.

This code is incorrect, and it's not obvious why it works with jemallocator. We must unlock and then free, rather than vice versa.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cfadb32..b98393a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -17,6 +17,8 @@ version number is tracked in the file `VERSION`.
 
 ### Fixed
 - `Value::get_inet` which would always return a zeroed `Inet`
+- Dropping futures early could cause a segfault when using the system
+  allocator (in Rust 1.32.0 or later).
 
 ## [0.13.2] - 2019-01-15
 - Avoid possible segfaults, by returning `None` where possible, otherwise
diff --git a/src/cassandra/future.rs b/src/cassandra/future.rs
index 9f0423a..901fcb5 100644
--- a/src/cassandra/future.rs
+++ b/src/cassandra/future.rs
@@ -238,8 +238,10 @@ enum FutureState {
 unsafe extern "C" fn notify_task(_c_future: *mut _Future, data: *mut ::std::os::raw::c_void) {
     let future_target: &FutureTarget = &*(data as *const FutureTarget);
     // The future is now ready, so transition to the appropriate state.
-    let mut lock = future_target.inner.lock().expect("notify_task");
-    let state = mem::replace(&mut *lock, FutureState::Ready);
+    let state = {
+        let mut lock = future_target.inner.lock().expect("notify_task");
+        mem::replace(&mut *lock, FutureState::Ready)
+    };
     if let FutureState::Awaiting { ref task, .. } = state {
         task.notify();
     } else {


Test patch: