From 86321400385172e6bb938d53ce733f1fd7984b20 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Thu, 9 Apr 2020 19:22:30 -0700 Subject: [incfs] Fix a crash in worker thread calling JNI Worker thread has to initialize JNI separately to be able to call into managed binders implemented in the same system_server process, e.g. DataLoaderManager Bug: 153513507 Test: adb install megacity.nov4.apk; adb install megacity.v4.apk Change-Id: I668e8664361cd2fb3353ec50efd689c7d613658f --- .../core/jni/com_android_server_SystemServer.cpp | 2 +- ...ver_pm_PackageManagerShellCommandDataLoader.cpp | 2 + services/incremental/BinderIncrementalService.cpp | 12 +- services/incremental/BinderIncrementalService.h | 5 +- services/incremental/IncrementalService.cpp | 18 ++- services/incremental/IncrementalService.h | 9 +- services/incremental/ServiceWrappers.cpp | 180 ++++++++++++++++++++- services/incremental/ServiceWrappers.h | 116 ++----------- services/incremental/include/incremental_service.h | 2 +- .../incremental/test/IncrementalServiceTest.cpp | 28 +++- 10 files changed, 243 insertions(+), 131 deletions(-) diff --git a/services/core/jni/com_android_server_SystemServer.cpp b/services/core/jni/com_android_server_SystemServer.cpp index e99a264c42e4..76b171337bb9 100644 --- a/services/core/jni/com_android_server_SystemServer.cpp +++ b/services/core/jni/com_android_server_SystemServer.cpp @@ -135,7 +135,7 @@ static void android_server_SystemServer_spawnFdLeakCheckThread(JNIEnv*, jobject) static jlong android_server_SystemServer_startIncrementalService(JNIEnv* env, jclass klass, jobject self) { - return Incremental_IncrementalService_Start(); + return Incremental_IncrementalService_Start(env); } static void android_server_SystemServer_setIncrementalServiceSystemReady(JNIEnv* env, jclass klass, diff --git a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp index 853eba71d88a..e32a343433a8 100644 --- a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp +++ b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp @@ -410,6 +410,8 @@ private: // Installation. bool onPrepareImage(dataloader::DataLoaderInstallationFiles addedFiles) final { + ALOGE("onPrepareImage: start."); + JNIEnv* env = GetOrAttachJNIEnvironment(mJvm); const auto& jni = jniIds(env); diff --git a/services/incremental/BinderIncrementalService.cpp b/services/incremental/BinderIncrementalService.cpp index ebebf606b1cc..fc8c6feac22b 100644 --- a/services/incremental/BinderIncrementalService.cpp +++ b/services/incremental/BinderIncrementalService.cpp @@ -58,10 +58,10 @@ static bool incFsValid(const sp& vold) { return true; } -BinderIncrementalService::BinderIncrementalService(const sp& sm) - : mImpl(RealServiceManager(sm), getIncrementalDir()) {} +BinderIncrementalService::BinderIncrementalService(const sp& sm, JNIEnv* env) + : mImpl(RealServiceManager(sm, env), getIncrementalDir()) {} -BinderIncrementalService* BinderIncrementalService::start() { +BinderIncrementalService* BinderIncrementalService::start(JNIEnv* env) { if (!incFsEnabled()) { return nullptr; } @@ -81,7 +81,7 @@ BinderIncrementalService* BinderIncrementalService::start() { return nullptr; } - sp self(new BinderIncrementalService(sm)); + sp self(new BinderIncrementalService(sm, env)); status_t ret = sm->addService(String16{getServiceName()}, self); if (ret != android::OK) { return nullptr; @@ -290,8 +290,8 @@ binder::Status BinderIncrementalService::waitForNativeBinariesExtraction(int sto } // namespace android::os::incremental -jlong Incremental_IncrementalService_Start() { - return (jlong)android::os::incremental::BinderIncrementalService::start(); +jlong Incremental_IncrementalService_Start(JNIEnv* env) { + return (jlong)android::os::incremental::BinderIncrementalService::start(env); } void Incremental_IncrementalService_OnSystemReady(jlong self) { if (self) { diff --git a/services/incremental/BinderIncrementalService.h b/services/incremental/BinderIncrementalService.h index aca10ab6d591..5a7d5da56f18 100644 --- a/services/incremental/BinderIncrementalService.h +++ b/services/incremental/BinderIncrementalService.h @@ -18,6 +18,7 @@ #include #include +#include #include "IncrementalService.h" #include "android/os/incremental/BnIncrementalService.h" @@ -28,9 +29,9 @@ namespace android::os::incremental { class BinderIncrementalService : public BnIncrementalService, public BinderService { public: - BinderIncrementalService(const sp& sm); + BinderIncrementalService(const sp& sm, JNIEnv* env); - static BinderIncrementalService* start(); + static BinderIncrementalService* start(JNIEnv* env); static const char16_t* getServiceName() { return u"incremental"; } status_t dump(int fd, const Vector& args) final; diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index 400388e93262..2c6bf0a80fe0 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -237,6 +237,7 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v mDataLoaderManager(sm.getDataLoaderManager()), mIncFs(sm.getIncFs()), mAppOpsManager(sm.getAppOpsManager()), + mJni(sm.getJni()), mIncrementalDir(rootDir) { if (!mVold) { LOG(FATAL) << "Vold service is unavailable"; @@ -249,7 +250,10 @@ IncrementalService::IncrementalService(ServiceManagerWrapper&& sm, std::string_v } mJobQueue.reserve(16); - mJobProcessor = std::thread([this]() { runJobProcessing(); }); + mJobProcessor = std::thread([this]() { + mJni->initializeForCurrentThread(); + runJobProcessing(); + }); mountExistingImages(); } @@ -1248,9 +1252,10 @@ bool IncrementalService::configureNativeBinaries(StorageId storage, std::string_ continue; } - jobQueue.emplace_back([this, zipFile, entry, ifs, libFileId, - libPath = std::move(targetLibPath), makeFileTs]() mutable { - extractZipFile(ifs, zipFile.get(), entry, libFileId, libPath, makeFileTs); + jobQueue.emplace_back([this, zipFile, entry, ifs = std::weak_ptr(ifs), + libFileId, libPath = std::move(targetLibPath), + makeFileTs]() mutable { + extractZipFile(ifs.lock(), zipFile.get(), entry, libFileId, libPath, makeFileTs); }); if (sEnablePerfLogging) { @@ -1296,6 +1301,11 @@ void IncrementalService::extractZipFile(const IfsMountPtr& ifs, ZipArchiveHandle ZipEntry& entry, const incfs::FileId& libFileId, std::string_view targetLibPath, Clock::time_point scheduledTs) { + if (!ifs) { + LOG(INFO) << "Skipping zip file " << targetLibPath << " extraction for an expired mount"; + return; + } + auto libName = path::basename(targetLibPath); auto startedTs = Clock::now(); diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h index 4fdce4bd92dc..e7705df633d1 100644 --- a/services/incremental/IncrementalService.h +++ b/services/incremental/IncrementalService.h @@ -289,10 +289,11 @@ private: Clock::time_point scheduledTs); private: - std::unique_ptr const mVold; - std::unique_ptr const mDataLoaderManager; - std::unique_ptr const mIncFs; - std::unique_ptr const mAppOpsManager; + const std::unique_ptr mVold; + const std::unique_ptr mDataLoaderManager; + const std::unique_ptr mIncFs; + const std::unique_ptr mAppOpsManager; + const std::unique_ptr mJni; const std::string mIncrementalDir; mutable std::mutex mLock; diff --git a/services/incremental/ServiceWrappers.cpp b/services/incremental/ServiceWrappers.cpp index 9f4192fbf531..bf8e696a264c 100644 --- a/services/incremental/ServiceWrappers.cpp +++ b/services/incremental/ServiceWrappers.cpp @@ -14,8 +14,11 @@ * limitations under the License. */ +#define LOG_TAG "IncrementalService" + #include "ServiceWrappers.h" +#include #include using namespace std::literals; @@ -25,8 +28,123 @@ namespace android::os::incremental { static constexpr auto kVoldServiceName = "vold"sv; static constexpr auto kDataLoaderManagerName = "dataloader_manager"sv; -RealServiceManager::RealServiceManager(sp serviceManager) - : mServiceManager(std::move(serviceManager)) {} +class RealVoldService : public VoldServiceWrapper { +public: + RealVoldService(const sp vold) : mInterface(std::move(vold)) {} + ~RealVoldService() = default; + binder::Status mountIncFs(const std::string& backingPath, const std::string& targetDir, + int32_t flags, + IncrementalFileSystemControlParcel* _aidl_return) const final { + return mInterface->mountIncFs(backingPath, targetDir, flags, _aidl_return); + } + binder::Status unmountIncFs(const std::string& dir) const final { + return mInterface->unmountIncFs(dir); + } + binder::Status bindMount(const std::string& sourceDir, + const std::string& targetDir) const final { + return mInterface->bindMount(sourceDir, targetDir); + } + binder::Status setIncFsMountOptions( + const ::android::os::incremental::IncrementalFileSystemControlParcel& control, + bool enableReadLogs) const final { + return mInterface->setIncFsMountOptions(control, enableReadLogs); + } + +private: + sp mInterface; +}; + +class RealDataLoaderManager : public DataLoaderManagerWrapper { +public: + RealDataLoaderManager(const sp manager) + : mInterface(manager) {} + ~RealDataLoaderManager() = default; + binder::Status initializeDataLoader(MountId mountId, const DataLoaderParamsParcel& params, + const FileSystemControlParcel& control, + const sp& listener, + bool* _aidl_return) const final { + return mInterface->initializeDataLoader(mountId, params, control, listener, _aidl_return); + } + binder::Status getDataLoader(MountId mountId, sp* _aidl_return) const final { + return mInterface->getDataLoader(mountId, _aidl_return); + } + binder::Status destroyDataLoader(MountId mountId) const final { + return mInterface->destroyDataLoader(mountId); + } + +private: + sp mInterface; +}; + +class RealAppOpsManager : public AppOpsManagerWrapper { +public: + ~RealAppOpsManager() = default; + binder::Status checkPermission(const char* permission, const char* operation, + const char* package) const final { + return android::incremental::CheckPermissionForDataDelivery(permission, operation, package); + } + void startWatchingMode(int32_t op, const String16& packageName, + const sp& callback) final { + mAppOpsManager.startWatchingMode(op, packageName, callback); + } + void stopWatchingMode(const sp& callback) final { + mAppOpsManager.stopWatchingMode(callback); + } + +private: + android::AppOpsManager mAppOpsManager; +}; + +class RealJniWrapper final : public JniWrapper { +public: + RealJniWrapper(JavaVM* jvm); + void initializeForCurrentThread() const final; + + static JavaVM* getJvm(JNIEnv* env); + +private: + JavaVM* const mJvm; +}; + +class RealIncFs : public IncFsWrapper { +public: + RealIncFs() = default; + ~RealIncFs() = default; + Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const final { + return incfs::createControl(cmd, pendingReads, logs); + } + ErrorCode makeFile(const Control& control, std::string_view path, int mode, FileId id, + NewFileParams params) const final { + return incfs::makeFile(control, path, mode, id, params); + } + ErrorCode makeDir(const Control& control, std::string_view path, int mode) const final { + return incfs::makeDir(control, path, mode); + } + RawMetadata getMetadata(const Control& control, FileId fileid) const final { + return incfs::getMetadata(control, fileid); + } + RawMetadata getMetadata(const Control& control, std::string_view path) const final { + return incfs::getMetadata(control, path); + } + FileId getFileId(const Control& control, std::string_view path) const final { + return incfs::getFileId(control, path); + } + ErrorCode link(const Control& control, std::string_view from, std::string_view to) const final { + return incfs::link(control, from, to); + } + ErrorCode unlink(const Control& control, std::string_view path) const final { + return incfs::unlink(control, path); + } + base::unique_fd openForSpecialOps(const Control& control, FileId id) const final { + return base::unique_fd{incfs::openForSpecialOps(control, id).release()}; + } + ErrorCode writeBlocks(Span blocks) const final { + return incfs::writeBlocks(blocks); + } +}; + +RealServiceManager::RealServiceManager(sp serviceManager, JNIEnv* env) + : mServiceManager(std::move(serviceManager)), mJvm(RealJniWrapper::getJvm(env)) {} template sp RealServiceManager::getRealService(std::string_view serviceName) const { @@ -63,4 +181,62 @@ std::unique_ptr RealServiceManager::getAppOpsManager() { return std::make_unique(); } +std::unique_ptr RealServiceManager::getJni() { + return std::make_unique(mJvm); +} + +static JavaVM* getJavaVm(JNIEnv* env) { + CHECK(env); + JavaVM* jvm = nullptr; + env->GetJavaVM(&jvm); + CHECK(jvm); + return jvm; +} + +static JNIEnv* getJniEnv(JavaVM* vm) { + JNIEnv* env; + if (vm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6) != JNI_OK) { + return nullptr; + } + return env; +} + +static JNIEnv* getOrAttachJniEnv(JavaVM* jvm) { + if (!jvm) { + LOG(ERROR) << "No JVM instance"; + return nullptr; + } + + JNIEnv* env = getJniEnv(jvm); + if (!env) { + int result = jvm->AttachCurrentThread(&env, nullptr); + if (result != JNI_OK) { + LOG(ERROR) << "JVM thread attach failed: " << result; + return nullptr; + } + struct VmDetacher { + VmDetacher(JavaVM* vm) : mVm(vm) {} + ~VmDetacher() { mVm->DetachCurrentThread(); } + + private: + JavaVM* const mVm; + }; + static thread_local VmDetacher detacher(jvm); + } + + return env; +} + +RealJniWrapper::RealJniWrapper(JavaVM* jvm) : mJvm(jvm) { + CHECK(!!mJvm) << "JVM is unavailable"; +} + +void RealJniWrapper::initializeForCurrentThread() const { + (void)getOrAttachJniEnv(mJvm); +} + +JavaVM* RealJniWrapper::getJvm(JNIEnv* env) { + return getJavaVm(env); +} + } // namespace android::os::incremental diff --git a/services/incremental/ServiceWrappers.h b/services/incremental/ServiceWrappers.h index 84bf1ffaf45c..142bf2ef32f3 100644 --- a/services/incremental/ServiceWrappers.h +++ b/services/incremental/ServiceWrappers.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -93,6 +94,12 @@ public: virtual void stopWatchingMode(const sp& callback) = 0; }; +class JniWrapper { +public: + virtual ~JniWrapper() = default; + virtual void initializeForCurrentThread() const = 0; +}; + class ServiceManagerWrapper { public: virtual ~ServiceManagerWrapper() = default; @@ -100,127 +107,26 @@ public: virtual std::unique_ptr getDataLoaderManager() = 0; virtual std::unique_ptr getIncFs() = 0; virtual std::unique_ptr getAppOpsManager() = 0; + virtual std::unique_ptr getJni() = 0; }; // --- Real stuff --- -class RealVoldService : public VoldServiceWrapper { -public: - RealVoldService(const sp vold) : mInterface(std::move(vold)) {} - ~RealVoldService() = default; - binder::Status mountIncFs(const std::string& backingPath, const std::string& targetDir, - int32_t flags, - IncrementalFileSystemControlParcel* _aidl_return) const final { - return mInterface->mountIncFs(backingPath, targetDir, flags, _aidl_return); - } - binder::Status unmountIncFs(const std::string& dir) const final { - return mInterface->unmountIncFs(dir); - } - binder::Status bindMount(const std::string& sourceDir, - const std::string& targetDir) const final { - return mInterface->bindMount(sourceDir, targetDir); - } - binder::Status setIncFsMountOptions( - const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs) const final { - return mInterface->setIncFsMountOptions(control, enableReadLogs); - } - -private: - sp mInterface; -}; - -class RealDataLoaderManager : public DataLoaderManagerWrapper { -public: - RealDataLoaderManager(const sp manager) - : mInterface(manager) {} - ~RealDataLoaderManager() = default; - binder::Status initializeDataLoader(MountId mountId, const DataLoaderParamsParcel& params, - const FileSystemControlParcel& control, - const sp& listener, - bool* _aidl_return) const final { - return mInterface->initializeDataLoader(mountId, params, control, listener, _aidl_return); - } - binder::Status getDataLoader(MountId mountId, sp* _aidl_return) const final { - return mInterface->getDataLoader(mountId, _aidl_return); - } - binder::Status destroyDataLoader(MountId mountId) const final { - return mInterface->destroyDataLoader(mountId); - } - -private: - sp mInterface; -}; - -class RealAppOpsManager : public AppOpsManagerWrapper { -public: - ~RealAppOpsManager() = default; - binder::Status checkPermission(const char* permission, const char* operation, - const char* package) const final { - return android::incremental::CheckPermissionForDataDelivery(permission, operation, package); - } - void startWatchingMode(int32_t op, const String16& packageName, - const sp& callback) final { - mAppOpsManager.startWatchingMode(op, packageName, callback); - } - void stopWatchingMode(const sp& callback) final { - mAppOpsManager.stopWatchingMode(callback); - } - -private: - android::AppOpsManager mAppOpsManager; -}; - class RealServiceManager : public ServiceManagerWrapper { public: - RealServiceManager(sp serviceManager); + RealServiceManager(sp serviceManager, JNIEnv* env); ~RealServiceManager() = default; std::unique_ptr getVoldService() final; std::unique_ptr getDataLoaderManager() final; std::unique_ptr getIncFs() final; std::unique_ptr getAppOpsManager() final; + std::unique_ptr getJni() final; private: template sp getRealService(std::string_view serviceName) const; sp mServiceManager; -}; - -class RealIncFs : public IncFsWrapper { -public: - RealIncFs() = default; - ~RealIncFs() = default; - Control createControl(IncFsFd cmd, IncFsFd pendingReads, IncFsFd logs) const final { - return incfs::createControl(cmd, pendingReads, logs); - } - ErrorCode makeFile(const Control& control, std::string_view path, int mode, FileId id, - NewFileParams params) const final { - return incfs::makeFile(control, path, mode, id, params); - } - ErrorCode makeDir(const Control& control, std::string_view path, int mode) const final { - return incfs::makeDir(control, path, mode); - } - RawMetadata getMetadata(const Control& control, FileId fileid) const final { - return incfs::getMetadata(control, fileid); - } - RawMetadata getMetadata(const Control& control, std::string_view path) const final { - return incfs::getMetadata(control, path); - } - FileId getFileId(const Control& control, std::string_view path) const final { - return incfs::getFileId(control, path); - } - ErrorCode link(const Control& control, std::string_view from, std::string_view to) const final { - return incfs::link(control, from, to); - } - ErrorCode unlink(const Control& control, std::string_view path) const final { - return incfs::unlink(control, path); - } - base::unique_fd openForSpecialOps(const Control& control, FileId id) const final { - return base::unique_fd{incfs::openForSpecialOps(control, id).release()}; - } - ErrorCode writeBlocks(Span blocks) const final { - return incfs::writeBlocks(blocks); - } + JavaVM* const mJvm; }; } // namespace android::os::incremental diff --git a/services/incremental/include/incremental_service.h b/services/incremental/include/incremental_service.h index 4a34b11261b9..321387531694 100644 --- a/services/incremental/include/incremental_service.h +++ b/services/incremental/include/incremental_service.h @@ -24,7 +24,7 @@ __BEGIN_DECLS #define INCREMENTAL_LIBRARY_NAME "service.incremental.so" -jlong Incremental_IncrementalService_Start(); +jlong Incremental_IncrementalService_Start(JNIEnv* env); void Incremental_IncrementalService_OnSystemReady(jlong self); void Incremental_IncrementalService_OnDump(jlong self, jint fd); diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp index 991131950531..117dca8c37b3 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -261,28 +261,39 @@ public: sp mStoredCallback; }; +class MockJniWrapper : public JniWrapper { +public: + MOCK_CONST_METHOD0(initializeForCurrentThread, void()); + + MockJniWrapper() { EXPECT_CALL(*this, initializeForCurrentThread()).Times(1); } +}; + class MockServiceManager : public ServiceManagerWrapper { public: MockServiceManager(std::unique_ptr vold, - std::unique_ptr manager, + std::unique_ptr dataLoaderManager, std::unique_ptr incfs, - std::unique_ptr appOpsManager) + std::unique_ptr appOpsManager, + std::unique_ptr jni) : mVold(std::move(vold)), - mDataLoaderManager(std::move(manager)), + mDataLoaderManager(std::move(dataLoaderManager)), mIncFs(std::move(incfs)), - mAppOpsManager(std::move(appOpsManager)) {} + mAppOpsManager(std::move(appOpsManager)), + mJni(std::move(jni)) {} std::unique_ptr getVoldService() final { return std::move(mVold); } std::unique_ptr getDataLoaderManager() final { return std::move(mDataLoaderManager); } std::unique_ptr getIncFs() final { return std::move(mIncFs); } std::unique_ptr getAppOpsManager() final { return std::move(mAppOpsManager); } + std::unique_ptr getJni() final { return std::move(mJni); } private: std::unique_ptr mVold; std::unique_ptr mDataLoaderManager; std::unique_ptr mIncFs; std::unique_ptr mAppOpsManager; + std::unique_ptr mJni; }; // --- IncrementalServiceTest --- @@ -298,11 +309,15 @@ public: mIncFs = incFs.get(); auto appOps = std::make_unique>(); mAppOpsManager = appOps.get(); + auto jni = std::make_unique>(); + mJni = jni.get(); mIncrementalService = std::make_unique(MockServiceManager(std::move(vold), - std::move(dataloaderManager), + std::move( + dataloaderManager), std::move(incFs), - std::move(appOps)), + std::move(appOps), + std::move(jni)), mRootDir.path); mDataLoaderParcel.packageName = "com.test"; mDataLoaderParcel.arguments = "uri"; @@ -336,6 +351,7 @@ protected: NiceMock* mIncFs; NiceMock* mDataLoaderManager; NiceMock* mAppOpsManager; + NiceMock* mJni; std::unique_ptr mIncrementalService; TemporaryDir mRootDir; DataLoaderParamsParcel mDataLoaderParcel; -- cgit v1.2.3-59-g8ed1b