Make dc_msg_get_summarytext_by_raw safe (#316)

* Make dc_msg_get_summarytext_by_raw safe

* use dc_truncate method in all places

* Fix tests and add docs to dc_truncate()

* Make text argument an AsRef<&str> and rename type_0 to viewtype

* Fix too early return in dc_msg_get_summarytext_by_raw. Fixes https://github.com/deltachat/deltachat-core-rust/issues/313

deltachat-ffi/src/lib.rs

@@ -12,6 +12,7 @@ extern crate human_panic;
 extern crate num_traits;
 
 use num_traits::{FromPrimitive, ToPrimitive};
+use std::convert::TryInto;
 use std::str::FromStr;
 
 use deltachat::contact::Contact;
@@ -1510,7 +1511,7 @@ pub unsafe extern "C" fn dc_msg_get_summarytext(
 ) -> *mut libc::c_char {
     assert!(!msg.is_null());
 
-    dc_msg::dc_msg_get_summarytext(msg, approx_characters)
+    dc_msg::dc_msg_get_summarytext(msg, approx_characters.try_into().unwrap())
 }
 
 #[no_mangle]

src/dc_lot.rs

@@ -166,15 +166,14 @@ pub unsafe fn dc_lot_fill(
         }
     }
 
-    let message_text = (*msg).text.as_ref().unwrap();
-
     (*lot).text2 = dc_msg_get_summarytext_by_raw(
         (*msg).type_0,
-        message_text.strdup(),
+        (*msg).text.as_ref(),
         &mut (*msg).param,
         160,
         context,
-    );
+    )
+    .strdup();
 
     (*lot).timestamp = dc_msg_get_timestamp(msg);
     (*lot).state = (*msg).state;

src/dc_mimefactory.rs

@@ -9,7 +9,6 @@ use mmime::mailmime_types_helper::*;
 use mmime::mailmime_write_mem::*;
 use mmime::mmapstring::*;
 use mmime::other::*;
-use std::ptr;
 
 use crate::constants::*;
 use crate::contact::*;
