commit a1ef2e020664860de562eccd63c2c5895519763c
author Kenny Root <kroot@google.com> Fri Oct 01 18:28:28 2010 -0700
committer Alex Ray <aray@google.com> Tue Jul 30 13:56:55 2013 -0700
tree 57005e4ff576850a57e2642a2f68a7fc9001acaa
parent 7d90df8dc30e7b22aea030f7dca01095529cc6b1

ZipFileRO: moar logging and wrap close

There is apparently still a race upon reading the entry Local File
Header that can't be tracked down, so move the LFH check inside the
mutex-protected block so we can call lseek again to see where we are
when we log an error.

Also, close() can fail so use TEMP_FAILURE_RETRY on it so we don't
unwittingly leak file descriptors when Mean Mr. EINTR comes a-knocking.

Change-Id: I753abad0bd882fe28f7281c406fa76f64393ef4c

include/utils/ZipFileRO.h

diff --git a/include/utils/ZipFileRO.h b/include/utils/ZipFileRO.h
index 9668bde..e1ff780 100644
--- a/include/utils/ZipFileRO.h
+++ b/include/utils/ZipFileRO.h
@@ -64,15 +64,8 @@
           mNumEntries(-1), mDirectoryOffset(-1),
           mHashTableSize(-1), mHashTable(NULL)
         {}
-    ~ZipFileRO() {
-        free(mHashTable);
-        if (mDirectoryMap)
-            mDirectoryMap->release();
-        if (mFd >= 0)
-            close(mFd);
-        if (mFileName)
-            free(mFileName);
-    }
+
+    ~ZipFileRO();
 
     /*
      * Open an archive.

libs/utils/ZipFileRO.cpp

diff --git a/libs/utils/ZipFileRO.cpp b/libs/utils/ZipFileRO.cpp
index 9fcae72..bee86b2 100644
--- a/libs/utils/ZipFileRO.cpp
+++ b/libs/utils/ZipFileRO.cpp
@@ -86,6 +86,16 @@
  */
 #define kZipEntryAdj        10000
 
+ZipFileRO::~ZipFileRO() {
+    free(mHashTable);
+    if (mDirectoryMap)
+        mDirectoryMap->release();
+    if (mFd >= 0)
+        TEMP_FAILURE_RETRY(close(mFd));
+    if (mFileName)
+        free(mFileName);
+}
+
 /*
  * Convert a ZipEntryRO to a hash table index, verifying that it's in a
  * valid range.
@@ -122,7 +132,7 @@
 
     mFileLength = lseek(fd, 0, SEEK_END);
     if (mFileLength < kEOCDLen) {
-        close(fd);
+        TEMP_FAILURE_RETRY(close(fd));
         return UNKNOWN_ERROR;
     }
 
@@ -152,7 +162,7 @@
 bail:
     free(mFileName);
     mFileName = NULL;
-    close(fd);
+    TEMP_FAILURE_RETRY(close(fd));
     return UNKNOWN_ERROR;
 }
 
@@ -512,12 +522,14 @@
                 LOGW("failed reading lfh from offset %ld\n", localHdrOffset);
                 return false;
             }
-        }
 
-        if (get4LE(lfhBuf) != kLFHSignature) {
-            LOGW("didn't find signature at start of lfh, offset=%ld (got 0x%08lx, expected 0x%08x)\n",
-                localHdrOffset, get4LE(lfhBuf), kLFHSignature);
-            return false;
+            if (get4LE(lfhBuf) != kLFHSignature) {
+                off_t actualOffset = lseek(mFd, 0, SEEK_CUR);
+                LOGW("didn't find signature at start of lfh; wanted: offset=%ld data=0x%08x; "
+                        "got: offset=%zd data=0x%08lx\n",
+                        localHdrOffset, kLFHSignature, (size_t)actualOffset, get4LE(lfhBuf));
+                return false;
+            }
         }
 
         off_t dataOffset = localHdrOffset + kLFHLen