bring back memory leak

(and possibly also related to #633)

src/context.rs

@@ -1,7 +1,5 @@
 use std::collections::HashMap;
 use std::ffi::OsString;
-use std::fs;
-use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::sync::{Arc, Condvar, Mutex, RwLock};
 
@@ -10,7 +8,6 @@ use libc::uintptr_t;
 use crate::chat::*;
 use crate::constants::*;
 use crate::contact::*;
-use crate::dc_tools::{dc_copy_file, dc_derive_safe_stem_ext};
 use crate::error::*;
 use crate::events::Event;
 use crate::imap::*;
@@ -23,7 +20,6 @@ use crate::message::{self, Message};
 use crate::param::Params;
 use crate::smtp::*;
 use crate::sql::Sql;
-use rand::{thread_rng, Rng};
 
 /// Callback function type for [Context]
 ///
@@ -162,57 +158,6 @@ impl Context {
         self.blobdir.as_path()
     }
 
-    pub fn copy_to_blobdir(&self, orig_filename: impl AsRef<str>) -> Result<String> {
-        // return a $BLOBDIR/<filename> with the content of orig_filename
-        // copied into it. The <filename> will be safely derived from
-        // orig_filename, and will not clash with existing filenames.
-        let dest = self.new_blob_file(&orig_filename, b"")?;
-        if dc_copy_file(
-            &self,
-            PathBuf::from(orig_filename.as_ref()),
-            PathBuf::from(&dest),
-        ) {
-            Ok(dest)
-        } else {
-            bail!("could not copy {} to {}", orig_filename.as_ref(), dest);
-        }
-    }
-
-    pub fn new_blob_file(&self, orig_filename: impl AsRef<str>, data: &[u8]) -> Result<String> {
-        // return a $BLOBDIR/<FILENAME> string which corresponds to the
-        // respective file in the blobdir, and which contains the data.
-        // FILENAME is computed by looking and possibly mangling the
-        // basename of orig_filename. The resulting filenames are meant
-        // to be human-readable.
-        let (stem, ext) = dc_derive_safe_stem_ext(orig_filename.as_ref());
-
-        // ext starts with "." or is empty string, so we can always resconstruct
-
-        for i in 0..3 {
-            let candidate_basename = match i {
-                // first a try to just use the (possibly mangled) original basename
-                0 => format!("{}{}", stem, ext),
-
-                // otherwise extend stem with random numbers
-                _ => {
-                    let mut rng = thread_rng();
-                    let random_id: u32 = rng.gen();
-                    format!("{}-{}{}", stem, random_id, ext)
-                }
-            };
-            let path = self.get_blobdir().join(&candidate_basename);
-            if let Ok(mut file) = fs::OpenOptions::new()
-                .create_new(true)
-                .write(true)
-                .open(&path)
-            {
-                file.write_all(data)?;
-                return Ok(format!("$BLOBDIR/{}", candidate_basename));
-            }
-        }
-        bail!("out of luck to create new blob file");
-    }
-
     pub fn call_cb(&self, event: Event) -> uintptr_t {
         (*self.cb)(self, event)
     }
@@ -493,7 +438,6 @@ pub fn get_version_str() -> &'static str {
 mod tests {
     use super::*;
 
-    use crate::dc_tools::*;
     use crate::test_utils::*;
 
     #[test]
@@ -531,51 +475,6 @@ mod tests {
         assert!(res.is_err());
     }
 
-    #[test]
-    fn test_new_blob_file() {
-        let t = dummy_context();
-        let context = t.ctx;
-        let x = &context.new_blob_file("hello", b"data").unwrap();
-        assert!(dc_file_exist(&context, x));
-        assert!(x.starts_with("$BLOBDIR"));
-        assert!(dc_read_file(&context, x).unwrap() == b"data");
-
-        let y = &context.new_blob_file("hello", b"data").unwrap();
-        assert!(dc_file_exist(&context, y));
-        assert!(y.starts_with("$BLOBDIR/hello-"));
-
-        let x = &context.new_blob_file("xyz/hello.png", b"data").unwrap();
-        assert!(dc_file_exist(&context, x));
-        assert_eq!(x, "$BLOBDIR/hello.png");
-
-        let y = &context.new_blob_file("hello\\world.png", b"data").unwrap();
-        assert!(dc_file_exist(&context, y));
-        assert_eq!(y, "$BLOBDIR/world.png");
-    }
-
-    #[test]
-    fn test_new_blob_file_long_names() {
-        let t = dummy_context();
-        let context = t.ctx;
-        let s = "12312312039182039182039812039810293810293810293810293801293801293123123";
-        let x = &context.new_blob_file(s, b"data").unwrap();
-        println!("blobfilename '{}'", x);
-        println!("xxxxfilename '{}'", s);
-        assert!(x.len() < s.len());
-        assert!(dc_file_exist(&context, x));
-        assert!(x.starts_with("$BLOBDIR"));
-    }
-
-    #[test]
-    fn test_new_blob_file_unicode() {
-        let t = dummy_context();
-        let context = t.ctx;
-        let s = "helloäworld.qwe";
-        let x = &context.new_blob_file(s, b"data").unwrap();
-        assert_eq!(x, "$BLOBDIR/hello-world.qwe");
-        assert_eq!(dc_read_file(&context, x).unwrap(), b"data");
-    }
-
     #[test]
     fn test_sqlite_parent_not_exists() {
         let tmp = tempfile::tempdir().unwrap();

src/dc_mimeparser.rs

@@ -776,34 +776,28 @@ impl<'a> MimeParser<'a> {
         decoded_data: &[u8],
         desired_filename: &str,
     ) {
-        /* write decoded data to new blob file */
-        let bpath = match self.context.new_blob_file(desired_filename, decoded_data) {
-            Ok(path) => path,
-            Err(err) => {
-                error!(
-                    self.context,
-                    "Could not add blob for mime part {}, error {}", desired_filename, err
-                );
-                return;
-            }
-        };
+        /* create a free file name to use */
+        let path_filename = dc_get_fine_path_filename(self.context, "$BLOBDIR", desired_filename);
 
-        let mut part = Part::default();
-        part.typ = msg_type;
-        part.mimetype = mime_type;
-        part.bytes = decoded_data.len() as libc::c_int;
-        part.param.set(Param::File, bpath);
-        if let Some(raw_mime) = raw_mime {
-            part.param.set(Param::MimeType, raw_mime);
-        }
-
-        if mime_type == DC_MIMETYPE_IMAGE {
-            if let Ok((width, height)) = dc_get_filemeta(decoded_data) {
-                part.param.set_int(Param::Width, width as i32);
-                part.param.set_int(Param::Height, height as i32);
+        /* copy data to file */
+        if dc_write_file(self.context, &path_filename, decoded_data) {
+            let mut part = Part::default();
+            part.typ = msg_type;
+            part.mimetype = mime_type;
+            part.bytes = decoded_data.len() as libc::c_int;
+            part.param.set(Param::File, path_filename.to_string_lossy());
+            if let Some(raw_mime) = raw_mime {
+                part.param.set(Param::MimeType, raw_mime);
             }
+
+            if mime_type == DC_MIMETYPE_IMAGE {
+                if let Ok((width, height)) = dc_get_filemeta(decoded_data) {
+                    part.param.set_int(Param::Width, width as i32);
+                    part.param.set_int(Param::Height, height as i32);
+                }
+            }
+            self.do_add_single_part(part);
         }
-        self.do_add_single_part(part);
     }
 
     fn do_add_single_part(&mut self, mut part: Part) {

src/dc_tools.rs

@@ -1,9 +1,8 @@
 //! Some tools and enhancements to the used libraries, there should be
 //! no references to Context and other "larger" entities here.
 
-use core::cmp::max;
 use std::borrow::Cow;
-use std::ffi::{CStr, CString};
+use std::ffi::{CStr, CString, OsString};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::time::SystemTime;
@@ -416,41 +415,12 @@ pub(crate) fn dc_ensure_no_slash_safe(path: &str) -> &str {
     path
 }
 
-// Function returns a sanitized basename that does not contain
-// win/linux path separators and also not any non-ascii chars
-fn get_safe_basename(filename: &str) -> String {
-    // return the (potentially mangled) basename of the input filename
-    // this might be a path that comes in from another operating system
-    let mut index: usize = 0;
-
-    if let Some(unix_index) = filename.rfind("/") {
-        index = unix_index + 1;
-    }
-    if let Some(win_index) = filename.rfind("\\") {
-        index = max(index, win_index + 1);
-    }
-    if index >= filename.len() {
-        "nobasename".to_string()
-    } else {
-        // we don't allow any non-ascii to be super-safe
-        filename[index..].replace(|c: char| !c.is_ascii() || c == ':', "-")
-    }
-}
-
-pub fn dc_derive_safe_stem_ext(filename: &str) -> (String, String) {
-    let basename = get_safe_basename(&filename);
-    let (mut stem, mut ext) = if let Some(index) = basename.rfind(".") {
-        (
-            basename[0..index].to_string(),
-            basename[index..].to_string(),
-        )
-    } else {
-        (basename, "".to_string())
-    };
-    // limit length of stem and ext
-    stem.truncate(32);
-    ext.truncate(32);
-    (stem, ext)
+/// Function modifies the given buffer and replaces all characters not valid in filenames by a "-".
+fn validate_filename(filename: &str) -> String {
+    filename
+        .replace('/', "-")
+        .replace('\\', "-")
+        .replace(':', "-")
 }
 
 // the returned suffix is lower-case
@@ -596,24 +566,49 @@ pub fn dc_read_file<P: AsRef<std::path::Path>>(
     }
 }
 
-pub(crate) fn dc_get_next_backup_path(
+pub(crate) fn dc_get_fine_path_filename(
+    context: &Context,
     folder: impl AsRef<Path>,
-    backup_time: i64,
-) -> Result<PathBuf, Error> {
-    let folder = PathBuf::from(folder.as_ref());
-    let stem = chrono::NaiveDateTime::from_timestamp(backup_time, 0)
-        .format("delta-chat-%Y-%m-%d")
-        .to_string();
+    desired_filename_suffix: impl AsRef<str>,
+) -> PathBuf {
+    let now = time();
 
-    // 64 backup files per day should be enough for everyone
-    for i in 0..64 {
-        let mut path = folder.clone();
-        path.push(format!("{}-{}.bak", stem, i));
-        if !path.exists() {
-            return Ok(path);
+    let folder = PathBuf::from(folder.as_ref());
+    // XXX sanitize desired_filename eg using
+    // https://github.com/kardeiz/sanitize-filename/blob/master/src/lib.rs
+    let suffix = validate_filename(desired_filename_suffix.as_ref());
+    let file_name = PathBuf::from(suffix);
+    let extension = file_name.extension().map(|c| c.clone());
+
+    for i in 0..100_000 {
+        let ret = if i == 0 {
+            let mut folder = folder.clone();
+            folder.push(&file_name);
+            folder
+        } else {
+            let idx = if i < 100 { i } else { now + i };
+            let file_name = if let Some(stem) = file_name.file_stem() {
+                let mut stem = stem.to_os_string();
+                stem.push(format!("-{}", idx));
+                stem
+            } else {
+                OsString::from(idx.to_string())
+            };
+            let mut folder = folder.clone();
+            folder.push(file_name);
+            if let Some(ext) = extension {
+                folder.set_extension(&ext);
+            }
+            folder
+        };
+
+        if !dc_file_exist(context, &ret) {
+            // fine filename found
+            return ret;
         }
     }
-    bail!("could not create backup file, disk full?");
+
+    panic!("Something is really wrong, you need to clean up your disk");
 }
 
 pub(crate) fn dc_is_blobdir_path(context: &Context, path: impl AsRef<str>) -> bool {
@@ -641,10 +636,13 @@ pub(crate) fn dc_make_rel_and_copy(context: &Context, path: &mut String) -> bool
         dc_make_rel_path(context, path);
         return true;
     }
-    if let Ok(blobdir_path) = context.copy_to_blobdir(&path) {
-        *path = blobdir_path;
+    let blobdir_path = dc_get_fine_path_filename(context, "$BLOBDIR", &path);
+    if dc_copy_file(context, &path, &blobdir_path) {
+        *path = blobdir_path.to_string_lossy().to_string();
+        dc_make_rel_path(context, path);
         return true;
     }
+
     false
 }
 
@@ -1439,19 +1437,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn test_file_get_safe_basename() {
-        assert_eq!(get_safe_basename("12312/hello"), "hello");
-        assert_eq!(get_safe_basename("12312\\hello"), "hello");
-        assert_eq!(get_safe_basename("//12312\\hello"), "hello");
-        assert_eq!(get_safe_basename("//123:12\\hello"), "hello");
-        assert_eq!(get_safe_basename("//123:12/\\\\hello"), "hello");
-        assert_eq!(get_safe_basename("//123:12//hello"), "hello");
-        assert_eq!(get_safe_basename("//123:12//"), "nobasename");
-        assert_eq!(get_safe_basename("//123:12/"), "nobasename");
-        assert!(get_safe_basename("123\x012.hello").ends_with(".hello"));
-    }
-
     #[test]
     fn test_file_handling() {
         let t = dummy_context();
@@ -1486,7 +1471,6 @@ mod tests {
         assert!(dc_file_exist(context, &abs_path));
 
         assert!(dc_copy_file(context, "$BLOBDIR/foobar", "$BLOBDIR/dada",));
-
         assert_eq!(dc_get_filebytes(context, "$BLOBDIR/dada",), 7);
 
         let buf = dc_read_file(context, "$BLOBDIR/dada").unwrap();
@@ -1499,12 +1483,14 @@ mod tests {
         assert!(dc_create_folder(context, "$BLOBDIR/foobar-folder"));
         assert!(dc_file_exist(context, "$BLOBDIR/foobar-folder",));
         assert!(!dc_delete_file(context, "$BLOBDIR/foobar-folder"));
+        let fn0 = dc_get_fine_path_filename(context, "$BLOBDIR", "foobar.dadada");
+        assert_eq!(fn0, PathBuf::from("$BLOBDIR/foobar.dadada"));
 
-        let fn0 = "$BLOBDIR/data.data";
         assert!(dc_write_file(context, &fn0, b"content"));
+        let fn1 = dc_get_fine_path_filename(context, "$BLOBDIR", "foobar.dadada");
+        assert_eq!(fn1, PathBuf::from("$BLOBDIR/foobar-1.dadada"));
 
         assert!(dc_delete_file(context, &fn0));
-        assert!(!dc_file_exist(context, &fn0));
     }
 
     #[test]

src/imex.rs

@@ -129,15 +129,14 @@ pub fn initiate_key_transfer(context: &Context) -> Result<String> {
                 .unwrap()
                 .shall_stop_ongoing
             {
-                let setup_file_name = context.new_blob_file(
-                    "autocrypt-setup-message.html",
-                    setup_file_content.as_bytes(),
-                )?;
-                {
+                let setup_file_name =
+                    dc_get_fine_path_filename(context, "$BLOBDIR", "autocrypt-setup-message.html");
+                if dc_write_file(context, &setup_file_name, setup_file_content.as_bytes()) {
                     if let Ok(chat_id) = chat::create_by_contact_id(context, 1) {
                         msg = Message::default();
                         msg.type_0 = Viewtype::File;
-                        msg.param.set(Param::File, setup_file_name);
+                        msg.param
+                            .set(Param::File, setup_file_name.to_string_lossy());
 
                         msg.param
                             .set(Param::MimeType, "application/autocrypt-setup");
@@ -574,10 +573,13 @@ fn export_backup(context: &Context, dir: impl AsRef<Path>) -> Result<()> {
 
     let mut delete_dest_file: libc::c_int = 0;
     // get a fine backup file name (the name includes the date so that multiple backup instances are possible)
-    // FIXME: we should write to a temporary file first and rename it on success. this would guarantee the backup is complete.
-    // let dest_path_filename = dc_get_next_backup_file(context, dir, res);
+    // FIXME: we should write to a temporary file first and rename it on success. this would guarantee the backup is complete. however, currently it is not clear it the import exists in the long run (may be replaced by a restore-from-imap)
     let now = time();
-    let dest_path_filename = dc_get_next_backup_path(dir, now)?;
+    let res = chrono::NaiveDateTime::from_timestamp(now as i64, 0)
+        .format("delta-chat-%Y-%m-%d.bak")
+        .to_string();
+
+    let dest_path_filename = dc_get_fine_path_filename(context, dir, res);
 
     sql::housekeeping(context);
 

src/job.rs

@@ -965,39 +965,41 @@ fn send_mdn(context: &Context, msg_id: u32) {
 
 #[allow(non_snake_case)]
 fn add_smtp_job(context: &Context, action: Action, mimefactory: &MimeFactory) -> libc::c_int {
+    let mut success: libc::c_int = 0i32;
     let mut param = Params::new();
+    let path_filename = dc_get_fine_path_filename(context, "$BLOBDIR", &mimefactory.rfc724_mid);
     let bytes = unsafe {
         std::slice::from_raw_parts(
             (*mimefactory.out).str_0 as *const u8,
             (*mimefactory.out).len,
         )
     };
-    let bpath = match context.new_blob_file(&mimefactory.rfc724_mid, bytes) {
-        Ok(path) => path,
-        Err(err) => {
-            error!(
-                context,
-                "Could not write {} smtp-message, error {}", mimefactory.rfc724_mid, err
-            );
-            return 0;
-        }
-    };
-    info!(context, "add_smtp_job file written: {:?}", bpath);
-    let recipients = mimefactory.recipients_addr.join("\x1e");
-    param.set(Param::File, &bpath);
-    param.set(Param::Recipients, &recipients);
-    job_add(
-        context,
-        action,
-        (if mimefactory.loaded == Loaded::Message {
-            mimefactory.msg.id
-        } else {
-            0
-        }) as libc::c_int,
-        param,
-        0,
-    );
-    1
+    if !dc_write_file(context, &path_filename, bytes) {
+        error!(
+            context,
+            "Could not write message <{}> to \"{}\".",
+            mimefactory.rfc724_mid,
+            path_filename.display(),
+        );
+    } else {
+        info!(context, "add_smtp_job file written: {:?}", path_filename);
+        let recipients = mimefactory.recipients_addr.join("\x1e");
+        param.set(Param::File, path_filename.to_string_lossy());
+        param.set(Param::Recipients, &recipients);
+        job_add(
+            context,
+            action,
+            (if mimefactory.loaded == Loaded::Message {
+                mimefactory.msg.id
+            } else {
+                0
+            }) as libc::c_int,
+            param,
+            0,
+        );
+        success = 1;
+    }
+    success
 }
 
 pub fn job_add(