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

Commit c357f719 authored by Yurii Zubrytskyi's avatar Yurii Zubrytskyi
Browse files

Use reference counted pointers for ApkAssets 

The primary reason for memory corruption is freed ApkAssets
Java expected them to only be freed in the finalizers, but
there are explicit close() calls now, destroying objects that
are still in use in some AssetManager2 objects

This CL makes sure those AssetManagers don't assume ApkAssets
always exist, but instead tries to lock them in memory for any
access

It also adds logging in case of deleting an assets object with
any weak pointers still existing. Those will get into the
bugreports attached to related bugs to help with investigation.

Benchmarks don't regress, and the device appears to be working.
Given that the crashes used to be pretty rare, let's wait for
any new reports or lack of those.

+ add a missing .clang-format file to the jni directory
+ enabled C++23 in the project that uses AssetManager headers

Bug: 197260547
Bug: 276922628
Test: unit tests + boot + benchmarks
Change-Id: I495fd9e012fe370a1f725dbb0265b4ee1be8d805
parent 77224899

cmds/idmap2/idmap2/Lookup.cpp

+14 −17
Original line number Diff line number Diff line
@@ -174,7 +174,7 @@ Result<Unit> Lookup(const std::vector<std::string>& args) { 
    return Error("failed to parse config");

  }



  std::vector<std::unique_ptr<const ApkAssets>> apk_assets;

  std::vector<AssetManager2::ApkAssetsPtr> apk_assets;

  std::string target_path;

  std::string target_package_name;

  for (size_t i = 0; i < idmap_paths.size(); i++) {
@@ -217,13 +217,9 @@ Result<Unit> Lookup(const std::vector<std::string>& args) { 
    apk_assets.push_back(std::move(overlay_apk));

  }



  // AssetManager2::SetApkAssets requires raw ApkAssets pointers, not unique_ptrs

  std::vector<const ApkAssets*> raw_pointer_apk_assets;

  std::transform(apk_assets.cbegin(), apk_assets.cend(), std::back_inserter(raw_pointer_apk_assets),

                 [](const auto& p) -> const ApkAssets* { return p.get(); });

  AssetManager2 am;

  am.SetApkAssets(raw_pointer_apk_assets);

  am.SetConfiguration(config);

  {

    // Make sure |apk_assets| vector outlives the asset manager as it doesn't own the assets.

    AssetManager2 am(apk_assets, config);



    const Result<ResourceId> resid = ParseResReference(am, resid_str, target_package_name);

    if (!resid) {
@@ -235,6 +231,7 @@ Result<Unit> Lookup(const std::vector<std::string>& args) { 
      return Error(value.GetError(), "resource 0x%08x not found", *resid);

    }

    std::cout << *value << std::endl;

  }



  return Unit{};

}

cmds/idmap2/libidmap2/ResourceContainer.cpp

+2 −2
Original line number Diff line number Diff line
@@ -262,7 +262,7 @@ OverlayData CreateResourceMappingLegacy(const AssetManager2* overlay_am, 
}



struct ResState {

  std::unique_ptr<ApkAssets> apk_assets;

  AssetManager2::ApkAssetsPtr apk_assets;

  const LoadedArsc* arsc;

  const LoadedPackage* package;

  std::unique_ptr<AssetManager2> am;
@@ -284,7 +284,7 @@ struct ResState { 
    }



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

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

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

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

    }

cmds/idmap2/tests/ResourceUtilsTests.cpp

+2 −2
Original line number Diff line number Diff line
@@ -38,7 +38,7 @@ class ResourceUtilsTests : public Idmap2Tests { 
    apk_assets_ = ApkAssets::Load(GetTargetApkPath());

    ASSERT_THAT(apk_assets_, NotNull());



    am_.SetApkAssets({apk_assets_.get()});

    am_.SetApkAssets({apk_assets_});

  }



  const AssetManager2& GetAssetManager() {
@@ -47,7 +47,7 @@ class ResourceUtilsTests : public Idmap2Tests { 


 private:

  AssetManager2 am_;

  std::unique_ptr<const ApkAssets> apk_assets_;

  AssetManager2::ApkAssetsPtr apk_assets_;

};



TEST_F(ResourceUtilsTests, ResToTypeEntryName) {

core/jni/android_content_res_ApkAssets.cpp

+37 −11
Original line number Diff line number Diff line
@@ -74,16 +74,36 @@ enum : format_type_t { 
  FORMAT_DIRECTORY = 3,

};



Guarded<std::unique_ptr<const ApkAssets>>& ApkAssetsFromLong(jlong ptr) {

    return *reinterpret_cast<Guarded<std::unique_ptr<const ApkAssets>>*>(ptr);

Guarded<AssetManager2::ApkAssetsPtr>& ApkAssetsFromLong(jlong ptr) {

  return *reinterpret_cast<Guarded<AssetManager2::ApkAssetsPtr>*>(ptr);

}



static jlong CreateGuardedApkAssets(std::unique_ptr<const ApkAssets> assets) {

    auto guarded_assets = new Guarded<std::unique_ptr<const ApkAssets>>(std::move(assets));

static jlong CreateGuardedApkAssets(AssetManager2::ApkAssetsPtr assets) {

  auto guarded_assets = new Guarded<AssetManager2::ApkAssetsPtr>(std::move(assets));

  return reinterpret_cast<jlong>(guarded_assets);

}



static void DeleteGuardedApkAssets(Guarded<std::unique_ptr<const ApkAssets>>& apk_assets) {

static void DeleteGuardedApkAssets(Guarded<AssetManager2::ApkAssetsPtr>& apk_assets) {

  apk_assets.safeDelete([&apk_assets](AssetManager2::ApkAssetsPtr* assets) {

    if (!assets) {

      ALOGE("ApkAssets: Double delete of native assets object %p, ignored", &apk_assets);

    } else if (!*assets) {

      ALOGE("ApkAssets: Empty native assets pointer in native assets object %p", &apk_assets);

    } else {

      // |RefBase| increments |StrongCount| for each |sp<>| instance, and |WeakCount| for

      // both |sp<>| and |wp<>| instances. This means the actual |wp<>| instance count

      // is |WeakCount - StrongCount|.

      const auto useCount = (*assets)->getStrongCount();

      const auto weakCount = (*assets)->getWeakRefs()->getWeakCount() - useCount;

      if (useCount > 1) {

        ALOGE("ApkAssets: Deleting an object '%s' with %d > 1 strong and %d weak references",

              (*assets)->GetDebugName().c_str(), int(useCount), int(weakCount));

      } else if (weakCount > 0) {

        ALOGE("ApkAssets: Deleting an ApkAssets object '%s' with %d weak references",

              (*assets)->GetDebugName().c_str(), int(weakCount));

      }

    }

  });

  delete &apk_assets;

}
@@ -209,7 +229,7 @@ static jlong NativeLoad(JNIEnv* env, jclass /*clazz*/, const format_type_t forma 
  ATRACE_NAME(base::StringPrintf("LoadApkAssets(%s)", path.c_str()).c_str());



  auto loader_assets = LoaderAssetsProvider::Create(env, assets_provider);

  std::unique_ptr<ApkAssets> apk_assets;

  AssetManager2::ApkAssetsPtr apk_assets;

  switch (format) {

    case FORMAT_APK: {

        auto assets = MultiAssetsProvider::Create(std::move(loader_assets),
@@ -269,7 +289,7 @@ static jlong NativeLoadFromFd(JNIEnv* env, jclass /*clazz*/, const format_type_t 
  }



  auto loader_assets = LoaderAssetsProvider::Create(env, assets_provider);

  std::unique_ptr<const ApkAssets> apk_assets;

  AssetManager2::ApkAssetsPtr apk_assets;

  switch (format) {

    case FORMAT_APK: {

        auto assets =
@@ -336,7 +356,7 @@ static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_ 
  }



  auto loader_assets = LoaderAssetsProvider::Create(env, assets_provider);

  std::unique_ptr<const ApkAssets> apk_assets;

  AssetManager2::ApkAssetsPtr apk_assets;

  switch (format) {

    case FORMAT_APK: {

        auto assets =
@@ -374,6 +394,12 @@ static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_ 


static jlong NativeLoadEmpty(JNIEnv* env, jclass /*clazz*/, jint flags, jobject assets_provider) {

  auto apk_assets = ApkAssets::Load(LoaderAssetsProvider::Create(env, assets_provider), flags);

  if (apk_assets == nullptr) {

    const std::string error_msg =

        base::StringPrintf("Failed to load empty assets with provider %p", (void*)assets_provider);

    jniThrowException(env, "java/io/IOException", error_msg.c_str());

    return 0;

  }

  return CreateGuardedApkAssets(std::move(apk_assets));

}

core/jni/android_content_res_ApkAssets.h

+2 −2
Original line number Diff line number Diff line
@@ -18,13 +18,13 @@ 
#define ANDROID_CONTENT_RES_APKASSETS_H



#include "androidfw/ApkAssets.h"

#include "androidfw/AssetManager2.h"

#include "androidfw/MutexGuard.h"



#include "jni.h"



namespace android {



Guarded<std::unique_ptr<const ApkAssets>>& ApkAssetsFromLong(jlong ptr);

Guarded<AssetManager2::ApkAssetsPtr>& ApkAssetsFromLong(jlong ptr);



} // namespace android