From ef538436300f6424f47368b448c5e9303a52a1b2 Mon Sep 17 00:00:00 2001 From: Ryan Mitchell Date: Mon, 1 Mar 2021 14:52:14 -0800 Subject: [PATCH] Don't use ApkAssets::GetPath for equality checks With the introduction of ResourcesProviders, not all ApkAssets have paths on disk. Theme::SetTo and various AssetManager methods use the path to perform equality checking on ApkAssets. This equality check will be performed on the debug string of an ApkAssets if it does not have a path on disk. This causes ApkAssets with the same debug name (like "") to be seen as the same ApkAssets. Rather than using path, the pointer to the ApkAssets should be used for equality checking since ResourcesManager caches and reuses ApkAssets when multiple AssetManagers request the same assets. Bug: 177101983 Test: atest CtsResourcesLoaderTests Change-Id: I11f6a2a3a7cc8febe3f976236792f78e41cf07e6 --- core/java/android/content/res/ApkAssets.java | 13 ++- core/jni/android_content_res_ApkAssets.cpp | 17 +++- libs/androidfw/ApkAssets.cpp | 6 +- libs/androidfw/AssetManager2.cpp | 88 +++++++++---------- libs/androidfw/AssetsProvider.cpp | 29 ++++-- libs/androidfw/include/androidfw/ApkAssets.h | 12 ++- .../include/androidfw/AssetManager2.h | 2 +- .../include/androidfw/AssetsProvider.h | 10 ++- .../NavigationModeController.java | 2 +- 9 files changed, 111 insertions(+), 68 deletions(-) diff --git a/core/java/android/content/res/ApkAssets.java b/core/java/android/content/res/ApkAssets.java index 2d381eb85e90a..de48ed75746d6 100644 --- a/core/java/android/content/res/ApkAssets.java +++ b/core/java/android/content/res/ApkAssets.java @@ -22,6 +22,7 @@ import android.compat.annotation.UnsupportedAppUsage; import android.content.om.OverlayableInfo; import android.content.res.loader.AssetsProvider; import android.content.res.loader.ResourcesProvider; +import android.text.TextUtils; import com.android.internal.annotations.GuardedBy; @@ -344,7 +345,14 @@ public final class ApkAssets { @UnsupportedAppUsage public @NonNull String getAssetPath() { synchronized (this) { - return nativeGetAssetPath(mNativePtr); + return TextUtils.emptyIfNull(nativeGetAssetPath(mNativePtr)); + } + } + + /** @hide */ + public @NonNull String getDebugName() { + synchronized (this) { + return nativeGetDebugName(mNativePtr); } } @@ -422,7 +430,7 @@ public final class ApkAssets { @Override public String toString() { - return "ApkAssets{path=" + getAssetPath() + "}"; + return "ApkAssets{path=" + getDebugName() + "}"; } /** @@ -450,6 +458,7 @@ public final class ApkAssets { @NonNull FileDescriptor fd, @NonNull String friendlyName, long offset, long length, @PropertyFlags int flags, @Nullable AssetsProvider asset) throws IOException; private static native @NonNull String nativeGetAssetPath(long ptr); + private static native @NonNull String nativeGetDebugName(long ptr); private static native long nativeGetStringBlock(long ptr); private static native boolean nativeIsUpToDate(long ptr); private static native long nativeOpenXml(long ptr, @NonNull String fileName) throws IOException; diff --git a/core/jni/android_content_res_ApkAssets.cpp b/core/jni/android_content_res_ApkAssets.cpp index c7439f1b32d43..b0c575162b566 100644 --- a/core/jni/android_content_res_ApkAssets.cpp +++ b/core/jni/android_content_res_ApkAssets.cpp @@ -83,6 +83,10 @@ class LoaderAssetsProvider : public AssetsProvider { return true; } + std::optional GetPath() const override { + return {}; + } + const std::string& GetDebugName() const override { return debug_name_; } @@ -358,8 +362,16 @@ static jlong NativeGetFinalizer(JNIEnv* /*env*/, jclass /*clazz*/) { } static jstring NativeGetAssetPath(JNIEnv* env, jclass /*clazz*/, jlong ptr) { - const ApkAssets* apk_assets = reinterpret_cast(ptr); - return env->NewStringUTF(apk_assets->GetPath().c_str()); + auto apk_assets = reinterpret_cast(ptr); + if (auto path = apk_assets->GetPath()) { + return env->NewStringUTF(path->data()); + } + return nullptr; +} + +static jstring NativeGetDebugName(JNIEnv* env, jclass /*clazz*/, jlong ptr) { + auto apk_assets = reinterpret_cast(ptr); + return env->NewStringUTF(apk_assets->GetDebugName().c_str()); } static jlong NativeGetStringBlock(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) { @@ -467,6 +479,7 @@ static const JNINativeMethod gApkAssetsMethods[] = { (void*)NativeLoadFromFdOffset}, {"nativeGetFinalizer", "()J", (void*)NativeGetFinalizer}, {"nativeGetAssetPath", "(J)Ljava/lang/String;", (void*)NativeGetAssetPath}, + {"nativeGetDebugName", "(J)Ljava/lang/String;", (void*)NativeGetDebugName}, {"nativeGetStringBlock", "(J)J", (void*)NativeGetStringBlock}, {"nativeIsUpToDate", "(J)Z", (void*)NativeIsUpToDate}, {"nativeOpenXml", "(JLjava/lang/String;)J", (void*)NativeOpenXml}, diff --git a/libs/androidfw/ApkAssets.cpp b/libs/androidfw/ApkAssets.cpp index 9c743cea592a6..76366fce2aeaf 100755 --- a/libs/androidfw/ApkAssets.cpp +++ b/libs/androidfw/ApkAssets.cpp @@ -156,7 +156,11 @@ std::unique_ptr ApkAssets::LoadImpl(std::unique_ptr resources_ std::move(loaded_idmap))); } -const std::string& ApkAssets::GetPath() const { +std::optional ApkAssets::GetPath() const { + return assets_provider_->GetPath(); +} + +const std::string& ApkAssets::GetDebugName() const { return assets_provider_->GetDebugName(); } diff --git a/libs/androidfw/AssetManager2.cpp b/libs/androidfw/AssetManager2.cpp index 36bde5ccf267b..c0ef7be8b673d 100644 --- a/libs/androidfw/AssetManager2.cpp +++ b/libs/androidfw/AssetManager2.cpp @@ -116,8 +116,10 @@ void AssetManager2::BuildDynamicRefTable() { package_groups_.clear(); package_ids_.fill(0xff); - // A mapping from apk assets path to the runtime package id of its first loaded package. - std::unordered_map apk_assets_package_ids; + // A mapping from path of apk assets that could be target packages of overlays to the runtime + // package id of its first loaded package. Overlays currently can only override resources in the + // first package in the target resource table. + std::unordered_map target_assets_package_ids; // Overlay resources are not directly referenced by an application so their resource ids // can change throughout the application's lifetime. Assign overlay package ids last. @@ -140,8 +142,8 @@ void AssetManager2::BuildDynamicRefTable() { if (auto loaded_idmap = apk_assets->GetLoadedIdmap(); loaded_idmap != nullptr) { // The target package must precede the overlay package in the apk assets paths in order // to take effect. - auto iter = apk_assets_package_ids.find(std::string(loaded_idmap->TargetApkPath())); - if (iter == apk_assets_package_ids.end()) { + auto iter = target_assets_package_ids.find(std::string(loaded_idmap->TargetApkPath())); + if (iter == target_assets_package_ids.end()) { LOG(INFO) << "failed to find target package for overlay " << loaded_idmap->OverlayApkPath(); } else { @@ -205,7 +207,10 @@ void AssetManager2::BuildDynamicRefTable() { package_name, static_cast(entry.package_id)); } - apk_assets_package_ids.insert(std::make_pair(apk_assets->GetPath(), package_id)); + if (auto apk_assets_path = apk_assets->GetPath()) { + // Overlay target ApkAssets must have been created using path based load apis. + target_assets_package_ids.insert(std::make_pair(std::string(*apk_assets_path), package_id)); + } } } @@ -227,7 +232,7 @@ void AssetManager2::DumpToLog() const { std::string list; for (const auto& apk_assets : apk_assets_) { - base::StringAppendF(&list, "%s,", apk_assets->GetPath().c_str()); + base::StringAppendF(&list, "%s,", apk_assets->GetDebugName().c_str()); } LOG(INFO) << "ApkAssets: " << list; @@ -383,8 +388,8 @@ void AssetManager2::SetConfiguration(const ResTable_config& configuration) { } } -std::set AssetManager2::GetNonSystemOverlayPaths() const { - std::set non_system_overlays; +std::set AssetManager2::GetNonSystemOverlays() const { + std::set non_system_overlays; for (const PackageGroup& package_group : package_groups_) { bool found_system_package = false; for (const ConfiguredPackage& package : package_group.packages_) { @@ -396,7 +401,7 @@ std::set AssetManager2::GetNonSystemOverlayPaths() const { if (!found_system_package) { for (const ConfiguredOverlay& overlay : package_group.overlays_) { - non_system_overlays.insert(apk_assets_[overlay.cookie]->GetPath()); + non_system_overlays.insert(apk_assets_[overlay.cookie]); } } } @@ -408,7 +413,7 @@ base::expected, IOError> AssetManager2::GetResourceCon bool exclude_system, bool exclude_mipmap) const { ATRACE_NAME("AssetManager::GetResourceConfigurations"); const auto non_system_overlays = - (exclude_system) ? GetNonSystemOverlayPaths() : std::set(); + (exclude_system) ? GetNonSystemOverlays() : std::set(); std::set configurations; for (const PackageGroup& package_group : package_groups_) { @@ -419,8 +424,8 @@ base::expected, IOError> AssetManager2::GetResourceCon } auto apk_assets = apk_assets_[package_group.cookies_[i]]; - if (exclude_system && apk_assets->IsOverlay() - && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) { + if (exclude_system && apk_assets->IsOverlay() && + non_system_overlays.find(apk_assets) == non_system_overlays.end()) { // Exclude overlays that target system resources. continue; } @@ -439,7 +444,7 @@ std::set AssetManager2::GetResourceLocales(bool exclude_system, ATRACE_NAME("AssetManager::GetResourceLocales"); std::set locales; const auto non_system_overlays = - (exclude_system) ? GetNonSystemOverlayPaths() : std::set(); + (exclude_system) ? GetNonSystemOverlays() : std::set(); for (const PackageGroup& package_group : package_groups_) { for (size_t i = 0; i < package_group.packages_.size(); i++) { @@ -449,8 +454,8 @@ std::set AssetManager2::GetResourceLocales(bool exclude_system, } auto apk_assets = apk_assets_[package_group.cookies_[i]]; - if (exclude_system && apk_assets->IsOverlay() - && non_system_overlays.find(apk_assets->GetPath()) == non_system_overlays.end()) { + if (exclude_system && apk_assets->IsOverlay() && + non_system_overlays.find(apk_assets) == non_system_overlays.end()) { // Exclude overlays that target system resources. continue; } @@ -491,7 +496,7 @@ std::unique_ptr AssetManager2::OpenDir(const std::string& dirname) con AssetDir::FileInfo info; info.setFileName(String8(name.data(), name.size())); info.setFileType(type); - info.setSourceName(String8(apk_assets->GetPath().c_str())); + info.setSourceName(String8(apk_assets->GetDebugName().c_str())); files->add(info); }; @@ -846,7 +851,7 @@ std::string AssetManager2::GetLastResourceResolution() const { } log_stream << "\n\t" << prefix->second << ": " << *step.package_name << " (" - << apk_assets_[step.cookie]->GetPath() << ")"; + << apk_assets_[step.cookie]->GetDebugName() << ")"; if (!step.config_name.isEmpty()) { log_stream << " -" << step.config_name; } @@ -1556,41 +1561,32 @@ base::expected Theme::SetTo(const Theme& o) { std::map src_asset_cookie_id_map; // Determine which ApkAssets are loaded in both theme AssetManagers. - std::vector src_assets = o.asset_manager_->GetApkAssets(); + const auto src_assets = o.asset_manager_->GetApkAssets(); for (size_t i = 0; i < src_assets.size(); i++) { const ApkAssets* src_asset = src_assets[i]; - std::vector dest_assets = asset_manager_->GetApkAssets(); + const auto dest_assets = asset_manager_->GetApkAssets(); for (size_t j = 0; j < dest_assets.size(); j++) { const ApkAssets* dest_asset = dest_assets[j]; - - // Map the runtime package of the source apk asset to the destination apk asset. - if (src_asset->GetPath() == dest_asset->GetPath()) { - const auto& src_packages = src_asset->GetLoadedArsc()->GetPackages(); - const auto& dest_packages = dest_asset->GetLoadedArsc()->GetPackages(); - - SourceToDestinationRuntimePackageMap package_map; - - // The source and destination package should have the same number of packages loaded in - // the same order. - const size_t N = src_packages.size(); - CHECK(N == dest_packages.size()) - << " LoadedArsc " << src_asset->GetPath() << " differs number of packages."; - for (size_t p = 0; p < N; p++) { - auto& src_package = src_packages[p]; - auto& dest_package = dest_packages[p]; - CHECK(src_package->GetPackageName() == dest_package->GetPackageName()) - << " Package " << src_package->GetPackageName() << " differs in load order."; - - int src_package_id = o.asset_manager_->GetAssignedPackageId(src_package.get()); - int dest_package_id = asset_manager_->GetAssignedPackageId(dest_package.get()); - package_map[src_package_id] = dest_package_id; - } - - src_to_dest_asset_cookies.insert(std::make_pair(i, j)); - src_asset_cookie_id_map.insert(std::make_pair(i, package_map)); - break; + if (src_asset != dest_asset) { + // ResourcesManager caches and reuses ApkAssets when the same apk must be present in + // multiple AssetManagers. Two ApkAssets point to the same version of the same resources + // if they are the same instance. + continue; } + + // Map the package ids of the asset in the source AssetManager to the package ids of the + // asset in th destination AssetManager. + SourceToDestinationRuntimePackageMap package_map; + for (const auto& loaded_package : src_asset->GetLoadedArsc()->GetPackages()) { + const int src_package_id = o.asset_manager_->GetAssignedPackageId(loaded_package.get()); + const int dest_package_id = asset_manager_->GetAssignedPackageId(loaded_package.get()); + package_map[src_package_id] = dest_package_id; + } + + src_to_dest_asset_cookies.insert(std::make_pair(i, j)); + src_asset_cookie_id_map.insert(std::make_pair(i, package_map)); + break; } } diff --git a/libs/androidfw/AssetsProvider.cpp b/libs/androidfw/AssetsProvider.cpp index f3c48f7f9fc84..0aaf0b359b604 100644 --- a/libs/androidfw/AssetsProvider.cpp +++ b/libs/androidfw/AssetsProvider.cpp @@ -261,6 +261,13 @@ std::optional ZipAssetsProvider::GetCrc(std::string_view path) const { return entry.crc32; } +std::optional ZipAssetsProvider::GetPath() const { + if (name_.GetPath() != nullptr) { + return *name_.GetPath(); + } + return {}; +} + const std::string& ZipAssetsProvider::GetDebugName() const { return name_.GetDebugName(); } @@ -318,6 +325,10 @@ bool DirectoryAssetsProvider::ForEachFile( return true; } +std::optional DirectoryAssetsProvider::GetPath() const { + return dir_; +} + const std::string& DirectoryAssetsProvider::GetDebugName() const { return dir_; } @@ -336,13 +347,9 @@ MultiAssetsProvider::MultiAssetsProvider(std::unique_ptr&& prima std::unique_ptr&& secondary) : primary_(std::forward>(primary)), secondary_(std::forward>(secondary)) { - if (primary_->GetDebugName() == kEmptyDebugString) { - debug_name_ = secondary_->GetDebugName(); - } else if (secondary_->GetDebugName() == kEmptyDebugString) { - debug_name_ = primary_->GetDebugName(); - } else { - debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName(); - } + debug_name_ = primary_->GetDebugName() + " and " + secondary_->GetDebugName(); + path_ = (primary_->GetDebugName() != kEmptyDebugString) ? primary_->GetPath() + : secondary_->GetPath(); } std::unique_ptr MultiAssetsProvider::Create( @@ -367,6 +374,10 @@ bool MultiAssetsProvider::ForEachFile(const std::string& root_path, return primary_->ForEachFile(root_path, f) && secondary_->ForEachFile(root_path, f); } +std::optional MultiAssetsProvider::GetPath() const { + return path_; +} + const std::string& MultiAssetsProvider::GetDebugName() const { return debug_name_; } @@ -394,6 +405,10 @@ bool EmptyAssetsProvider::ForEachFile( return true; } +std::optional EmptyAssetsProvider::GetPath() const { + return {}; +} + const std::string& EmptyAssetsProvider::GetDebugName() const { const static std::string kEmpty = kEmptyDebugString; return kEmpty; diff --git a/libs/androidfw/include/androidfw/ApkAssets.h b/libs/androidfw/include/androidfw/ApkAssets.h index d0019ed6be303..6f88f41976cdb 100644 --- a/libs/androidfw/include/androidfw/ApkAssets.h +++ b/libs/androidfw/include/androidfw/ApkAssets.h @@ -34,7 +34,6 @@ namespace android { // Holds an APK. class ApkAssets { public: - // Creates an ApkAssets from a path on device. static std::unique_ptr Load(const std::string& path, package_property_t flags = 0U); @@ -61,12 +60,11 @@ class ApkAssets { static std::unique_ptr LoadOverlay(const std::string& idmap_path, package_property_t flags = 0U); - // TODO(177101983): Remove all uses of GetPath for checking whether two ApkAssets are the same. - // With the introduction of ResourcesProviders, not all ApkAssets have paths. This could cause - // bugs when path is used for comparison because multiple ApkAssets could have the same "firendly - // name". Use pointer equality instead. ResourceManager caches and reuses ApkAssets so the - // same asset should have the same pointer. - const std::string& GetPath() const; + // Path to the contents of the ApkAssets on disk. The path could represent an APk, a directory, + // or some other file type. + std::optional GetPath() const; + + const std::string& GetDebugName() const; const AssetsProvider* GetAssetsProvider() const { return assets_provider_.get(); diff --git a/libs/androidfw/include/androidfw/AssetManager2.h b/libs/androidfw/include/androidfw/AssetManager2.h index 2255973f10391..119f531b8634a 100644 --- a/libs/androidfw/include/androidfw/AssetManager2.h +++ b/libs/androidfw/include/androidfw/AssetManager2.h @@ -412,7 +412,7 @@ class AssetManager2 { void RebuildFilterList(); // Retrieves the APK paths of overlays that overlay non-system packages. - std::set GetNonSystemOverlayPaths() const; + std::set GetNonSystemOverlays() const; // AssetManager2::GetBag(resid) wraps this function to track which resource ids have already // been seen while traversing bag parents. diff --git a/libs/androidfw/include/androidfw/AssetsProvider.h b/libs/androidfw/include/androidfw/AssetsProvider.h index 6f16ff453905f..63bbdcc9698da 100644 --- a/libs/androidfw/include/androidfw/AssetsProvider.h +++ b/libs/androidfw/include/androidfw/AssetsProvider.h @@ -48,6 +48,10 @@ struct AssetsProvider { virtual bool ForEachFile(const std::string& path, const std::function& f) const = 0; + // Retrieves the path to the contents of the AssetsProvider on disk. The path could represent an + // APk, a directory, or some other file type. + WARN_UNUSED virtual std::optional GetPath() const = 0; + // Retrieves a name that represents the interface. This may or may not be the path of the // interface source. WARN_UNUSED virtual const std::string& GetDebugName() const = 0; @@ -85,9 +89,9 @@ struct ZipAssetsProvider : public AssetsProvider { bool ForEachFile(const std::string& root_path, const std::function& f) const override; + WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; WARN_UNUSED bool IsUpToDate() const override; - WARN_UNUSED std::optional GetCrc(std::string_view path) const; ~ZipAssetsProvider() override = default; @@ -125,6 +129,7 @@ struct DirectoryAssetsProvider : public AssetsProvider { bool ForEachFile(const std::string& path, const std::function& f) const override; + WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; WARN_UNUSED bool IsUpToDate() const override; @@ -149,6 +154,7 @@ struct MultiAssetsProvider : public AssetsProvider { bool ForEachFile(const std::string& root_path, const std::function& f) const override; + WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; WARN_UNUSED bool IsUpToDate() const override; @@ -163,6 +169,7 @@ struct MultiAssetsProvider : public AssetsProvider { std::unique_ptr primary_; std::unique_ptr secondary_; + std::optional path_; std::string debug_name_; }; @@ -173,6 +180,7 @@ struct EmptyAssetsProvider : public AssetsProvider { bool ForEachFile(const std::string& path, const std::function& f) const override; + WARN_UNUSED std::optional GetPath() const override; WARN_UNUSED const std::string& GetDebugName() const override; WARN_UNUSED bool IsUpToDate() const override; diff --git a/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java b/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java index 65d4e23f7734c..4b9a4310be5f3 100644 --- a/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java +++ b/packages/SystemUI/src/com/android/systemui/navigationbar/NavigationModeController.java @@ -200,7 +200,7 @@ public class NavigationModeController implements Dumpable { Log.d(TAG, " assetPaths="); ApkAssets[] assets = context.getResources().getAssets().getApkAssets(); for (ApkAssets a : assets) { - Log.d(TAG, " " + a.getAssetPath()); + Log.d(TAG, " " + a.getDebugName()); } } }