Synchronize native ApkAssets data

If an ApkAssets is created and destroyed before being put into an
AssetManager, the native layer may not have been synchronized in the
thread that does Java object finalization. So when the Java object gets
deleted on the finalization thread, it may see old/unallocated native
data and not delete the native object correctly.

Similar to how we already lock on the native AssetManager whenever
entering the JNI layer, we should also do the same for ApkAssets.

Bug: 186528775
Test: boot and no crashes
Change-Id: I5fcb9e788a9e3cb8baeb92fa6efc882d5bbbf0b0

core/jni/android_content_res_ApkAssets.cpp

@@ -16,6 +16,8 @@
 
 #define ATRACE_TAG ATRACE_TAG_RESOURCES
 
+#include <mutex>
+
 #include "signal.h"
 
 #include "android-base/logging.h"
@@ -26,6 +28,7 @@
 #include "utils/misc.h"
 #include "utils/Trace.h"
 
+#include "android_content_res_ApkAssets.h"
 #include "android_util_AssetManager_private.h"
 #include "core_jni_helpers.h"
 #include "jni.h"
@@ -72,6 +75,19 @@ 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);
+}
+
+static jlong CreateGuardedApkAssets(std::unique_ptr<const ApkAssets> assets) {
+    auto guarded_assets = new Guarded<std::unique_ptr<const ApkAssets>>(std::move(assets));
+    return reinterpret_cast<jlong>(guarded_assets);
+}
+
+static void DeleteGuardedApkAssets(Guarded<std::unique_ptr<const ApkAssets>>& apk_assets) {
+    delete &apk_assets;
+}
+
 class LoaderAssetsProvider : public AssetsProvider {
  public:
   static std::unique_ptr<AssetsProvider> Create(JNIEnv* env, jobject assets_provider) {
@@ -227,7 +243,7 @@ static jlong NativeLoad(JNIEnv* env, jclass /*clazz*/, const format_type_t forma
     jniThrowException(env, "java/io/IOException", error_msg.c_str());
     return 0;
   }
-  return reinterpret_cast<jlong>(apk_assets.release());
+  return CreateGuardedApkAssets(std::move(apk_assets));
 }
 
 static jlong NativeLoadFromFd(JNIEnv* env, jclass /*clazz*/, const format_type_t format,
@@ -279,7 +295,7 @@ static jlong NativeLoadFromFd(JNIEnv* env, jclass /*clazz*/, const format_type_t
     jniThrowException(env, "java/io/IOException", error_msg.c_str());
     return 0;
   }
-  return reinterpret_cast<jlong>(apk_assets.release());
+  return CreateGuardedApkAssets(std::move(apk_assets));
 }
 
 static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_type_t format,
@@ -347,12 +363,12 @@ static jlong NativeLoadFromFdOffset(JNIEnv* env, jclass /*clazz*/, const format_
     jniThrowException(env, "java/io/IOException", error_msg.c_str());
     return 0;
   }
-  return reinterpret_cast<jlong>(apk_assets.release());
+  return CreateGuardedApkAssets(std::move(apk_assets));
 }
 
 static jlong NativeLoadEmpty(JNIEnv* env, jclass /*clazz*/, jint flags, jobject assets_provider) {
   auto apk_assets = ApkAssets::Load(LoaderAssetsProvider::Create(env, assets_provider), flags);
-  return reinterpret_cast<jlong>(apk_assets.release());
+  return CreateGuardedApkAssets(std::move(apk_assets));
 }
 
 // STOPSHIP (b/159041693): Revert signal handler when reason for issue is found.
