Merge "Maintain compatibility by deleting target directory before renaming into it."

1 files changed, 20 insertions, 11 deletions

diff --git a/core/java/android/util/AtomicFile.java b/core/java/android/util/AtomicFile.java
index ec687b989689..e4015bb44501 100644
--- a/core/java/android/util/AtomicFile.java
+++ b/core/java/android/util/AtomicFile.java
@@ -143,10 +143,7 @@ public class AtomicFile {
         }
 
         if (mLegacyBackupName.exists()) {
-            if (!mLegacyBackupName.renameTo(mBaseName)) {
-                Log.e(LOG_TAG, "Failed to rename legacy backup file " + mLegacyBackupName
-                        + " to base file " + mBaseName);
-            }
+            rename(mLegacyBackupName, mBaseName);
         }
 
         try {
@@ -184,9 +181,7 @@ public class AtomicFile {
         } catch (IOException e) {
             Log.e(LOG_TAG, "Failed to close file output stream", e);
         }
-        if (!mNewName.renameTo(mBaseName)) {
-            Log.e(LOG_TAG, "Failed to rename new file " + mNewName + " to base file " + mBaseName);
-        }
+        rename(mNewName, mBaseName);
         if (mCommitEventLogger != null) {
             mCommitEventLogger.onFinishWrite();
         }
@@ -247,10 +242,7 @@ public class AtomicFile {
      */
     public FileInputStream openRead() throws FileNotFoundException {
         if (mLegacyBackupName.exists()) {
-            if (!mLegacyBackupName.renameTo(mBaseName)) {
-                Log.e(LOG_TAG, "Failed to rename legacy backup file " + mLegacyBackupName
-                        + " to base file " + mBaseName);
-            }
+            rename(mLegacyBackupName, mBaseName);
         }
 
         // Don't delete mNewName here - it was okay to call openRead() between startWrite() and
@@ -327,4 +319,21 @@ public class AtomicFile {
             IoUtils.closeQuietly(out);
         }
     }
+
+    private static void rename(File source, File target) {
+        // We used to delete the target file before rename, but that isn't atomic, and the rename()
+        // syscall should atomically replace the target file. However in the case where the target
+        // file is a directory, a simple rename() won't work. We need to delete the file in this
+        // case because there are callers who erroneously called mBaseName.mkdirs() (instead of
+        // mBaseName.getParentFile().mkdirs()) before creating the AtomicFile, and it worked
+        // regardless, so this deletion became some kind of API.
+        if (target.isDirectory()) {
+            if (!target.delete()) {
+                Log.e(LOG_TAG, "Failed to delete file which is a directory " + target);
+            }
+        }
+        if (!source.renameTo(target)) {
+            Log.e(LOG_TAG, "Failed to rename " + source + " to " + target);
+        }
+    }
 }