From a3b90a08b6f57040ee4edfb526b42237371e6deb Mon Sep 17 00:00:00 2001
From: Floris Bruynooghe <flub@google.com>
Date: Sat, 2 Nov 2019 17:25:56 +0100
Subject: [PATCH] Copy the file contents manually

Before we created an empty file and asked the OS to copy the file.
The OS is very good at this so this is a good idea generally.  However
it seems that in some cases, possibly an Android Dowload folder, we
might be able to create a file but not overwrite it.  Thus refactor
this a bit so we are copying the file ourselves.

There are no new tests here since the behaviour remains identical.
The good news is that the existing tests were good enough to catch
some bugs already.
---
 src/blob.rs | 65 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/src/blob.rs b/src/blob.rs
index 89f113d48..2c6fdae81 100644
--- a/src/blob.rs
+++ b/src/blob.rs
@@ -25,7 +25,8 @@ impl<'a> BlobObject<'a> {
     /// Creates a new file in the blob directory.  The name will be
     /// derived from the platform-agnostic basename of the suggested
     /// name, followed by a random number and followed by a possible
-    /// extension.  The `data` will be written into the file.
+    /// extension.  The `data` will be written into the file without
+    /// race-conditions.
     ///
     /// # Errors
     ///
@@ -42,29 +43,33 @@ impl<'a> BlobObject<'a> {
         data: &[u8],
     ) -> std::result::Result<BlobObject<'a>, BlobError> {
         let blobdir = context.get_blobdir();
-        let (stem, ext) = BlobObject::sanitise_name(suggested_name.as_ref().to_string());
-        let mut name = format!("{}{}", stem, ext);
+        let (stem, ext) = BlobObject::sanitise_name(suggested_name.as_ref());
+        let (name, mut file) = BlobObject::create_new_file(&blobdir, &stem, &ext)?;
+        file.write_all(data)
+            .map_err(|err| BlobError::new_write_failure(blobdir, &name, err))?;
+        let blob = BlobObject {
+            blobdir,
+            name: format!("$BLOBDIR/{}", name),
+        };
+        context.call_cb(Event::NewBlobFile(blob.as_name().to_string()));
+        Ok(blob)
+    }
+
+    // Creates a new file, returning a tuple of the name and the handle.
+    fn create_new_file(dir: &Path, stem: &str, ext: &str) -> Result<(String, fs::File), BlobError> {
         let max_attempt = 15;
+        let mut name = format!("{}{}", stem, ext);
         for attempt in 0..max_attempt {
-            let path = blobdir.join(&name);
+            let path = dir.join(&name);
             match fs::OpenOptions::new()
                 .create_new(true)
                 .write(true)
                 .open(&path)
             {
-                Ok(mut file) => {
-                    file.write_all(data)
-                        .map_err(|err| BlobError::new_write_failure(blobdir, &name, err))?;
-                    let blob = BlobObject {
-                        blobdir,
-                        name: format!("$BLOBDIR/{}", name),
-                    };
-                    context.call_cb(Event::NewBlobFile(blob.as_name().to_string()));
-                    return Ok(blob);
-                }
+                Ok(file) => return Ok((name, file)),
                 Err(err) => {
                     if attempt == max_attempt {
-                        return Err(BlobError::new_create_failure(blobdir, &name, err));
+                        return Err(BlobError::new_create_failure(dir, &name, err));
                     } else {
                         name = format!("{}-{}{}", stem, rand::random::<u32>(), ext);
                     }
@@ -72,7 +77,7 @@ impl<'a> BlobObject<'a> {
             }
         }
         Err(BlobError::new_create_failure(
-            blobdir,
+            dir,
             &name,
             format_err!("Unreachable code - supposedly"),
         ))
@@ -81,7 +86,9 @@ impl<'a> BlobObject<'a> {
     /// Creates a new blob object with unique name by copying an existing file.
     ///
     /// This creates a new blob as described in [BlobObject::create]
-    /// but also copies an existing file into it.
+    /// but also copies an existing file into it.  This is done in a
+    /// in way which avoids race-conditions when multiple files are
+    /// concurrently created.
     ///
     /// # Errors
     ///
@@ -92,11 +99,24 @@ impl<'a> BlobObject<'a> {
         context: &'a Context,
         src: impl AsRef<Path>,
     ) -> std::result::Result<BlobObject<'a>, BlobError> {
-        let blob = BlobObject::create(context, src.as_ref().to_string_lossy(), b"")?;
-        fs::copy(src.as_ref(), blob.to_abs_path()).map_err(|err| {
-            fs::remove_file(blob.to_abs_path()).ok();
-            BlobError::new_copy_failure(blob.blobdir, &blob.name, src.as_ref(), err)
+        let mut src_file = fs::File::open(src.as_ref()).map_err(|err| {
+            BlobError::new_copy_failure(context.get_blobdir(), "", src.as_ref(), err)
         })?;
+        let (stem, ext) = BlobObject::sanitise_name(&src.as_ref().to_string_lossy());
+        let (name, mut dst_file) = BlobObject::create_new_file(context.get_blobdir(), &stem, &ext)?;
+        std::io::copy(&mut src_file, &mut dst_file).map_err(|err| {
+            {
+                // Attempt to remove the failed file, swallow errors resulting from that.
+                let path = context.get_blobdir().join(&name);
+                fs::remove_file(path).ok();
+            }
+            BlobError::new_copy_failure(context.get_blobdir(), &name, src.as_ref(), err)
+        })?;
+        let blob = BlobObject {
+            blobdir: context.get_blobdir(),
+            name: format!("$BLOBDIR/{}", name),
+        };
+        context.call_cb(Event::NewBlobFile(blob.as_name().to_string()));
         Ok(blob)
     }
 
@@ -239,7 +259,8 @@ impl<'a> BlobObject<'a> {
     /// ".txt")` while "bar" is returned as `("bar", "")`.
     ///
     /// The extension part will always be lowercased.
-    fn sanitise_name(mut name: String) -> (String, String) {
+    fn sanitise_name(name: &str) -> (String, String) {
+        let mut name = name.to_string();
         for part in name.rsplit('/') {
             if part.len() > 0 {
                 name = part.to_string();