@@ -383,54 +399,60 @@ static void DestroyErrorHandler(int sig, siginfo_t* info, void* ucontext) {
 }
 
 static void NativeDestroy(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) {
-  auto apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
-  destroy_info << "{ptr=" << apk_assets;
-  if (apk_assets != nullptr) {
-    destroy_info << ", name='" << apk_assets->GetDebugName() << "'"
-                 << ", idmap=" << apk_assets->GetLoadedIdmap()
-                 << ", {arsc=" << apk_assets->GetLoadedArsc();
-    if (auto arsc = apk_assets->GetLoadedArsc()) {
-      destroy_info << ", strings=" << arsc->GetStringPool()
-                   << ", packages=" << &arsc->GetPackages()
-                   << " [";
-      for (auto& package : arsc->GetPackages()) {
-        destroy_info << "{unique_ptr=" << &package
-                     << ", package=" << package.get() << "},";
-      }
-      destroy_info << "]";
+    {
+        auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+        auto apk_assets = scoped_apk_assets->get();
+        destroy_info << "{ptr=" << apk_assets;
+        if (apk_assets != nullptr) {
+            destroy_info << ", name='" << apk_assets->GetDebugName() << "'"
+                         << ", idmap=" << apk_assets->GetLoadedIdmap()
+                         << ", {arsc=" << apk_assets->GetLoadedArsc();
+            if (auto arsc = apk_assets->GetLoadedArsc()) {
+                destroy_info << ", strings=" << arsc->GetStringPool()
+                             << ", packages=" << &arsc->GetPackages() << " [";
+                for (auto& package : arsc->GetPackages()) {
+                    destroy_info << "{unique_ptr=" << &package << ", package=" << package.get()
+                                 << "},";
+                }
+                destroy_info << "]";
+            }
+            destroy_info << "}";
+        }
+        destroy_info << "}";
     }
-    destroy_info << "}";
-  }
-  destroy_info << "}";
 
-  delete apk_assets;
+    DeleteGuardedApkAssets(ApkAssetsFromLong(ptr));
 
-  // Deleting the apk assets did not lead to a crash.
-  destroy_info.str("");
-  destroy_info.clear();
+    // Deleting the apk assets did not lead to a crash.
+    destroy_info.str("");
+    destroy_info.clear();
 }
 
 static jstring NativeGetAssetPath(JNIEnv* env, jclass /*clazz*/, jlong ptr) {
-  auto apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
-  if (auto path = apk_assets->GetPath()) {
-    return env->NewStringUTF(path->data());
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
+    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<const ApkAssets*>(ptr);
-  return env->NewStringUTF(apk_assets->GetDebugName().c_str());
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
+    return env->NewStringUTF(apk_assets->GetDebugName().c_str());
 }
 
 static jlong NativeGetStringBlock(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) {
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
-  return reinterpret_cast<jlong>(apk_assets->GetLoadedArsc()->GetStringPool());
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
+    return reinterpret_cast<jlong>(apk_assets->GetLoadedArsc()->GetStringPool());
 }
 
 static jboolean NativeIsUpToDate(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) {
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
-  return apk_assets->IsUpToDate() ? JNI_TRUE : JNI_FALSE;
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
+    return apk_assets->IsUpToDate() ? JNI_TRUE : JNI_FALSE;
 }
 
 static jlong NativeOpenXml(JNIEnv* env, jclass /*clazz*/, jlong ptr, jstring file_name) {
@@ -439,7 +461,8 @@ static jlong NativeOpenXml(JNIEnv* env, jclass /*clazz*/, jlong ptr, jstring fil
     return 0;
   }
 
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+  auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+  auto apk_assets = scoped_apk_assets->get();
   std::unique_ptr<Asset> asset = apk_assets->GetAssetsProvider()->Open(
       path_utf8.c_str(),Asset::AccessMode::ACCESS_RANDOM);
   if (asset == nullptr) {
@@ -467,12 +490,13 @@ static jlong NativeOpenXml(JNIEnv* env, jclass /*clazz*/, jlong ptr, jstring fil
 
 static jobject NativeGetOverlayableInfo(JNIEnv* env, jclass /*clazz*/, jlong ptr,
                                          jstring overlayable_name) {
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
 
-  const auto& packages = apk_assets->GetLoadedArsc()->GetPackages();
-  if (packages.empty()) {
-    jniThrowException(env, "java/io/IOException", "Error reading overlayable from APK");
-    return 0;
+    const auto& packages = apk_assets->GetLoadedArsc()->GetPackages();
+    if (packages.empty()) {
+        jniThrowException(env, "java/io/IOException", "Error reading overlayable from APK");
+        return 0;
   }
 
   // TODO(b/119899133): Convert this to a search for the info rather than assuming it's at index 0
@@ -502,13 +526,14 @@ static jobject NativeGetOverlayableInfo(JNIEnv* env, jclass /*clazz*/, jlong ptr
 }
 
 static jboolean NativeDefinesOverlayable(JNIEnv* env, jclass /*clazz*/, jlong ptr) {
-  const ApkAssets* apk_assets = reinterpret_cast<const ApkAssets*>(ptr);
+    auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr));
+    auto apk_assets = scoped_apk_assets->get();
 
-  const auto& packages = apk_assets->GetLoadedArsc()->GetPackages();
-  if (packages.empty()) {
-    // Must throw to prevent bypass by returning false
-    jniThrowException(env, "java/io/IOException", "Error reading overlayable from APK");
-    return 0;
+    const auto& packages = apk_assets->GetLoadedArsc()->GetPackages();
+    if (packages.empty()) {
+        // Must throw to prevent bypass by returning false
+        jniThrowException(env, "java/io/IOException", "Error reading overlayable from APK");
+        return 0;
   }
 
   const auto& overlayable_infos = packages[0]->GetOverlayableMap();

core/jni/android_content_res_ApkAssets.h

@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ANDROID_CONTENT_RES_APKASSETS_H
+#define ANDROID_CONTENT_RES_APKASSETS_H
+
+#include "androidfw/ApkAssets.h"
+#include "androidfw/MutexGuard.h"
+
+#include "jni.h"
+
+namespace android {
+
+Guarded<std::unique_ptr<const ApkAssets>>& ApkAssetsFromLong(jlong ptr);
+
+} // namespace android
+
+#endif // ANDROID_CONTENT_RES_APKASSETS_H

core/jni/android_util_AssetManager.cpp

@@ -42,6 +42,7 @@
 #include "androidfw/ResourceTypes.h"
 #include "androidfw/ResourceUtils.h"
 
+#include "android_content_res_ApkAssets.h"
 #include "android_util_AssetManager_private.h"
 #include "core_jni_helpers.h"
 #include "jni.h"
@@ -50,9 +51,9 @@
 #include "nativehelper/ScopedStringChars.h"
 #include "nativehelper/ScopedUtfChars.h"
 #include "utils/Log.h"
-#include "utils/misc.h"
 #include "utils/String8.h"
 #include "utils/Trace.h"
+#include "utils/misc.h"
 
 extern "C" int capget(cap_user_header_t hdrp, cap_user_data_t datap);
 extern "C" int capset(cap_user_header_t hdrp, const cap_user_data_t datap);
@@ -307,7 +308,9 @@ static void NativeSetApkAssets(JNIEnv* env, jclass /*clazz*/, jlong ptr,
     if (env->ExceptionCheck()) {
       return;
     }
-    apk_assets.push_back(reinterpret_cast<const ApkAssets*>(apk_assets_native_ptr));
+
+    auto scoped_assets = ScopedLock(ApkAssetsFromLong(apk_assets_native_ptr));
+    apk_assets.push_back(scoped_assets->get());
   }
 
   ScopedLock<AssetManager2> assetmanager(AssetManagerFromLong(ptr));

libs/androidfw/LoadedArsc.cpp

@@ -70,9 +70,6 @@ struct TypeSpecBuilder {
 
 }  // namespace
 
-LoadedPackage::LoadedPackage() = default;
-LoadedPackage::~LoadedPackage() = default;
-
 // Precondition: The header passed in has already been verified, so reading any fields and trusting
 // the ResChunk_header is safe.
 static bool VerifyResTableType(incfs::map_ptr<ResTable_type> header) {

libs/androidfw/include/androidfw/AssetManager2.h

@@ -94,6 +94,7 @@ class AssetManager2 {
   };
 
   AssetManager2();
+  explicit AssetManager2(AssetManager2&& other) = default;
 
   // Sets/resets the underlying ApkAssets for this AssetManager. The ApkAssets
   // are not owned by the AssetManager, and must have a longer lifetime.

libs/androidfw/include/androidfw/LoadedArsc.h

@@ -153,8 +153,6 @@ class LoadedPackage {
   static std::unique_ptr<const LoadedPackage> Load(const Chunk& chunk,
                                                    package_property_t property_flags);
 
-  ~LoadedPackage();
-
   // Finds the entry with the specified type name and entry name. The names are in UTF-16 because
   // the underlying ResStringPool API expects this. For now this is acceptable, but since
   // the default policy in AAPT2 is to build UTF-8 string pools, this needs to change.
@@ -275,7 +273,7 @@ class LoadedPackage {
  private:
   DISALLOW_COPY_AND_ASSIGN(LoadedPackage);
 
-  LoadedPackage();
+  LoadedPackage() = default;
 
   ResStringPool type_string_pool_;
   ResStringPool key_string_pool_;

libs/androidfw/include/androidfw/MutexGuard.h

@@ -18,6 +18,7 @@
 #define ANDROIDFW_MUTEXGUARD_H
 
 #include <mutex>
+#include <optional>
 #include <type_traits>
 
 #include "android-base/macros.h"
@@ -47,34 +48,32 @@ class Guarded {
   static_assert(!std::is_pointer<T>::value, "T must not be a raw pointer");
 
  public:
-  explicit Guarded() : guarded_() {
+  Guarded() : guarded_(std::in_place, T()) {
   }
 
-  template <typename U = T>
-  explicit Guarded(const T& guarded,
-                   typename std::enable_if<std::is_copy_constructible<U>::value>::type = void())
-      : guarded_(guarded) {
+  explicit Guarded(const T& guarded) : guarded_(std::in_place, guarded) {
   }
 
-  template <typename U = T>
-  explicit Guarded(T&& guarded,
-                   typename std::enable_if<std::is_move_constructible<U>::value>::type = void())
-      : guarded_(std::move(guarded)) {
+  explicit Guarded(T&& guarded) : guarded_(std::in_place, std::forward<T>(guarded)) {
+  }
+
+  ~Guarded() {
+    std::lock_guard<std::mutex> scoped_lock(lock_);
+    guarded_.reset();
   }
 
  private:
   friend class ScopedLock<T>;
-
   DISALLOW_COPY_AND_ASSIGN(Guarded);
 
   std::mutex lock_;
-  T guarded_;
+  std::optional<T> guarded_;
 };
 
 template <typename T>
 class ScopedLock {
  public:
-  explicit ScopedLock(Guarded<T>& guarded) : lock_(guarded.lock_), guarded_(guarded.guarded_) {
+  explicit ScopedLock(Guarded<T>& guarded) : lock_(guarded.lock_), guarded_(*guarded.guarded_) {
   }
 
   T& operator*() {