From b92c16e245dd4838e010afd2ebfe1f3d4087ef9c Mon Sep 17 00:00:00 2001 From: Alex Buynytskyy Date: Wed, 5 Jul 2023 21:48:59 -0700 Subject: [PATCH] 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 --- ...d_internal_content_NativeLibraryHelper.cpp | 67 ++++++++++++------ libs/androidfw/ZipFileRO.cpp | 68 ++++++++++++------- libs/androidfw/include/androidfw/ZipFileRO.h | 13 ++++ 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 9017d58913b3c..da308b26fe595 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, 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( + new NativeLibrariesIterator(zipFile, debuggable, result.value())); } - ZipEntryRO next() { - ZipEntryRO next = nullptr; - while ((next = mZipFile->nextEntry(mCookie)) != nullptr) { + base::expected 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 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 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 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 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 52e7a70521a1e..d7b5914130ee0 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; + _ZipEntryRO data; + data.name = entryName; - 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 +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 : "", 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 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 : "", 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 10f6d0655bf4a..be1f98f4843d7 100644 --- a/libs/androidfw/include/androidfw/ZipFileRO.h +++ b/libs/androidfw/include/androidfw/ZipFileRO.h @@ -37,6 +37,8 @@ #include #include +#include + #include #include @@ -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 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 nextEntryOrError(void* cookie); + void endIteration(void* cookie); void releaseEntry(ZipEntryRO entry) const;