Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 93ff40f2 authored by Yurii Zubrytskyi's avatar Yurii Zubrytskyi
Browse files

[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
parent ed6a189e

cmds/idmap2/Android.bp

+1 −0
Original line number Original line Diff line number Diff line
@@ -165,6 +165,7 @@ cc_test { 
    ],

    ],

    host_supported: true,

    host_supported: true,

    test_suites: ["general-tests"],

    test_suites: ["general-tests"],

    require_root: true,

    srcs: [

    srcs: [

        "tests/BinaryStreamVisitorTests.cpp",

        "tests/BinaryStreamVisitorTests.cpp",

        "tests/CommandLineOptionsTests.cpp",

        "tests/CommandLineOptionsTests.cpp",

cmds/idmap2/libidmap2/ResourceContainer.cpp

+19 −5
Original line number Original line Diff line number Diff line
@@ -22,6 +22,7 @@ 
#include <utility>

#include <utility>

#include <vector>

#include <vector>





#include "android-base/scopeguard.h"

#include "androidfw/ApkAssets.h"

#include "androidfw/ApkAssets.h"

#include "androidfw/AssetManager.h"

#include "androidfw/AssetManager.h"

#include "androidfw/Util.h"

#include "androidfw/Util.h"
@@ -269,27 +270,40 @@ struct ResState { 
  std::unique_ptr<AssetManager2> am;

  std::unique_ptr<AssetManager2> am;

  ZipAssetsProvider* zip_assets;

  ZipAssetsProvider* zip_assets;





  static Result<ResState> Initialize(std::unique_ptr<ZipAssetsProvider> zip,

  static Result<ResState> Initialize(std::unique_ptr<ZipAssetsProvider>&& zip,

                                     package_property_t flags) {

                                     package_property_t flags) {

    ResState state;

    ResState state;

    state.zip_assets = zip.get();

    state.zip_assets = zip.get();

    if ((state.apk_assets = ApkAssets::Load(std::move(zip), flags)) == nullptr) {

    if ((state.apk_assets = ApkAssets::Load(std::move(zip), flags)) == nullptr) {

      return Error("failed to load apk asset");

      return Error("failed to load apk asset for '%s'",

                   state.zip_assets->GetDebugName().c_str());

    }

    }





    // Make sure we put ZipAssetsProvider where we took it if initialization fails, so the

    // original object stays valid for any next call it may get.

    auto scoped_restore_zip_assets = android::base::ScopeGuard([&zip, &state]() {

      zip = std::unique_ptr<ZipAssetsProvider>(

          static_cast<ZipAssetsProvider*>(

              std::move(const_cast<ApkAssets&>(*state.apk_assets)).TakeAssetsProvider().release()));

    });



    if ((state.arsc = state.apk_assets->GetLoadedArsc()) == nullptr) {

    if ((state.arsc = state.apk_assets->GetLoadedArsc()) == nullptr) {

      return Error("failed to retrieve loaded arsc");

      return Error("failed to retrieve loaded arsc for '%s'",

                   state.zip_assets->GetDebugName().c_str());

    }

    }





    if ((state.package = GetPackageAtIndex0(state.arsc)) == nullptr) {

    if ((state.package = GetPackageAtIndex0(state.arsc)) == nullptr) {

      return Error("failed to retrieve loaded package at index 0");

      return Error("failed to retrieve loaded package at index 0 for '%s'",

                   state.zip_assets->GetDebugName().c_str());

    }

    }





    state.am = std::make_unique<AssetManager2>();

    state.am = std::make_unique<AssetManager2>();

    if (!state.am->SetApkAssets({state.apk_assets}, false)) {

    if (!state.am->SetApkAssets({state.apk_assets}, false)) {

      return Error("failed to create asset manager");

      return Error("failed to create asset manager for '%s'",

                   state.zip_assets->GetDebugName().c_str());

    }

    }





    scoped_restore_zip_assets.Disable();

    return state;

    return state;

  }

  }

};

};

cmds/idmap2/tests/IdmapTests.cpp

+14 −0
Original line number Original line Diff line number Diff line
@@ -214,6 +214,20 @@ TEST(IdmapTests, CreateIdmapHeaderFromApkAssets) { 
  ASSERT_EQ(idmap->GetHeader()->GetOverlayName(), TestConstants::OVERLAY_NAME_ALL_POLICIES);

  ASSERT_EQ(idmap->GetHeader()->GetOverlayName(), TestConstants::OVERLAY_NAME_ALL_POLICIES);

}

}





TEST(IdmapTests, TargetContainerWorksAfterError) {

  auto target = TargetResourceContainer::FromPath(GetTestDataPath() + "/target/target-bad.apk");

  ASSERT_TRUE(target);



  auto crc = target->get()->GetCrc();

  ASSERT_TRUE(crc);



  // This call tries to construct the full ApkAssets state, and fails.

  ASSERT_FALSE(target->get()->DefinesOverlayable());

  auto crc2 = target->get()->GetCrc();

  ASSERT_TRUE(crc2);

  EXPECT_EQ(*crc, *crc2);

}