@@ -1075,13 +1074,14 @@ unsafe fn get_subject(
     let ret: *mut libc::c_char;
 
     let raw_subject = {
-        let msgtext_c = (*msg)
+        dc_msg_get_summarytext_by_raw(
-            .text
+            (*msg).type_0,
-            .as_ref()
+            (*msg).text.as_ref(),
-            .map(|s| CString::yolo(String::as_str(s)));
+            &mut (*msg).param,
-        let msgtext_ptr = msgtext_c.map_or(ptr::null(), |s| s.as_ptr());
+            32,
-
+            context,
-        dc_msg_get_summarytext_by_raw((*msg).type_0, msgtext_ptr, &mut (*msg).param, 32, context)
+        )
+        .strdup()
     };
 
     let fwd = if 0 != afwd_email {

src/dc_msg.rs

@@ -67,7 +67,7 @@ pub unsafe fn dc_get_msg_info(context: &Context, msg_id: u32) -> *mut libc::c_ch
         return ret.strdup();
     }
     let rawtxt = rawtxt.unwrap();
-    let rawtxt = dc_truncate_str(rawtxt.trim(), 100000);
+    let rawtxt = dc_truncate(rawtxt.trim(), 100000, false);
 
     let fts = dc_timestamp_to_str(dc_msg_get_timestamp(msg));
     ret += &format!("Sent: {}", fts);
@@ -669,7 +669,7 @@ pub unsafe fn dc_msg_get_text(msg: *const dc_msg_t) -> *mut libc::c_char {
         return dc_strdup(0 as *const libc::c_char);
     }
     if let Some(ref text) = (*msg).text {
-        dc_truncate_str(text, 30000).strdup()
+        dc_truncate(text, 30000, false).strdup()
     } else {
         ptr::null_mut()
     }
@@ -780,105 +780,82 @@ pub unsafe fn dc_msg_get_summary<'a>(
 
 pub unsafe fn dc_msg_get_summarytext(
     msg: *mut dc_msg_t,
-    approx_characters: libc::c_int,
+    approx_characters: usize,
 ) -> *mut libc::c_char {
     if msg.is_null() {
         return dc_strdup(0 as *const libc::c_char);
     }
 
-    let msgtext_c = (*msg)
-        .text
-        .as_ref()
-        .map(|s| CString::yolo(String::as_str(s)));
-    let msgtext_ptr = msgtext_c.map_or(ptr::null(), |s| s.as_ptr());
-
     dc_msg_get_summarytext_by_raw(
         (*msg).type_0,
-        msgtext_ptr,
+        (*msg).text.as_ref(),
         &mut (*msg).param,
         approx_characters,
         (*msg).context,
     )
+    .strdup()
 }
 
-/* the returned value must be free()'d */
+/// Returns a summary test.
-#[allow(non_snake_case)]
+pub fn dc_msg_get_summarytext_by_raw(
-pub unsafe fn dc_msg_get_summarytext_by_raw(
+    viewtype: Viewtype,
-    type_0: Viewtype,
+    text: Option<impl AsRef<str>>,
-    text: *const libc::c_char,
     param: &mut Params,
-    approx_characters: libc::c_int,
+    approx_characters: usize,
     context: &Context,
-) -> *mut libc::c_char {
+) -> String {
-    /* get a summary text, result must be free()'d, never returns NULL. */
+    let mut append_text = true;
-    let mut ret;
+    let prefix = match viewtype {
-    let mut prefix: *mut libc::c_char = 0 as *mut libc::c_char;
+        Viewtype::Image => context.stock_str(StockMessage::Image).into_owned(),
-    let mut pathNfilename: *mut libc::c_char = 0 as *mut libc::c_char;
+        Viewtype::Gif => context.stock_str(StockMessage::Gif).into_owned(),
-    let mut value: *mut libc::c_char = 0 as *mut libc::c_char;
+        Viewtype::Video => context.stock_str(StockMessage::Video).into_owned(),
-    let mut append_text: libc::c_int = 1i32;
+        Viewtype::Voice => context.stock_str(StockMessage::VoiceMessage).into_owned(),
-    match type_0 {
-        Viewtype::Image => prefix = context.stock_str(StockMessage::Image).strdup(),
-        Viewtype::Gif => prefix = context.stock_str(StockMessage::Gif).strdup(),
-        Viewtype::Video => prefix = context.stock_str(StockMessage::Video).strdup(),
-        Viewtype::Voice => prefix = context.stock_str(StockMessage::VoiceMessage).strdup(),
         Viewtype::Audio | Viewtype::File => {
             if param.get_int(Param::Cmd) == Some(6) {
-                prefix = context.stock_str(StockMessage::AcSetupMsgSubject).strdup();
+                append_text = false;
-                append_text = 0i32
-            } else {
-                pathNfilename = param
-                    .get(Param::File)
-                    .unwrap_or_else(|| "ErrFilename")
-                    .strdup();
-                value = dc_get_filename(pathNfilename);
-                let label = CString::new(
                 context
-                        .stock_str(if type_0 == Viewtype::Audio {
+                    .stock_str(StockMessage::AcSetupMsgSubject)
+                    .to_string()
+            } else {
+                let file_name: String = if let Some(file_path) = param.get(Param::File) {
+                    if let Some(file_name) = Path::new(file_path).file_name() {
+                        Some(file_name.to_string_lossy().into_owned())
+                    } else {
+                        None
+                    }
+                } else {
+                    None
+                }
+                .unwrap_or("ErrFileName".to_string());
+
+                let label = context.stock_str(if viewtype == Viewtype::Audio {
                     StockMessage::Audio
                 } else {
                     StockMessage::File
-                        })
+                });
-                        .as_ref(),
+                format!("{} – {}", label, file_name)
-                )
-                .unwrap();
-                prefix = dc_mprintf(
-                    b"%s \xe2\x80\x93 %s\x00" as *const u8 as *const libc::c_char,
-                    label.as_ptr(),
-                    value,
-                )
             }
         }
         _ => {
-            if param.get_int(Param::Cmd) == Some(9) {
+            if param.get_int(Param::Cmd) != Some(9) {
-                prefix = context.stock_str(StockMessage::Location).strdup();
+                "".to_string()
-                append_text = 0;
-            }
-        }
-    }
-    if 0 != append_text
-        && !prefix.is_null()
-        && !text.is_null()
-        && 0 != *text.offset(0isize) as libc::c_int
-    {
-        ret = dc_mprintf(
-            b"%s \xe2\x80\x93 %s\x00" as *const u8 as *const libc::c_char,
-            prefix,
-            text,
-        );
-        dc_truncate_n_unwrap_str(ret, approx_characters, 1i32);
-    } else if 0 != append_text && !text.is_null() && 0 != *text.offset(0isize) as libc::c_int {
-        ret = dc_strdup(text);
-        dc_truncate_n_unwrap_str(ret, approx_characters, 1i32);
             } else {
-        ret = prefix;
+                append_text = false;
-        prefix = 0 as *mut libc::c_char
+                context.stock_str(StockMessage::Location).to_string()
             }
-    free(prefix as *mut libc::c_void);
-    free(pathNfilename as *mut libc::c_void);
-    free(value as *mut libc::c_void);
-    if ret.is_null() {
-        ret = dc_strdup(0 as *const libc::c_char)
         }
+    };
+    let ret = if append_text && text.is_some() {
+        let text = text.unwrap();
+        if !prefix.is_empty() {
+            let tmp = format!("{} – {}", prefix, text.as_ref());
+            dc_truncate(&tmp, approx_characters, true).to_string()
+        } else {
+            dc_truncate(text.as_ref(), approx_characters, true).to_string()
+        }
+    } else {
+        prefix
+    };
 
     ret
 }

src/dc_tools.rs

@@ -14,8 +14,6 @@ use crate::error::Error;
 use crate::types::*;
 use crate::x::*;
 
-const ELLIPSE: &str = "[...]";
-
 /* Some tools and enhancements to the used libraries, there should be
 no references to Context and other "larger" classes here. */
 /* ** library-private **********************************************************/
@@ -311,12 +309,25 @@ unsafe fn dc_utf8_strlen(s: *const libc::c_char) -> size_t {
     j
 }
 
-pub fn dc_truncate_str(buf: &str, approx_chars: usize) -> Cow<str> {
+/// Shortens a string to a specified length and adds "..." or "[...]" to the end of
-    if approx_chars > 0 && buf.len() > approx_chars + ELLIPSE.len() {
+/// the shortened string.
+///
+/// # Examples
+/// ```
+/// use deltachat::dc_tools::dc_truncate;
+///
+/// let s = "this is a little test string";
+/// assert_eq!(dc_truncate(s, 16, false), "this is a [...]");
+/// assert_eq!(dc_truncate(s, 16, true), "this is a ...");
+/// ```
+pub fn dc_truncate(buf: &str, approx_chars: usize, do_unwrap: bool) -> Cow<str> {
+    let ellipse = if do_unwrap { "..." } else { "[...]" };
+
+    if approx_chars > 0 && buf.len() > approx_chars + ellipse.len() {
         if let Some(index) = buf[..approx_chars].rfind(|c| c == ' ' || c == '\n') {
-            Cow::Owned(format!("{}{}", &buf[..index + 1], ELLIPSE))
+            Cow::Owned(format!("{}{}", &buf[..index + 1], ellipse))
         } else {
-            Cow::Owned(format!("{}{}", &buf[..approx_chars], ELLIPSE))
+            Cow::Owned(format!("{}{}", &buf[..approx_chars], ellipse))
         }
     } else {
         Cow::Borrowed(buf)
@@ -1640,25 +1651,28 @@ mod tests {
     }
 
     #[test]
-    fn test_dc_str_truncate_1() {
+    fn test_dc_truncate_1() {
         let s = "this is a little test string";
-        assert_eq!(dc_truncate_str(s, 16), "this is a [...]");
+        assert_eq!(dc_truncate(s, 16, false), "this is a [...]");
+        assert_eq!(dc_truncate(s, 16, true), "this is a ...");
     }
 
     #[test]
-    fn test_dc_str_truncate_2() {
+    fn test_dc_truncate_2() {
-        assert_eq!(dc_truncate_str("1234", 2), "1234");
+        assert_eq!(dc_truncate("1234", 2, false), "1234");
+        assert_eq!(dc_truncate("1234", 2, true), "1234");
     }
 
-    // This test seems wrong
+    #[test]
-    // #[test]
+    fn test_dc_truncate_3() {
-    // fn test_dc_str_truncate_3() {
+        assert_eq!(dc_truncate("1234567", 1, false), "1[...]");
-    //     assert_eq!(dc_truncate_str("1234567", 3), "1[...]");
+        assert_eq!(dc_truncate("1234567", 1, true), "1...");
-    // }
+    }
 
     #[test]
-    fn test_dc_str_truncate_4() {
+    fn test_dc_truncate_4() {
-        assert_eq!(dc_truncate_str("123456", 4), "123456");
+        assert_eq!(dc_truncate("123456", 4, false), "123456");
+        assert_eq!(dc_truncate("123456", 4, true), "123456");
     }
 
     #[test]