Auto merge of #36264 - matklad:zeroing-cstring, r=alexcrichton

Zero first byte of CString on drop

Hi! This is one more attempt to ameliorate `CString::new("...").unwrap().as_ptr()` problem (related RFC: https://github.com/rust-lang/rfcs/pull/1642).

One of the biggest problems with this code is that it may actually work in practice, so the idea of this PR is to proactively break such invalid code.

Looks like writing a `null` byte at the start of the CString should do the trick, and I think is an affordable cost: zeroing a single byte in `Drop` should be cheap enough compared to actual memory deallocation which would follow.

I would actually prefer to do something like

```Rust
impl Drop for CString {
    fn drop(&mut self) {
        let pattern = b"CTHULHU FHTAGN ";
        let bytes = self.inner[..self.inner.len() - 1];
        for (d, s) in bytes.iter_mut().zip(pattern.iter().cycle()) {
            *d = *s;
        }
    }
}
```

because Cthulhu error should be much easier to google, but unfortunately this would be too expensive in release builds, and we can't implement things `cfg(debug_assertions)` conditionally in stdlib.

Not sure if the whole idea or my implementation (I've used ~~`transmute`~~ `mem::unitialized` to workaround move out of Drop thing) makes sense :)

2 files changed, 74 insertions, 3 deletions

diff --git a/src/libstd/ffi/c_str.rs b/src/libstd/ffi/c_str.rs
index 38222c014f6..2d5e8c04194 100644
--- a/src/libstd/ffi/c_str.rs
+++ b/src/libstd/ffi/c_str.rs
@@ -19,6 +19,7 @@ use mem;
 use memchr;
 use ops;
 use os::raw::c_char;
+use ptr;
 use slice;
 use str::{self, Utf8Error};
 
@@ -68,6 +69,9 @@ use str::{self, Utf8Error};
 #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Clone)]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct CString {
+    // Invariant 1: the slice ends with a zero byte and has a length of at least one.
+    // Invariant 2: the slice contains only one zero byte.
+    // Improper usage of unsafe function can break Invariant 2, but not Invariant 1.
     inner: Box<[u8]>,
 }
 
@@ -244,7 +248,7 @@ impl CString {
     /// Failure to call `from_raw` will lead to a memory leak.
     #[stable(feature = "cstr_memory", since = "1.4.0")]
     pub fn into_raw(self) -> *mut c_char {
-        Box::into_raw(self.inner) as *mut c_char
+        Box::into_raw(self.into_inner()) as *mut c_char
     }
 
     /// Converts the `CString` into a `String` if it contains valid Unicode data.
@@ -265,7 +269,7 @@ impl CString {
     /// it is guaranteed to not have any interior nul bytes.
     #[stable(feature = "cstring_into", since = "1.7.0")]
     pub fn into_bytes(self) -> Vec<u8> {
-        let mut vec = self.inner.into_vec();
+        let mut vec = self.into_inner().into_vec();
         let _nul = vec.pop();
         debug_assert_eq!(_nul, Some(0u8));
         vec
@@ -275,7 +279,7 @@ impl CString {
     /// includes the trailing nul byte.
     #[stable(feature = "cstring_into", since = "1.7.0")]
     pub fn into_bytes_with_nul(self) -> Vec<u8> {
-        self.inner.into_vec()
+        self.into_inner().into_vec()
     }
 
     /// Returns the contents of this `CString` as a slice of bytes.
@@ -293,6 +297,24 @@ impl CString {
     pub fn as_bytes_with_nul(&self) -> &[u8] {
         &self.inner
     }
+
+    // Bypass "move out of struct which implements `Drop` trait" restriction.
+    fn into_inner(self) -> Box<[u8]> {
+        unsafe {
+            let result = ptr::read(&self.inner);
+            mem::forget(self);
+            result
+        }
+    }
+}
+
+// Turns this `CString` into an empty string to prevent
+// memory unsafe code from working by accident.
+#[stable(feature = "cstring_drop", since = "1.13.0")]
+impl Drop for CString {
+    fn drop(&mut self) {
+        unsafe { *self.inner.get_unchecked_mut(0) = 0; }
+    }
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
diff --git a/src/test/run-pass/cstring-drop.rs b/src/test/run-pass/cstring-drop.rs
new file mode 100644
index 00000000000..960391bb8de
--- /dev/null
+++ b/src/test/run-pass/cstring-drop.rs
@@ -0,0 +1,49 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// ignore-emscripten
+
+// Test that `CString::new("hello").unwrap().as_ptr()` pattern
+// leads to failure.
+
+use std::env;
+use std::ffi::{CString, CStr};
+use std::os::raw::c_char;
+use std::process::{Command, Stdio};
+
+fn main() {
+    let args: Vec<String> = env::args().collect();
+    if args.len() > 1 && args[1] == "child" {
+        // Repeat several times to be more confident that
+        // it is `Drop` for `CString` that does the cleanup,
+        // and not just some lucky UB.
+        let xs = vec![CString::new("Hello").unwrap(); 10];
+        let ys = xs.iter().map(|s| s.as_ptr()).collect::<Vec<_>>();
+        drop(xs);
+        assert!(ys.into_iter().any(is_hello));
+        return;
+    }
+
+    let output = Command::new(&args[0]).arg("child").output().unwrap();
+    assert!(!output.status.success());
+}
+
+fn is_hello(s: *const c_char) -> bool {
+    // `s` is a dangling pointer and reading it is technically
+    // undefined behavior. But we want to prevent the most diabolical
+    // kind of UB (apart from nasal demons): reading a value that was
+    // previously written.
+    //
+    // Segfaulting or reading an empty string is Ok,
+    // reading "Hello" is bad.
+    let s = unsafe { CStr::from_ptr(s) };
+    let hello = CString::new("Hello").unwrap();
+    s == hello.as_ref()
+}