Remove unsafe CString::yolo from ffi

CString::yolo was still used in the ffi, this was an unsafe
transitional thing.  To remove it there were two choices: 1. make
errors in creating CStrings hard errors or 2. try and be as lenient as
possible.  Given the to_string_lossy() convention adopted in the ffi
this choose the lenient option and simply skips over embedded null
bytes, leaving the rest of the strings intact.

Thus now CString::new_lossy().  It's only used for .strdup() however
so no longer a public trait.

This also cleans up the public visibility of things in the strings.rs
file:

- Rename StrExt/OptStrExt traits to what they actually do: provide
  .strdup() -> Strdup/OptStrdup.

- dc_strdup() should be an implementation detail, replace all usages
  with Strdup.strdup() method.

- Only allow visibility inside the crate for all things.

- Reduce visibility to only the module for things not used in lib.rs.

deltachat-ffi/src/lib.rs

@@ -433,7 +433,7 @@ pub unsafe extern "C" fn dc_set_config_from_qr(
 pub unsafe extern "C" fn dc_get_info(context: *mut dc_context_t) -> *mut libc::c_char {
     if context.is_null() {
         eprintln!("ignoring careless call to dc_get_info()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_context = &*context;
     let guard = ffi_context.inner.read().unwrap();
@@ -1455,7 +1455,7 @@ pub unsafe extern "C" fn dc_get_msg_info(
 ) -> *mut libc::c_char {
     if context.is_null() {
         eprintln!("ignoring careless call to dc_get_msg_info()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_context = &*context;
     ffi_context
@@ -2406,7 +2406,7 @@ pub unsafe extern "C" fn dc_chat_get_type(chat: *mut dc_chat_t) -> libc::c_int {
 pub unsafe extern "C" fn dc_chat_get_name(chat: *mut dc_chat_t) -> *mut libc::c_char {
     if chat.is_null() {
         eprintln!("ignoring careless call to dc_chat_get_name()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_chat = &*chat;
     ffi_chat.chat.get_name().strdup()
@@ -2728,7 +2728,7 @@ pub unsafe extern "C" fn dc_msg_get_sort_timestamp(msg: *mut dc_msg_t) -> i64 {
 pub unsafe extern "C" fn dc_msg_get_text(msg: *mut dc_msg_t) -> *mut libc::c_char {
     if msg.is_null() {
         eprintln!("ignoring careless call to dc_msg_get_text()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_msg = &*msg;
     ffi_msg.message.get_text().unwrap_or_default().strdup()
@@ -2747,8 +2747,7 @@ pub unsafe extern "C" fn dc_msg_get_file(msg: *mut dc_msg_t) -> *mut libc::c_cha
             ffi_msg
                 .message
                 .get_file(ctx)
-                .and_then(|p| p.to_c_string().ok())
+                .map(|p| p.strdup())
-                .map(|cs| dc_strdup(cs.as_ptr()))
                 .unwrap_or_else(|| "".strdup())
         })
         .unwrap_or_else(|_| "".strdup())
@@ -2758,7 +2757,7 @@ pub unsafe extern "C" fn dc_msg_get_file(msg: *mut dc_msg_t) -> *mut libc::c_cha
 pub unsafe extern "C" fn dc_msg_get_filename(msg: *mut dc_msg_t) -> *mut libc::c_char {
     if msg.is_null() {
         eprintln!("ignoring careless call to dc_msg_get_filename()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_msg = &*msg;
     ffi_msg.message.get_filename().unwrap_or_default().strdup()
@@ -2768,13 +2767,13 @@ pub unsafe extern "C" fn dc_msg_get_filename(msg: *mut dc_msg_t) -> *mut libc::c
 pub unsafe extern "C" fn dc_msg_get_filemime(msg: *mut dc_msg_t) -> *mut libc::c_char {
     if msg.is_null() {
         eprintln!("ignoring careless call to dc_msg_get_filemime()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_msg = &*msg;
     if let Some(x) = ffi_msg.message.get_filemime() {
         x.strdup()
     } else {
-        dc_strdup(ptr::null())
+        "".strdup()
     }
 }
 
@@ -3098,7 +3097,7 @@ pub unsafe extern "C" fn dc_contact_get_id(contact: *mut dc_contact_t) -> u32 {
 pub unsafe extern "C" fn dc_contact_get_addr(contact: *mut dc_contact_t) -> *mut libc::c_char {
     if contact.is_null() {
         eprintln!("ignoring careless call to dc_contact_get_addr()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_contact = &*contact;
     ffi_contact.contact.get_addr().strdup()
@@ -3108,7 +3107,7 @@ pub unsafe extern "C" fn dc_contact_get_addr(contact: *mut dc_contact_t) -> *mut
 pub unsafe extern "C" fn dc_contact_get_name(contact: *mut dc_contact_t) -> *mut libc::c_char {
     if contact.is_null() {
         eprintln!("ignoring careless call to dc_contact_get_name()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_contact = &*contact;
     ffi_contact.contact.get_name().strdup()
@@ -3120,7 +3119,7 @@ pub unsafe extern "C" fn dc_contact_get_display_name(
 ) -> *mut libc::c_char {
     if contact.is_null() {
         eprintln!("ignoring careless call to dc_contact_get_display_name()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_contact = &*contact;
     ffi_contact.contact.get_display_name().strdup()
@@ -3132,7 +3131,7 @@ pub unsafe extern "C" fn dc_contact_get_name_n_addr(
 ) -> *mut libc::c_char {
     if contact.is_null() {
         eprintln!("ignoring careless call to dc_contact_get_name_n_addr()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_contact = &*contact;
     ffi_contact.contact.get_name_n_addr().strdup()
@@ -3144,7 +3143,7 @@ pub unsafe extern "C" fn dc_contact_get_first_name(
 ) -> *mut libc::c_char {
     if contact.is_null() {
         eprintln!("ignoring careless call to dc_contact_get_first_name()");
-        return dc_strdup(ptr::null());
+        return "".strdup();
     }
     let ffi_contact = &*contact;
     ffi_contact.contact.get_first_name().strdup()

deltachat-ffi/src/string.rs

@@ -9,7 +9,7 @@ use std::ptr;
 /// # Examples
 ///
 /// ```rust,norun
-/// use deltachat::dc_tools::{dc_strdup, to_string_lossy};
+/// use crate::string::{dc_strdup, to_string_lossy};
 /// unsafe {
 ///     let str_a = b"foobar\x00" as *const u8 as *const libc::c_char;
 ///     let str_a_copy = dc_strdup(str_a);
@@ -17,7 +17,7 @@ use std::ptr;
 ///     assert_ne!(str_a, str_a_copy);
 /// }
 /// ```
-pub unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char {
+unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char {
     let ret: *mut libc::c_char;
     if !s.is_null() {
         ret = libc::strdup(s);
@@ -32,7 +32,7 @@ pub unsafe fn dc_strdup(s: *const libc::c_char) -> *mut libc::c_char {
 
 /// Error type for the [OsStrExt] trait
 #[derive(Debug, Fail, PartialEq)]
-pub enum CStringError {
+pub(crate) enum CStringError {
     /// The string contains an interior null byte
     #[fail(display = "String contains an interior null byte")]
     InteriorNullByte,
@@ -66,7 +66,7 @@ pub enum CStringError {
 ///     let mut c_ptr: *mut libc::c_char = dc_strdup(path_c.as_ptr());
 /// }
 /// ```
-pub trait OsStrExt {
+pub(crate) trait OsStrExt {
     /// Convert a  [std::ffi::OsStr] to an [std::ffi::CString]
     ///
     /// This is useful to convert e.g. a [std::path::Path] to
@@ -131,15 +131,16 @@ fn os_str_to_c_string_unicode(
 }
 
 /// Convenience methods/associated functions for working with [CString]
-///
+trait CStringExt {
-/// This is helps transitioning from unsafe code.
+    /// Create a new [CString], best effort
-pub trait CStringExt {
-    /// Create a new [CString], yolo style
     ///
-    /// This unwrap the result, panicking when there are embedded NULL
+    /// Like the [to_string_lossy] this doesn't give up in the face of
-    /// bytes.
+    /// bad input (embedded null bytes in this case) instead it does
-    fn yolo<T: Into<Vec<u8>>>(t: T) -> CString {
+    /// the best it can by stripping the embedded null bytes.
-        CString::new(t).expect("String contains null byte, can not be CString")
+    fn new_lossy<T: Into<Vec<u8>>>(t: T) -> CString {
+        let mut s = t.into();
+        s.retain(|&c| c != 0);
+        CString::new(s).unwrap_or_default()
     }
 }
 
@@ -151,7 +152,7 @@ impl CStringExt for CString {}
 /// Rust strings to raw C strings.  This can be clumsy to do correctly
 /// and the compiler sometimes allows it in an unsafe way.  These
 /// methods make it more succinct and help you get it right.
-pub trait StrExt {
+pub(crate) trait Strdup {
     /// Allocate a new raw C `*char` version of this string.
     ///
     /// This allocates a new raw C string which must be freed using
@@ -168,35 +169,44 @@ pub trait StrExt {
     unsafe fn strdup(&self) -> *mut libc::c_char;
 }
 
-impl<T: AsRef<str>> StrExt for T {
+impl<T: AsRef<str>> Strdup for T {
     unsafe fn strdup(&self) -> *mut libc::c_char {
-        let tmp = CString::yolo(self.as_ref());
+        let tmp = CString::new_lossy(self.as_ref());
+        dc_strdup(tmp.as_ptr())
+    }
+}
+
+// We can not implement for AsRef<OsStr> because we already implement
+// AsRev<str> and this conflicts.  So implement for Path directly.
+impl Strdup for std::path::Path {
+    unsafe fn strdup(&self) -> *mut libc::c_char {
+        let tmp = self.to_c_string().unwrap_or_else(|_| CString::default());
         dc_strdup(tmp.as_ptr())
     }
 }
 
 /// Convenience methods to turn optional strings into C strings.
 ///
-/// This is the same as the [StrExt] trait but a different trait name
+/// This is the same as the [Strdup] trait but a different trait name
-/// to work around the type system not allowing to implement [StrExt]
+/// to work around the type system not allowing to implement [Strdup]
-/// for `Option<impl StrExt>` When we already have an [StrExt] impl
+/// for `Option<impl Strdup>` When we already have an [Strdup] impl
 /// for `AsRef<&str>`.
 ///
 /// When the [Option] is [Option::Some] this behaves just like
-/// [StrExt::strdup], when it is [Option::None] a null pointer is
+/// [Strdup::strdup], when it is [Option::None] a null pointer is
 /// returned.
-pub trait OptStrExt {
+pub(crate) trait OptStrdup {
     /// Allocate a new raw C `*char` version of this string, or NULL.
     ///
-    /// See [StrExt::strdup] for details.
+    /// See [Strdup::strdup] for details.
     unsafe fn strdup(&self) -> *mut libc::c_char;
 }
 
-impl<T: AsRef<str>> OptStrExt for Option<T> {
+impl<T: AsRef<str>> OptStrdup for Option<T> {
     unsafe fn strdup(&self) -> *mut libc::c_char {
         match self {
             Some(s) => {
-                let tmp = CString::yolo(s.as_ref());
+                let tmp = CString::new_lossy(s.as_ref());
                 dc_strdup(tmp.as_ptr())
             }
             None => ptr::null_mut(),
@@ -204,7 +214,7 @@ impl<T: AsRef<str>> OptStrExt for Option<T> {
     }
 }
 
-pub fn to_string_lossy(s: *const libc::c_char) -> String {
+pub(crate) fn to_string_lossy(s: *const libc::c_char) -> String {
     if s.is_null() {
         return "".into();
     }
@@ -214,7 +224,7 @@ pub fn to_string_lossy(s: *const libc::c_char) -> String {
     cstr.to_string_lossy().to_string()
 }
 
-pub fn to_opt_string_lossy(s: *const libc::c_char) -> Option<String> {
+pub(crate) fn to_opt_string_lossy(s: *const libc::c_char) -> Option<String> {
     if s.is_null() {
         return None;
     }
@@ -235,7 +245,7 @@ pub fn to_opt_string_lossy(s: *const libc::c_char) -> Option<String> {
 ///
 /// [Path]: std::path::Path
 #[cfg(not(target_os = "windows"))]
-pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path {
+pub(crate) fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path {
     assert!(!s.is_null(), "cannot be used on null pointers");
     use std::os::unix::ffi::OsStrExt;
     unsafe {
@@ -247,7 +257,7 @@ pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path {
 
 // as_path() implementation for windows, documented above.
 #[cfg(target_os = "windows")]
-pub fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path {
+pub(crate) fn as_path<'a>(s: *const libc::c_char) -> &'a std::path::Path {
     as_path_unicode(s)
 }
 
@@ -354,8 +364,14 @@ mod tests {
     }
 
     #[test]
-    fn test_cstring_yolo() {
+    fn test_cstring_new_lossy() {
-        assert_eq!(CString::new("hello").unwrap(), CString::yolo("hello"));
+        assert!(CString::new("hel\x00lo").is_err());
+        assert!(CString::new(String::from("hel\x00o")).is_err());
+        let r = CString::new("hello").unwrap();
+        assert_eq!(CString::new_lossy("hello"), r);
+        assert_eq!(CString::new_lossy("hel\x00lo"), r);
+        assert_eq!(CString::new_lossy(String::from("hello")), r);
+        assert_eq!(CString::new_lossy(String::from("hel\x00lo")), r);
     }
 
     #[test]

tests/stress.rs

@@ -44,9 +44,9 @@ fn stress_functions(context: &Context) {
     // assert!(dc_is_configured(context) != 0, "Missing configured context");
 
     // let setupcode = dc_create_setup_code(context);
-    // let setupcode_c = CString::yolo(setupcode.clone());
+    // let setupcode_c = CString::new(setupcode.clone()).unwrap();
     // let setupfile = dc_render_setup_file(context, &setupcode).unwrap();
-    // let setupfile_c = CString::yolo(setupfile);
+    // let setupfile_c = CString::new(setupfile).unwrap();
     // let mut headerline_2: *const libc::c_char = ptr::null();
     // let payload = dc_decrypt_setup_file(context, setupcode_c.as_ptr(), setupfile_c.as_ptr());