From 93ff40f24d4054bbbaa5375ff2eae5f38e284d73 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 27 Nov 2024 14:13:35 -0800 Subject: [res] Make TargetResourcesContainer valid after an error ApkResourcesContainer class has a variant that is either a zip file object, or a full blown AssetManager with ApkAssets with the same zip file inside. But if transition from one to the other fails, it could end up in a state with neither, causing a crash if the users try accessing it. This change ensures that: 1. The zip file object is only moved into ApkAssets if the loading succeeds 2. If any part of the further initialization of ResState fails, we take the zip file out and put it back into the variant + Add unit tests for both ApkAssets and libidmap2 to ensure they don't crash in any of those scenarios (they used to) + Enable root tests in libidmap2 as they were never running Flag: EXEMPT bugfix Bug: 381108280 Test: atest libandroidfw_tests idmap2_tests + boot Change-Id: I8f4803fdf03a41ba7a6892e6aed07f04b77788ce --- libs/androidfw/include/androidfw/ApkAssets.h | 58 ++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 11 deletions(-) (limited to 'libs/androidfw/include') diff --git a/libs/androidfw/include/androidfw/ApkAssets.h b/libs/androidfw/include/androidfw/ApkAssets.h index 1fa67528c78b..231808beb718 100644 --- a/libs/androidfw/include/androidfw/ApkAssets.h +++ b/libs/androidfw/include/androidfw/ApkAssets.h @@ -47,13 +47,37 @@ class ApkAssets : public RefBase { package_property_t flags = 0U, off64_t offset = 0, off64_t len = AssetsProvider::kUnknownLength); + // // Creates an ApkAssets from an AssetProvider. - // The ApkAssets will take care of destroying the AssetsProvider when it is destroyed. - static ApkAssetsPtr Load(std::unique_ptr assets, package_property_t flags = 0U); + // The ApkAssets will take care of destroying the AssetsProvider when it is destroyed; + // the original argument is not moved from if loading fails. + // + // Note: this function takes care of the case when you pass a move(unique_ptr) + // that would create a temporary unique_ptr by moving your pointer into + // it before the function call, making it impossible to not move from the parameter + // on loading failure. The two overloads take care of moving the pointer back if needed. + // + + template + static ApkAssetsPtr Load(std::unique_ptr&& assets, package_property_t flags = 0U) + requires(std::is_same_v) { + return LoadImpl(std::move(assets), flags); + } + + template + static ApkAssetsPtr Load(std::unique_ptr&& assets, package_property_t flags = 0U) + requires(!std::is_same_v && std::is_base_of_v) { + std::unique_ptr base_assets(std::move(assets)); + auto res = LoadImpl(std::move(base_assets), flags); + if (!res) { + assets.reset(static_cast(base_assets.release())); + } + return res; + } // Creates an ApkAssets from the given asset file representing a resources.arsc. - static ApkAssetsPtr LoadTable(std::unique_ptr resources_asset, - std::unique_ptr assets, + static ApkAssetsPtr LoadTable(std::unique_ptr&& resources_asset, + std::unique_ptr&& assets, package_property_t flags = 0U); // Creates an ApkAssets from an IDMAP, which contains the original APK path, and the overlay @@ -94,17 +118,29 @@ class ApkAssets : public RefBase { bool IsUpToDate() const; + // DANGER! + // This is a destructive method that rips the assets provider out of ApkAssets object. + // It is only useful when one knows this assets object can't be used anymore, and they + // need the underlying assets provider back (e.g. when initialization fails for some + // reason). + std::unique_ptr TakeAssetsProvider() && { + return std::move(assets_provider_); + } + private: - static ApkAssetsPtr LoadImpl(std::unique_ptr assets, + static ApkAssetsPtr LoadImpl(std::unique_ptr&& assets, package_property_t property_flags, - std::unique_ptr idmap_asset, - std::unique_ptr loaded_idmap); + std::unique_ptr&& idmap_asset, + std::unique_ptr&& loaded_idmap); - static ApkAssetsPtr LoadImpl(std::unique_ptr resources_asset, - std::unique_ptr assets, + static ApkAssetsPtr LoadImpl(std::unique_ptr&& resources_asset, + std::unique_ptr&& assets, package_property_t property_flags, - std::unique_ptr idmap_asset, - std::unique_ptr loaded_idmap); + std::unique_ptr&& idmap_asset, + std::unique_ptr&& loaded_idmap); + + static ApkAssetsPtr LoadImpl(std::unique_ptr&& assets, + package_property_t flags = 0U); // Allows us to make it possible to call make_shared from inside the class but still keeps the // ctor 'private' for all means and purposes. -- cgit v1.2.3-59-g8ed1b