Merge "Synchronize native ApkAssets data" into sc-dev

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*() {