diff --git a/core/jni/android_content_res_ApkAssets.cpp b/core/jni/android_content_res_ApkAssets.cpp index 04528e98212f9..c32951af72e45 100644 --- a/core/jni/android_content_res_ApkAssets.cpp +++ b/core/jni/android_content_res_ApkAssets.cpp @@ -16,6 +16,8 @@ #define ATRACE_TAG ATRACE_TAG_RESOURCES +#include + #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>& ApkAssetsFromLong(jlong ptr) { + return *reinterpret_cast>*>(ptr); +} + +static jlong CreateGuardedApkAssets(std::unique_ptr assets) { + auto guarded_assets = new Guarded>(std::move(assets)); + return reinterpret_cast(guarded_assets); +} + +static void DeleteGuardedApkAssets(Guarded>& apk_assets) { + delete &apk_assets; +} + class LoaderAssetsProvider : public AssetsProvider { public: static std::unique_ptr 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(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(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(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(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(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(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(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(ptr); - return reinterpret_cast(apk_assets->GetLoadedArsc()->GetStringPool()); + auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr)); + auto apk_assets = scoped_apk_assets->get(); + return reinterpret_cast(apk_assets->GetLoadedArsc()->GetStringPool()); } static jboolean NativeIsUpToDate(JNIEnv* /*env*/, jclass /*clazz*/, jlong ptr) { - const ApkAssets* apk_assets = reinterpret_cast(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(ptr); + auto scoped_apk_assets = ScopedLock(ApkAssetsFromLong(ptr)); + auto apk_assets = scoped_apk_assets->get(); std::unique_ptr 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(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(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(); diff --git a/core/jni/android_content_res_ApkAssets.h b/core/jni/android_content_res_ApkAssets.h new file mode 100644 index 0000000000000..7e525dc75ef01 --- /dev/null +++ b/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>& ApkAssetsFromLong(jlong ptr); + +} // namespace android + +#endif // ANDROID_CONTENT_RES_APKASSETS_H diff --git a/core/jni/android_util_AssetManager.cpp b/core/jni/android_util_AssetManager.cpp index b2c69a0aeafdb..24f9abf6b8145 100644 --- a/core/jni/android_util_AssetManager.cpp +++ b/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(apk_assets_native_ptr)); + + auto scoped_assets = ScopedLock(ApkAssetsFromLong(apk_assets_native_ptr)); + apk_assets.push_back(scoped_assets->get()); } ScopedLock assetmanager(AssetManagerFromLong(ptr)); diff --git a/libs/androidfw/LoadedArsc.cpp b/libs/androidfw/LoadedArsc.cpp index 2a70f0d6a8040..cb620cc475a97 100644 --- a/libs/androidfw/LoadedArsc.cpp +++ b/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 header) { diff --git a/libs/androidfw/include/androidfw/AssetManager2.h b/libs/androidfw/include/androidfw/AssetManager2.h index 119f531b8634a..10666adfdceb3 100644 --- a/libs/androidfw/include/androidfw/AssetManager2.h +++ b/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. diff --git a/libs/androidfw/include/androidfw/LoadedArsc.h b/libs/androidfw/include/androidfw/LoadedArsc.h index d9225cd812ef8..3b222c556bfaa 100644 --- a/libs/androidfw/include/androidfw/LoadedArsc.h +++ b/libs/androidfw/include/androidfw/LoadedArsc.h @@ -153,8 +153,6 @@ class LoadedPackage { static std::unique_ptr 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_; diff --git a/libs/androidfw/include/androidfw/MutexGuard.h b/libs/androidfw/include/androidfw/MutexGuard.h index 64924f433245a..6fc6d64e2f6ee 100644 --- a/libs/androidfw/include/androidfw/MutexGuard.h +++ b/libs/androidfw/include/androidfw/MutexGuard.h @@ -18,6 +18,7 @@ #define ANDROIDFW_MUTEXGUARD_H #include +#include #include #include "android-base/macros.h" @@ -47,34 +48,32 @@ class Guarded { static_assert(!std::is_pointer::value, "T must not be a raw pointer"); public: - explicit Guarded() : guarded_() { + Guarded() : guarded_(std::in_place, T()) { } - template - explicit Guarded(const T& guarded, - typename std::enable_if::value>::type = void()) - : guarded_(guarded) { + explicit Guarded(const T& guarded) : guarded_(std::in_place, guarded) { } - template - explicit Guarded(T&& guarded, - typename std::enable_if::value>::type = void()) - : guarded_(std::move(guarded)) { + explicit Guarded(T&& guarded) : guarded_(std::in_place, std::forward(guarded)) { + } + + ~Guarded() { + std::lock_guard scoped_lock(lock_); + guarded_.reset(); } private: friend class ScopedLock; - DISALLOW_COPY_AND_ASSIGN(Guarded); std::mutex lock_; - T guarded_; + std::optional guarded_; }; template class ScopedLock { public: - explicit ScopedLock(Guarded& guarded) : lock_(guarded.lock_), guarded_(guarded.guarded_) { + explicit ScopedLock(Guarded& guarded) : lock_(guarded.lock_), guarded_(*guarded.guarded_) { } T& operator*() {