Fix redundant file backups

We'd observed a bug in which an unchanged file was nevertheless
being redundantly transmitted for backup on every backup pass.
The underlying issue turns out to have been the FileBackupHelper
base implementation's logic for diffing the prior-state file
set against the current state, in the case when there had been
deletions of prior files.  In addition, there was also a
parallel bug in which file checksums were not calculated
properly in some cases, leading to at least one additional
redundant backup of the file in question.

Bug 18694053

Change-Id: Ie0dec06486b5fef4624561737019569c85d6b2a0

1 files changed, 41 insertions, 38 deletions

diff --git a/libs/androidfw/BackupHelpers.cpp b/libs/androidfw/BackupHelpers.cpp
index 52dce9f..33cf8ef 100644
--- a/libs/androidfw/BackupHelpers.cpp
+++ b/libs/androidfw/BackupHelpers.cpp
@@ -225,8 +225,6 @@ write_update_file(BackupDataWriter* dataStream, int fd, int mode, const String8&
     file_metadata_v1 metadata;
 
     char* buf = (char*)malloc(bufsize);
-    int crc = crc32(0L, Z_NULL, 0);
-
 
     fileSize = lseek(fd, 0, SEEK_END);
     lseek(fd, 0, SEEK_SET);
@@ -310,8 +308,12 @@ write_update_file(BackupDataWriter* dataStream, const String8& key, char const*
 }
 
 static int
-compute_crc32(int fd)
-{
+compute_crc32(const char* file, FileRec* out) {
+    int fd = open(file, O_RDONLY);
+    if (fd < 0) {
+        return -1;
+    }
+
     const int bufsize = 4*1024;
     int amt;
 
@@ -324,8 +326,11 @@ compute_crc32(int fd)
         crc = crc32(crc, (Bytef*)buf, amt);
     }
 
+    close(fd);
     free(buf);
-    return crc;
+
+    out->s.crc32 = crc;
+    return NO_ERROR;
 }
 
 int
@@ -353,7 +358,8 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD
 
         err = stat(file, &st);
         if (err != 0) {
-            r.deleted = true;
+            // not found => treat as deleted
+            continue;
         } else {
             r.deleted = false;
             r.s.modTime_sec = st.st_mtime;
@@ -361,12 +367,17 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD
             //r.s.modTime_nsec = st.st_mtime_nsec;
             r.s.mode = st.st_mode;
             r.s.size = st.st_size;
-            // we compute the crc32 later down below, when we already have the file open.
 
             if (newSnapshot.indexOfKey(key) >= 0) {
                 LOGP("back_up_files key already in use '%s'", key.string());
                 return -1;
             }
+
+            // compute the CRC
+            if (compute_crc32(file, &r) != NO_ERROR) {
+                ALOGW("Unable to open file %s", file);
+                continue;
+            }
         }
         newSnapshot.add(key, r);
     }
@@ -374,49 +385,41 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD
     int n = 0;
     int N = oldSnapshot.size();
     int m = 0;
+    int M = newSnapshot.size();
 
-    while (n<N && m<fileCount) {
+    while (n<N && m<M) {
         const String8& p = oldSnapshot.keyAt(n);
         const String8& q = newSnapshot.keyAt(m);
         FileRec& g = newSnapshot.editValueAt(m);
         int cmp = p.compare(q);
-        if (g.deleted || cmp < 0) {
-            // file removed
+        if (cmp < 0) {
+            // file present in oldSnapshot, but not present in newSnapshot
             LOGP("file removed: %s", p.string());
-            g.deleted = true; // They didn't mention the file, but we noticed that it's gone.
-            dataStream->WriteEntityHeader(p, -1);
+            write_delete_file(dataStream, p);
             n++;
-        }
-        else if (cmp > 0) {
+        } else if (cmp > 0) {
             // file added
-            LOGP("file added: %s", g.file.string());
+            LOGP("file added: %s crc=0x%08x", g.file.string(), g.s.crc32);
             write_update_file(dataStream, q, g.file.string());
             m++;
-        }
-        else {
-            // both files exist, check them
+        } else {
+            // same file exists in both old and new; check whether to update
             const FileState& f = oldSnapshot.valueAt(n);
 
-            int fd = open(g.file.string(), O_RDONLY);
-            if (fd < 0) {
-                // We can't open the file.  Don't report it as a delete either.  Let the
-                // server keep the old version.  Maybe they'll be able to deal with it
-                // on restore.
-                LOGP("Unable to open file %s - skipping", g.file.string());
-            } else {
-                g.s.crc32 = compute_crc32(fd);
-
-                LOGP("%s", q.string());
-                LOGP("  new: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x",
-                        f.modTime_sec, f.modTime_nsec, f.mode, f.size, f.crc32);
-                LOGP("  old: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x",
-                        g.s.modTime_sec, g.s.modTime_nsec, g.s.mode, g.s.size, g.s.crc32);
-                if (f.modTime_sec != g.s.modTime_sec || f.modTime_nsec != g.s.modTime_nsec
-                        || f.mode != g.s.mode || f.size != g.s.size || f.crc32 != g.s.crc32) {
+            LOGP("%s", q.string());
+            LOGP("  old: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x",
+                    f.modTime_sec, f.modTime_nsec, f.mode, f.size, f.crc32);
+            LOGP("  new: modTime=%d,%d mode=%04o size=%-3d crc32=0x%08x",
+                    g.s.modTime_sec, g.s.modTime_nsec, g.s.mode, g.s.size, g.s.crc32);
+            if (f.modTime_sec != g.s.modTime_sec || f.modTime_nsec != g.s.modTime_nsec
+                    || f.mode != g.s.mode || f.size != g.s.size || f.crc32 != g.s.crc32) {
+                int fd = open(g.file.string(), O_RDONLY);
+                if (fd < 0) {
+                    ALOGE("Unable to read file for backup: %s", g.file.string());
+                } else {
                     write_update_file(dataStream, fd, g.s.mode, p, g.file.string());
+                    close(fd);
                 }
-
-                close(fd);
             }
             n++;
             m++;
@@ -425,12 +428,12 @@ back_up_files(int oldSnapshotFD, BackupDataWriter* dataStream, int newSnapshotFD
 
     // these were deleted
     while (n<N) {
-        dataStream->WriteEntityHeader(oldSnapshot.keyAt(n), -1);
+        write_delete_file(dataStream, oldSnapshot.keyAt(n));
         n++;
     }
 
     // these were added
-    while (m<fileCount) {
+    while (m<M) {
         const String8& q = newSnapshot.keyAt(m);
         FileRec& g = newSnapshot.editValueAt(m);
         write_update_file(dataStream, q, g.file.string());