remove dc_get_fine_* method and validate_filename

src/context.rs

@@ -1,5 +1,7 @@
 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};
 
@@ -8,6 +10,7 @@ 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::*;
@@ -20,6 +23,7 @@ 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]
 ///
@@ -158,6 +162,57 @@ 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)
     }
@@ -438,6 +493,7 @@ pub fn get_version_str() -> &'static str {
 mod tests {
     use super::*;
 
+    use crate::dc_tools::*;
     use crate::test_utils::*;
 
     #[test]
@@ -475,6 +531,51 @@ 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,28 +776,34 @@ impl<'a> MimeParser<'a> {
         decoded_data: &[u8],
         desired_filename: &str,
     ) {
-        /* create a free file name to use */
-        let path_filename = dc_get_fine_path_filename(self.context, "$BLOBDIR", desired_filename);
-
-        /* 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);
+        /* 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;
             }
+        };
 
-            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);
+        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);
+            }
+        }
+        self.do_add_single_part(part);
     }
 
     fn do_add_single_part(&mut self, mut part: Part) {

src/dc_tools.rs

@@ -1,8 +1,9 @@
 //! 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, OsString};
+use std::ffi::{CStr, CString};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::time::SystemTime;
@@ -415,12 +416,41 @@ pub(crate) fn dc_ensure_no_slash_safe(path: &str) -> &str {
     path
 }
 
-/// 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(':', "-")
+// 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)
 }
 
 // the returned suffix is lower-case
@@ -566,49 +596,24 @@ pub fn dc_read_file<P: AsRef<std::path::Path>>(
     }
 }
 
-pub(crate) fn dc_get_fine_path_filename(
-    context: &Context,
+pub(crate) fn dc_get_next_backup_path(
     folder: impl AsRef<Path>,
-    desired_filename_suffix: impl AsRef<str>,
-) -> PathBuf {
-    let now = time();
-
+    backup_time: i64,
+) -> Result<PathBuf, Error> {
     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());
+    let stem = chrono::NaiveDateTime::from_timestamp(backup_time, 0)
+        .format("delta-chat-%Y-%m-%d")
+        .to_string();
 
-    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;
+    // 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);
         }
     }
-
-    panic!("Something is really wrong, you need to clean up your disk");
+    bail!("could not create backup file, disk full?");
 }
 
 pub(crate) fn dc_is_blobdir_path(context: &Context, path: impl AsRef<str>) -> bool {
@@ -636,13 +641,10 @@ pub(crate) fn dc_make_rel_and_copy(context: &Context, path: &mut String) -> bool
         dc_make_rel_path(context, path);
         return true;
     }
-    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);
+    if let Ok(blobdir_path) = context.copy_to_blobdir(&path) {
+        *path = blobdir_path;
         return true;
     }
-
     false
 }
 
@@ -1437,6 +1439,19 @@ 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();
@@ -1471,6 +1486,7 @@ 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();
@@ -1483,14 +1499,12 @@ 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,14 +129,15 @@ pub fn initiate_key_transfer(context: &Context) -> Result<String> {
                 .unwrap()
                 .shall_stop_ongoing
             {
-                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()) {
+                let setup_file_name = context.new_blob_file(
+                    "autocrypt-setup-message.html",
+                    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.to_string_lossy());
+                        msg.param.set(Param::File, setup_file_name);
 
                         msg.param
                             .set(Param::MimeType, "application/autocrypt-setup");
@@ -573,13 +574,10 @@ 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. however, currently it is not clear it the import exists in the long run (may be replaced by a restore-from-imap)
+    // 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);
     let now = time();
-    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);
+    let dest_path_filename = dc_get_next_backup_path(dir, now)?;
 
     sql::housekeeping(context);
 

src/job.rs

@@ -965,41 +965,39 @@ 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,
         )
     };
-    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
+    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
 }
 
 pub fn job_add(