From 801c44115ecdee5511160f1837b514dd274f6a32 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 30 Nov 2022 16:47:23 -0800 Subject: [res] Properly create ZipAssetsProvider with fd Bug: 237583012 Test: atest com.android.overlaytest Change-Id: If79b4297edfcefe72bf579b50931a40f73bdfd58 --- libs/androidfw/AssetsProvider.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'libs/androidfw/AssetsProvider.cpp') diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index 80e560747a3e..24460a4c0867 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -92,16 +92,19 @@ ZipAssetsProvider::ZipAssetsProvider(ZipArchiveHandle handle, PathOrDebugName&& last_mod_time_(last_mod_time) {} std::unique_ptr ZipAssetsProvider::Create(std::string path, - package_property_t flags) { + package_property_t flags, + base::unique_fd fd) { + const auto released_fd = fd.ok() ? fd.release() : -1; ZipArchiveHandle handle; - if (int32_t result = OpenArchive(path.c_str(), &handle); result != 0) { + if (int32_t result = released_fd < 0 ? OpenArchive(path.c_str(), &handle) + : OpenArchiveFd(released_fd, path.c_str(), &handle)) { LOG(ERROR) << "Failed to open APK '" << path << "': " << ::ErrorCodeString(result); CloseArchive(handle); return {}; } struct stat sb{.st_mtime = -1}; - if (stat(path.c_str(), &sb) < 0) { + if ((released_fd < 0 ? stat(path.c_str(), &sb) : fstat(released_fd, &sb)) < 0) { // Stat requires execute permissions on all directories path to the file. If the process does // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will // always have to return true. -- cgit v1.2.3-59-g8ed1b From 2ab4447ad20bdbf664c00de5f47c2c638ee83241 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 30 Nov 2022 00:56:22 -0800 Subject: [res] Don't stat asset providers on RO filesystems IsUpToDate() is one of the most often called functions, let's make sure it only performs a syscall if it makes any sense and the underlying file can really change. Bug: 237583012 Test: build + boot + UTs Change-Id: Ie5999ddadf10b56f35354d00ad3402b229ffa2c3 --- libs/androidfw/AssetsProvider.cpp | 47 +++++++++++++++++++++------------ libs/androidfw/include/androidfw/misc.h | 4 +++ libs/androidfw/misc.cpp | 44 +++++++++++++++++++++++++----- 3 files changed, 72 insertions(+), 23 deletions(-) (limited to 'libs/androidfw/AssetsProvider.cpp') diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index 24460a4c0867..b9264c5d0f2d 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -104,12 +104,15 @@ std::unique_ptr ZipAssetsProvider::Create(std::string path, } struct stat sb{.st_mtime = -1}; - if ((released_fd < 0 ? stat(path.c_str(), &sb) : fstat(released_fd, &sb)) < 0) { - // Stat requires execute permissions on all directories path to the file. If the process does - // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will - // always have to return true. - LOG(WARNING) << "Failed to stat file '" << path << "': " - << base::SystemErrorCodeToString(errno); + // Skip all up-to-date checks if the file won't ever change. + if (!isReadonlyFilesystem(path.c_str())) { + if ((released_fd < 0 ? stat(path.c_str(), &sb) : fstat(released_fd, &sb)) < 0) { + // Stat requires execute permissions on all directories path to the file. If the process does + // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will + // always have to return true. + LOG(WARNING) << "Failed to stat file '" << path << "': " + << base::SystemErrorCodeToString(errno); + } } return std::unique_ptr( @@ -136,12 +139,15 @@ std::unique_ptr ZipAssetsProvider::Create(base::unique_fd fd, } struct stat sb{.st_mtime = -1}; - if (fstat(released_fd, &sb) < 0) { - // Stat requires execute permissions on all directories path to the file. If the process does - // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will - // always have to return true. - LOG(WARNING) << "Failed to fstat file '" << friendly_name << "': " - << base::SystemErrorCodeToString(errno); + // Skip all up-to-date checks if the file won't ever change. + if (!isReadonlyFilesystem(released_fd)) { + if (fstat(released_fd, &sb) < 0) { + // Stat requires execute permissions on all directories path to the file. If the process does + // not have execute permissions on this file, allow the zip to be opened but IsUpToDate() will + // always have to return true. + LOG(WARNING) << "Failed to fstat file '" << friendly_name + << "': " << base::SystemErrorCodeToString(errno); + } } return std::unique_ptr( @@ -278,6 +284,9 @@ const std::string& ZipAssetsProvider::GetDebugName() const { } bool ZipAssetsProvider::IsUpToDate() const { + if (last_mod_time_ == -1) { + return true; + } struct stat sb{}; if (fstat(GetFileDescriptor(zip_handle_.get()), &sb) < 0) { // If fstat fails on the zip archive, return true so the zip archive the resource system does @@ -291,7 +300,7 @@ DirectoryAssetsProvider::DirectoryAssetsProvider(std::string&& path, time_t last : dir_(std::forward(path)), last_mod_time_(last_mod_time) {} std::unique_ptr DirectoryAssetsProvider::Create(std::string path) { - struct stat sb{}; + struct stat sb; const int result = stat(path.c_str(), &sb); if (result == -1) { LOG(ERROR) << "Failed to find directory '" << path << "'."; @@ -307,8 +316,9 @@ std::unique_ptr DirectoryAssetsProvider::Create(std::st path += OS_PATH_SEPARATOR; } - return std::unique_ptr(new DirectoryAssetsProvider(std::move(path), - sb.st_mtime)); + const bool isReadonly = isReadonlyFilesystem(path.c_str()); + return std::unique_ptr( + new DirectoryAssetsProvider(std::move(path), isReadonly ? -1 : sb.st_mtime)); } std::unique_ptr DirectoryAssetsProvider::OpenInternal(const std::string& path, @@ -338,7 +348,10 @@ const std::string& DirectoryAssetsProvider::GetDebugName() const { } bool DirectoryAssetsProvider::IsUpToDate() const { - struct stat sb{}; + if (last_mod_time_ == -1) { + return true; + } + struct stat sb; if (stat(dir_.c_str(), &sb) < 0) { // If stat fails on the zip archive, return true so the zip archive the resource system does // attempt to refresh the ApkAsset. @@ -434,4 +447,4 @@ bool EmptyAssetsProvider::IsUpToDate() const { return true; } -} // namespace android \ No newline at end of file +} // namespace android diff --git a/libs/androidfw/include/androidfw/misc.h b/libs/androidfw/include/androidfw/misc.h index 5a5a0e29125d..d40d24ede769 100644 --- a/libs/androidfw/include/androidfw/misc.h +++ b/libs/androidfw/include/androidfw/misc.h @@ -44,6 +44,10 @@ FileType getFileType(const char* fileName); /* get the file's modification date; returns -1 w/errno set on failure */ time_t getFileModDate(const char* fileName); +// Check if |path| or |fd| resides on a readonly filesystem. +bool isReadonlyFilesystem(const char* path); +bool isReadonlyFilesystem(int fd); + }; // namespace android #endif // _LIBS_ANDROID_FW_MISC_H diff --git a/libs/androidfw/misc.cpp b/libs/androidfw/misc.cpp index 52854205207c..7af506638ebc 100644 --- a/libs/androidfw/misc.cpp +++ b/libs/androidfw/misc.cpp @@ -21,12 +21,17 @@ // #include -#include +#include "android-base/logging.h" + +#ifndef _WIN32 +#include +#include +#endif // _WIN32 + #include -#include #include - -using namespace android; +#include +#include namespace android { @@ -41,8 +46,7 @@ FileType getFileType(const char* fileName) if (errno == ENOENT || errno == ENOTDIR) return kFileTypeNonexistent; else { - fprintf(stderr, "getFileType got errno=%d on '%s'\n", - errno, fileName); + PLOG(ERROR) << "getFileType(): stat(" << fileName << ") failed"; return kFileTypeUnknown; } } else { @@ -82,4 +86,32 @@ time_t getFileModDate(const char* fileName) return sb.st_mtime; } +#ifdef _WIN32 +// No need to implement these for Windows, the functions only matter on a device. +bool isReadonlyFilesystem(const char*) { + return false; +} +bool isReadonlyFilesystem(int) { + return false; +} +#else // _WIN32 +bool isReadonlyFilesystem(const char* path) { + struct statfs sfs; + if (::statfs(path, &sfs)) { + PLOG(ERROR) << "isReadonlyFilesystem(): statfs(" << path << ") failed"; + return false; + } + return (sfs.f_flags & ST_RDONLY) != 0; +} + +bool isReadonlyFilesystem(int fd) { + struct statfs sfs; + if (::fstatfs(fd, &sfs)) { + PLOG(ERROR) << "isReadonlyFilesystem(): fstatfs(" << fd << ") failed"; + return false; + } + return (sfs.f_flags & ST_RDONLY) != 0; +} +#endif // _WIN32 + }; // namespace android -- cgit v1.2.3-59-g8ed1b