Fail installation in case zip file is malformed.

Currently we only fail install if we can't open zip file. We need to
also fail if there were issues traversing the content.

Bug: 288609769
Fixes: 288609769
Test: atest libandroidfw_tests
Change-Id: I37c62b485ec145a12ed8e5f637dc7c7f6a68110e

3 files changed, 104 insertions, 44 deletions

diff --git a/core/jni/com_android_internal_content_NativeLibraryHelper.cpp b/core/jni/com_android_internal_content_NativeLibraryHelper.cpp
index 9017d58913b3..da308b26fe59 100644
--- a/core/jni/com_android_internal_content_NativeLibraryHelper.cpp
+++ b/core/jni/com_android_internal_content_NativeLibraryHelper.cpp
@@ -292,21 +292,31 @@ private:
     }
 
 public:
-    static NativeLibrariesIterator* create(ZipFileRO* zipFile, bool debuggable) {
-        void* cookie = nullptr;
+    static base::expected<std::unique_ptr<NativeLibrariesIterator>, int32_t> create(
+            ZipFileRO* zipFile, bool debuggable) {
         // Do not specify a suffix to find both .so files and gdbserver.
-        if (!zipFile->startIteration(&cookie, APK_LIB.data(), nullptr /* suffix */)) {
-            return nullptr;
+        auto result = zipFile->startIterationOrError(APK_LIB.data(), nullptr /* suffix */);
+        if (!result.ok()) {
+            return base::unexpected(result.error());
         }
 
-        return new NativeLibrariesIterator(zipFile, debuggable, cookie);
+        return std::unique_ptr<NativeLibrariesIterator>(
+                new NativeLibrariesIterator(zipFile, debuggable, result.value()));
     }
 
-    ZipEntryRO next() {
-        ZipEntryRO next = nullptr;
-        while ((next = mZipFile->nextEntry(mCookie)) != nullptr) {
+    base::expected<ZipEntryRO, int32_t> next() {
+        ZipEntryRO nextEntry;
+        while (true) {
+            auto next = mZipFile->nextEntryOrError(mCookie);
+            if (!next.ok()) {
+                return base::unexpected(next.error());
+            }
+            nextEntry = next.value();
+            if (nextEntry == nullptr) {
+                break;
+            }
             // Make sure this entry has a filename.
-            if (mZipFile->getEntryFileName(next, fileName, sizeof(fileName))) {
+            if (mZipFile->getEntryFileName(nextEntry, fileName, sizeof(fileName))) {
                 continue;
             }
 
@@ -317,7 +327,7 @@ public:
             }
         }
 
-        return next;
+        return nextEntry;
     }
 
     inline const char* currentEntry() const {
@@ -348,19 +358,28 @@ iterateOverNativeFiles(JNIEnv *env, jlong apkHandle, jstring javaCpuAbi,
         return INSTALL_FAILED_INVALID_APK;
     }
 
-    std::unique_ptr<NativeLibrariesIterator> it(
-            NativeLibrariesIterator::create(zipFile, debuggable));
-    if (it.get() == nullptr) {
+    auto result = NativeLibrariesIterator::create(zipFile, debuggable);
+    if (!result.ok()) {
         return INSTALL_FAILED_INVALID_APK;
     }
+    std::unique_ptr<NativeLibrariesIterator> it(std::move(result.value()));
 
     const ScopedUtfChars cpuAbi(env, javaCpuAbi);
     if (cpuAbi.c_str() == nullptr) {
         // This would've thrown, so this return code isn't observable by Java.
         return INSTALL_FAILED_INVALID_APK;
     }
-    ZipEntryRO entry = nullptr;
-    while ((entry = it->next()) != nullptr) {
+
+    while (true) {
+        auto next = it->next();
+        if (!next.ok()) {
+            return INSTALL_FAILED_INVALID_APK;
+        }
+        auto entry = next.value();
+        if (entry == nullptr) {
+            break;
+        }
+
         const char* fileName = it->currentEntry();
         const char* lastSlash = it->lastSlash();
 
@@ -388,11 +407,11 @@ static int findSupportedAbi(JNIEnv* env, jlong apkHandle, jobjectArray supported
         return INSTALL_FAILED_INVALID_APK;
     }
 
-    std::unique_ptr<NativeLibrariesIterator> it(
-            NativeLibrariesIterator::create(zipFile, debuggable));
-    if (it.get() == nullptr) {
+    auto result = NativeLibrariesIterator::create(zipFile, debuggable);
+    if (!result.ok()) {
         return INSTALL_FAILED_INVALID_APK;
     }
+    std::unique_ptr<NativeLibrariesIterator> it(std::move(result.value()));
 
     const int numAbis = env->GetArrayLength(supportedAbisArray);
 
@@ -402,9 +421,17 @@ static int findSupportedAbi(JNIEnv* env, jlong apkHandle, jobjectArray supported
         supportedAbis.emplace_back(env, (jstring)env->GetObjectArrayElement(supportedAbisArray, i));
     }
 
-    ZipEntryRO entry = nullptr;
     int status = NO_NATIVE_LIBRARIES;
-    while ((entry = it->next()) != nullptr) {
+    while (true) {
+        auto next = it->next();
+        if (!next.ok()) {
+            return INSTALL_FAILED_INVALID_APK;
+        }
+        auto entry = next.value();
+        if (entry == nullptr) {
+            break;
+        }
+
         // We're currently in the lib/ directory of the APK, so it does have some native
         // code. We should return INSTALL_FAILED_NO_MATCHING_ABIS if none of the
         // libraries match.
diff --git a/libs/androidfw/ZipFileRO.cpp b/libs/androidfw/ZipFileRO.cpp
index 52e7a70521a1..d7b5914130ee 100644
--- a/libs/androidfw/ZipFileRO.cpp
+++ b/libs/androidfw/ZipFileRO.cpp
@@ -40,17 +40,24 @@ class _ZipEntryRO {
 public:
     ZipEntry entry;
     std::string_view name;
-    void *cookie;
+    void *cookie = nullptr;
 
-    _ZipEntryRO() : cookie(NULL) {}
+    _ZipEntryRO() = default;
 
     ~_ZipEntryRO() {
-      EndIteration(cookie);
+        EndIteration(cookie);
+    }
+
+    android::ZipEntryRO convertToPtr() {
+        _ZipEntryRO* result = new _ZipEntryRO;
+        result->entry = std::move(this->entry);
+        result->name = std::move(this->name);
+        result->cookie = std::exchange(this->cookie, nullptr);
+        return result;
     }
 
 private:
-    _ZipEntryRO(const _ZipEntryRO& other);
-    _ZipEntryRO& operator=(const _ZipEntryRO& other);
+    DISALLOW_COPY_AND_ASSIGN(_ZipEntryRO);
 };
 
 ZipFileRO::~ZipFileRO() {
@@ -94,17 +101,15 @@ ZipFileRO::~ZipFileRO() {
 
 ZipEntryRO ZipFileRO::findEntryByName(const char* entryName) const
 {
-    _ZipEntryRO* data = new _ZipEntryRO;
-
-    data->name = entryName;
+    _ZipEntryRO data;
+    data.name = entryName;
 
-    const int32_t error = FindEntry(mHandle, entryName, &(data->entry));
+    const int32_t error = FindEntry(mHandle, entryName, &(data.entry));
     if (error) {
-        delete data;
-        return NULL;
+        return nullptr;
     }
 
-    return (ZipEntryRO) data;
+    return data.convertToPtr();
 }
 
 /*
@@ -143,35 +148,50 @@ bool ZipFileRO::getEntryInfo(ZipEntryRO entry, uint16_t* pMethod,
 }
 
 bool ZipFileRO::startIteration(void** cookie) {
-  return startIteration(cookie, NULL, NULL);
+  return startIteration(cookie, nullptr, nullptr);
 }
 
-bool ZipFileRO::startIteration(void** cookie, const char* prefix, const char* suffix)
-{
-    _ZipEntryRO* ze = new _ZipEntryRO;
-    int32_t error = StartIteration(mHandle, &(ze->cookie),
+bool ZipFileRO::startIteration(void** cookie, const char* prefix, const char* suffix) {
+    auto result = startIterationOrError(prefix, suffix);
+    if (!result.ok()) {
+        return false;
+    }
+    *cookie = result.value();
+    return true;
+}
+
+base::expected<void*, int32_t>
+ZipFileRO::startIterationOrError(const char* prefix, const char* suffix) {
+    _ZipEntryRO ze;
+    int32_t error = StartIteration(mHandle, &(ze.cookie),
                                    prefix ? prefix : "", suffix ? suffix : "");
     if (error) {
         ALOGW("Could not start iteration over %s: %s", mFileName != NULL ? mFileName : "<null>",
                 ErrorCodeString(error));
-        delete ze;
-        return false;
+        return base::unexpected(error);
     }
 
-    *cookie = ze;
-    return true;
+    return ze.convertToPtr();
 }
 
-ZipEntryRO ZipFileRO::nextEntry(void* cookie)
-{
+ZipEntryRO ZipFileRO::nextEntry(void* cookie) {
+    auto result = nextEntryOrError(cookie);
+    if (!result.ok()) {
+        return nullptr;
+    }
+    return result.value();
+}
+
+base::expected<ZipEntryRO, int32_t> ZipFileRO::nextEntryOrError(void* cookie) {
     _ZipEntryRO* ze = reinterpret_cast<_ZipEntryRO*>(cookie);
     int32_t error = Next(ze->cookie, &(ze->entry), &(ze->name));
     if (error) {
         if (error != -1) {
             ALOGW("Error iteration over %s: %s", mFileName != NULL ? mFileName : "<null>",
                     ErrorCodeString(error));
+            return base::unexpected(error);
         }
-        return NULL;
+        return nullptr;
     }
 
     return &(ze->entry);
diff --git a/libs/androidfw/include/androidfw/ZipFileRO.h b/libs/androidfw/include/androidfw/ZipFileRO.h
index 10f6d0655bf4..be1f98f4843d 100644
--- a/libs/androidfw/include/androidfw/ZipFileRO.h
+++ b/libs/androidfw/include/androidfw/ZipFileRO.h
@@ -37,6 +37,8 @@
 #include <unistd.h>
 #include <time.h>
 
+#include <android-base/expected.h>
+
 #include <util/map_ptr.h>
 
 #include <utils/Compat.h>
@@ -102,6 +104,11 @@ public:
      */
     bool startIteration(void** cookie);
     bool startIteration(void** cookie, const char* prefix, const char* suffix);
+    /*
+     * Same as above, but returns the error code in case of failure.
+     * #see libziparchive/zip_error.h.
+     */
+    base::expected<void*, int32_t> startIterationOrError(const char* prefix, const char* suffix);
 
     /**
      * Return the next entry in iteration order, or NULL if there are no more
@@ -109,6 +116,12 @@ public:
      */
     ZipEntryRO nextEntry(void* cookie);
 
+    /**
+     * Same as above, but returns the error code in case of failure.
+     * #see libziparchive/zip_error.h.
+     */
+    base::expected<ZipEntryRO, int32_t> nextEntryOrError(void* cookie);
+
     void endIteration(void* cookie);
 
     void releaseEntry(ZipEntryRO entry) const;