TEST(IdmapTests, CreateIdmapDataFromApkAssets) {

TEST(IdmapTests, CreateIdmapDataFromApkAssets) {

  std::string target_apk_path = GetTestDataPath() + "/target/target.apk";

  std::string target_apk_path = GetTestDataPath() + "/target/target.apk";

  std::string overlay_apk_path = GetTestDataPath() + "/overlay/overlay.apk";

  std::string overlay_apk_path = GetTestDataPath() + "/overlay/overlay.apk";

cmds/idmap2/tests/data/target/target-bad.apk

0 → 100644
+821 B

File added.

No diff preview for this file type.

View file

libs/androidfw/ApkAssets.cpp

+13 −12
Original line number Original line Diff line number Diff line
@@ -40,20 +40,21 @@ ApkAssets::ApkAssets(PrivateConstructorUtil, std::unique_ptr<Asset> resources_as 
}

}





ApkAssetsPtr ApkAssets::Load(const std::string& path, package_property_t flags) {

ApkAssetsPtr ApkAssets::Load(const std::string& path, package_property_t flags) {

  return Load(ZipAssetsProvider::Create(path, flags), flags);

  return LoadImpl(ZipAssetsProvider::Create(path, flags), flags);

}

}





ApkAssetsPtr ApkAssets::LoadFromFd(base::unique_fd fd, const std::string& debug_name,

ApkAssetsPtr ApkAssets::LoadFromFd(base::unique_fd fd, const std::string& debug_name,

                                   package_property_t flags, off64_t offset, off64_t len) {

                                   package_property_t flags, off64_t offset, off64_t len) {

  return Load(ZipAssetsProvider::Create(std::move(fd), debug_name, offset, len), flags);

  return LoadImpl(ZipAssetsProvider::Create(std::move(fd), debug_name, offset, len), flags);

}

}





ApkAssetsPtr ApkAssets::Load(std::unique_ptr<AssetsProvider> assets, package_property_t flags) {

ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<AssetsProvider>&& assets,

                                 package_property_t flags) {

  return LoadImpl(std::move(assets), flags, nullptr /* idmap_asset */, nullptr /* loaded_idmap */);

  return LoadImpl(std::move(assets), flags, nullptr /* idmap_asset */, nullptr /* loaded_idmap */);

}

}





ApkAssetsPtr ApkAssets::LoadTable(std::unique_ptr<Asset> resources_asset,

ApkAssetsPtr ApkAssets::LoadTable(std::unique_ptr<Asset>&& resources_asset,

                                  std::unique_ptr<AssetsProvider> assets,

                                  std::unique_ptr<AssetsProvider>&& assets,

                                  package_property_t flags) {

                                  package_property_t flags) {

  if (resources_asset == nullptr) {

  if (resources_asset == nullptr) {

    return {};

    return {};
@@ -97,10 +98,10 @@ ApkAssetsPtr ApkAssets::LoadOverlay(const std::string& idmap_path, package_prope 
                  std::move(loaded_idmap));

                  std::move(loaded_idmap));

}

}





ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<AssetsProvider> assets,

ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<AssetsProvider>&& assets,

                                 package_property_t property_flags,

                                 package_property_t property_flags,

                                 std::unique_ptr<Asset> idmap_asset,

                                 std::unique_ptr<Asset>&& idmap_asset,

                                 std::unique_ptr<LoadedIdmap> loaded_idmap) {

                                 std::unique_ptr<LoadedIdmap>&& loaded_idmap) {

  if (assets == nullptr) {

  if (assets == nullptr) {

    return {};

    return {};

  }

  }
@@ -119,11 +120,11 @@ ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<AssetsProvider> assets, 
                  std::move(idmap_asset), std::move(loaded_idmap));

                  std::move(idmap_asset), std::move(loaded_idmap));

}

}





ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<Asset> resources_asset,

ApkAssetsPtr ApkAssets::LoadImpl(std::unique_ptr<Asset>&& resources_asset,

                                 std::unique_ptr<AssetsProvider> assets,

                                 std::unique_ptr<AssetsProvider>&& assets,

                                 package_property_t property_flags,

                                 package_property_t property_flags,

                                 std::unique_ptr<Asset> idmap_asset,

                                 std::unique_ptr<Asset>&& idmap_asset,

                                 std::unique_ptr<LoadedIdmap> loaded_idmap) {

                                 std::unique_ptr<LoadedIdmap>&& loaded_idmap) {

  if (assets == nullptr ) {

  if (assets == nullptr ) {

    return {};

    return {};

  }

  }