From c5dce22a8c921421e2150cf4835efd5e493a59e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=97=B1=20PixelyIon?= Date: Thu, 5 Dec 2019 21:05:34 +0530 Subject: [PATCH] Fix JNI Race Condition, Fix Release Builds and Fix Searching This commit fixes JNI race conditions by usage of a mutex, fixes a bug in release builds due to ProGuard member obfuscation and fix searching by fixing the HeaderAdapter filter. --- app/CMakeLists.txt | 4 ++ app/proguard-rules.pro | 2 +- app/src/main/cpp/main.cpp | 18 +++++- app/src/main/cpp/skyline/common.cpp | 10 --- app/src/main/cpp/skyline/common.h | 59 +---------------- app/src/main/cpp/skyline/gpu.cpp | 24 +++---- app/src/main/cpp/skyline/gpu.h | 1 + app/src/main/cpp/skyline/jvm.cpp | 13 ++++ app/src/main/cpp/skyline/jvm.h | 64 +++++++++++++++++++ app/src/main/cpp/skyline/nce.cpp | 6 +- app/src/main/java/emu/skyline/GameActivity.kt | 6 ++ .../java/emu/skyline/adapter/HeaderAdapter.kt | 4 +- 12 files changed, 125 insertions(+), 86 deletions(-) create mode 100644 app/src/main/cpp/skyline/jvm.cpp create mode 100644 app/src/main/cpp/skyline/jvm.h diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index df154f43..0e38efee 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -1,6 +1,7 @@ cmake_minimum_required(VERSION 3.8) project(Skyline VERSION 0.3) +set(BUILD_TESTS OFF) set(BUILD_TESTING OFF) set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED TRUE) @@ -10,8 +11,10 @@ if (uppercase_CMAKE_BUILD_TYPE STREQUAL "RELEASE") add_compile_definitions(NDEBUG) endif() +set(CMAKE_POLICY_DEFAULT_CMP0048 OLD) add_subdirectory("libraries/tinyxml2") add_subdirectory("libraries/fmt") +set(CMAKE_POLICY_DEFAULT_CMP0048 NEW) include_directories(${source_DIR}/skyline) @@ -19,6 +22,7 @@ add_library(skyline SHARED ${source_DIR}/main.cpp ${source_DIR}/skyline/common.cpp ${source_DIR}/skyline/nce.cpp + ${source_DIR}/skyline/jvm.cpp ${source_DIR}/skyline/gpu.cpp ${source_DIR}/skyline/gpu/display.cpp ${source_DIR}/skyline/gpu/parcel.cpp diff --git a/app/proguard-rules.pro b/app/proguard-rules.pro index ec37c031..4dde1733 100644 --- a/app/proguard-rules.pro +++ b/app/proguard-rules.pro @@ -6,4 +6,4 @@ void writeObject(java.io.ObjectOutputStream); void readObject(java.io.ObjectInputStream); } --keepclassmembernames class emu.skyline.GameActivity { *; } +-keep class emu.skyline.GameActivity { *; } diff --git a/app/src/main/cpp/main.cpp b/app/src/main/cpp/main.cpp index c4579be9..cecdfa15 100644 --- a/app/src/main/cpp/main.cpp +++ b/app/src/main/cpp/main.cpp @@ -1,11 +1,12 @@ #include "skyline/common.h" #include "skyline/os.h" +#include "skyline/jvm.h" #include -#include #include -bool Halt{}; -uint FaultCount{}; +bool Halt; +uint FaultCount; +std::mutex jniMtx; void signalHandler(int signal) { syslog(LOG_ERR, "Halting program due to signal: %s", strsignal(signal)); @@ -17,6 +18,9 @@ void signalHandler(int signal) { } extern "C" JNIEXPORT void Java_emu_skyline_GameActivity_executeRom(JNIEnv *env, jobject instance, jstring romJstring, jint romType, jint romFd, jint preferenceFd, jint logFd) { + Halt = false; + FaultCount = 0; + std::signal(SIGTERM, signalHandler); std::signal(SIGSEGV, signalHandler); std::signal(SIGINT, signalHandler); @@ -49,3 +53,11 @@ extern "C" JNIEXPORT void Java_emu_skyline_GameActivity_executeRom(JNIEnv *env, auto end = std::chrono::steady_clock::now(); logger->Info("Done in: {} ms", (std::chrono::duration_cast(end - start).count())); } + +extern "C" JNIEXPORT void Java_emu_skyline_GameActivity_lockMutex(JNIEnv *env, jobject instance) { + jniMtx.lock(); +} + +extern "C" JNIEXPORT void Java_emu_skyline_GameActivity_unlockMutex(JNIEnv *env, jobject instance) { + jniMtx.unlock(); +} diff --git a/app/src/main/cpp/skyline/common.cpp b/app/src/main/cpp/skyline/common.cpp index fca451e6..38937a45 100644 --- a/app/src/main/cpp/skyline/common.cpp +++ b/app/src/main/cpp/skyline/common.cpp @@ -75,16 +75,6 @@ namespace skyline { logFile.flush(); } - JvmManager::JvmManager(JNIEnv *env, jobject instance) : env(env), instance(instance), instanceClass(env->GetObjectClass(instance)) {} - - jobject JvmManager::GetField(const char *key, const char *signature) { - return env->GetObjectField(instance, env->GetFieldID(instanceClass, key, signature)); - } - - bool JvmManager::CheckNull(const char *key, const char *signature) { - return env->IsSameObject(env->GetObjectField(instance, env->GetFieldID(instanceClass, key, signature)), nullptr); - } - DeviceState::DeviceState(kernel::OS *os, std::shared_ptr &thisProcess, std::shared_ptr &thisThread, std::shared_ptr jvmManager, std::shared_ptr settings, std::shared_ptr logger) : os(os), jvmManager(std::move(jvmManager)), settings(std::move(settings)), logger(std::move(logger)), thisProcess(thisProcess), thisThread(thisThread) { // We assign these later as they use the state in their constructor and we don't want null pointers diff --git a/app/src/main/cpp/skyline/common.h b/app/src/main/cpp/skyline/common.h index 4b23a7cf..a269a986 100644 --- a/app/src/main/cpp/skyline/common.h +++ b/app/src/main/cpp/skyline/common.h @@ -333,64 +333,6 @@ namespace skyline { inline exception(const S &formatStr, Args &&... args) : runtime_error(fmt::format(formatStr, args...)) {} }; - /** - * @brief The JvmManager class is used to simplify transactions with the Java component - */ - class JvmManager { - public: - JNIEnv *env; //!< A pointer to the JNI environment - jobject instance; //!< A reference to the activity - jclass instanceClass; //!< The class of the activity - - /** - * @param env A pointer to the JNI environment - * @param instance A reference to the activity - */ - JvmManager(JNIEnv *env, jobject instance); - - /** - * @brief Retrieves a specific field of the given type from the activity - * @tparam objectType The type of the object in the field - * @param key The name of the field in the activity class - * @return The contents of the field as objectType - */ - template - objectType GetField(const char *key) { - if constexpr(std::is_same()) - return env->GetBooleanField(instance, env->GetFieldID(instanceClass, key, "Z")); - else if constexpr(std::is_same()) - return env->GetByteField(instance, env->GetFieldID(instanceClass, key, "B")); - else if constexpr(std::is_same()) - return env->GetCharField(instance, env->GetFieldID(instanceClass, key, "C")); - else if constexpr(std::is_same()) - return env->GetShortField(instance, env->GetFieldID(instanceClass, key, "S")); - else if constexpr(std::is_same()) - return env->GetIntField(instance, env->GetFieldID(instanceClass, key, "I")); - else if constexpr(std::is_same()) - return env->GetLongField(instance, env->GetFieldID(instanceClass, key, "J")); - else if constexpr(std::is_same()) - return env->GetFloatField(instance, env->GetFieldID(instanceClass, key, "F")); - else if constexpr(std::is_same()) - return env->GetDoubleField(instance, env->GetFieldID(instanceClass, key, "D")); - } - - /** - * @brief Retrieves a specific field from the activity as a jobject - * @param key The name of the field in the activity class - * @param signature The signature of the field - * @return A jobject of the contents of the field - */ - jobject GetField(const char *key, const char *signature); - - /** - * @brief Checks if a specific field from the activity is null or not - * @param key The name of the field in the activity class - * @param signature The signature of the field - * @return If the field is null or not - */ - bool CheckNull(const char *key, const char *signature); - }; - /** * @brief Returns the current time in nanoseconds * @return The current time in nanoseconds @@ -400,6 +342,7 @@ namespace skyline { } class NCE; + class JvmManager; namespace gpu { class GPU; } diff --git a/app/src/main/cpp/skyline/gpu.cpp b/app/src/main/cpp/skyline/gpu.cpp index 48501918..99796ba7 100644 --- a/app/src/main/cpp/skyline/gpu.cpp +++ b/app/src/main/cpp/skyline/gpu.cpp @@ -3,6 +3,7 @@ #include "gpu/devices/nvhost_ctrl_gpu.h" #include "gpu/devices/nvhost_channel.h" #include "gpu/devices/nvhost_as_gpu.h" +#include "jvm.h" #include #include @@ -21,18 +22,17 @@ namespace skyline::gpu { } void GPU::Loop() { - if(state.jvmManager->CheckNull("surface", "Landroid/view/Surface;")) { - while (state.jvmManager->CheckNull("surface", "Landroid/view/Surface;")) { - if(state.jvmManager->GetField("halt")) - return; - sched_yield(); - } - window = ANativeWindow_fromSurface(state.jvmManager->env, state.jvmManager->GetField("surface", "Landroid/view/Surface;")); - ANativeWindow_acquire(window); - resolution.width = static_cast(ANativeWindow_getWidth(window)); - resolution.height = static_cast(ANativeWindow_getHeight(window)); - format = ANativeWindow_getFormat(window); - } + if (surfaceUpdate) { + if (!state.jvmManager->CheckNull("surface", "Landroid/view/Surface;")) { + window = ANativeWindow_fromSurface(state.jvmManager->env, state.jvmManager->GetField("surface", "Landroid/view/Surface;")); + ANativeWindow_acquire(window); + resolution.width = static_cast(ANativeWindow_getWidth(window)); + resolution.height = static_cast(ANativeWindow_getHeight(window)); + format = ANativeWindow_getFormat(window); + } else + return; + } else + surfaceUpdate = state.jvmManager->CheckNull("surface", "Landroid/view/Surface;"); if (!bufferQueue.displayQueue.empty()) { auto &buffer = bufferQueue.displayQueue.front(); bufferQueue.displayQueue.pop(); diff --git a/app/src/main/cpp/skyline/gpu.h b/app/src/main/cpp/skyline/gpu.h index aa7b9ada..ad105a18 100644 --- a/app/src/main/cpp/skyline/gpu.h +++ b/app/src/main/cpp/skyline/gpu.h @@ -17,6 +17,7 @@ namespace skyline::gpu { u32 fdIndex{}; //!< Holds the index of a file descriptor std::unordered_map> deviceMap; //!< A map from a NvDeviceType to the NvDevice object std::unordered_map> fdMap; //!< A map from an FD to a shared pointer to it's NvDevice object + bool surfaceUpdate{}; //!< If the surface needs to be updated double prevTime{}; public: diff --git a/app/src/main/cpp/skyline/jvm.cpp b/app/src/main/cpp/skyline/jvm.cpp new file mode 100644 index 00000000..9f6fb15c --- /dev/null +++ b/app/src/main/cpp/skyline/jvm.cpp @@ -0,0 +1,13 @@ +#include "jvm.h" + +namespace skyline { + JvmManager::JvmManager(JNIEnv *env, jobject instance) : env(env), instance(instance), instanceClass(env->GetObjectClass(instance)) {} + + jobject JvmManager::GetField(const char *key, const char *signature) { + return env->GetObjectField(instance, env->GetFieldID(instanceClass, key, signature)); + } + + bool JvmManager::CheckNull(const char *key, const char *signature) { + return env->IsSameObject(env->GetObjectField(instance, env->GetFieldID(instanceClass, key, signature)), nullptr); + } +} diff --git a/app/src/main/cpp/skyline/jvm.h b/app/src/main/cpp/skyline/jvm.h new file mode 100644 index 00000000..aa418841 --- /dev/null +++ b/app/src/main/cpp/skyline/jvm.h @@ -0,0 +1,64 @@ +#pragma once + +#include +#include + +namespace skyline { + /** + * @brief The JvmManager class is used to simplify transactions with the Java component + */ + class JvmManager { + public: + JNIEnv *env; //!< A pointer to the JNI environment + jobject instance; //!< A reference to the activity + jclass instanceClass; //!< The class of the activity + + /** + * @param env A pointer to the JNI environment + * @param instance A reference to the activity + */ + JvmManager(JNIEnv *env, jobject instance); + + /** + * @brief Retrieves a specific field of the given type from the activity + * @tparam objectType The type of the object in the field + * @param key The name of the field in the activity class + * @return The contents of the field as objectType + */ + template + inline objectType GetField(const char *key) { + if constexpr(std::is_same()) + return env->GetBooleanField(instance, env->GetFieldID(instanceClass, key, "Z")); + else if constexpr(std::is_same()) + return env->GetByteField(instance, env->GetFieldID(instanceClass, key, "B")); + else if constexpr(std::is_same()) + return env->GetCharField(instance, env->GetFieldID(instanceClass, key, "C")); + else if constexpr(std::is_same()) + return env->GetShortField(instance, env->GetFieldID(instanceClass, key, "S")); + else if constexpr(std::is_same()) + return env->GetIntField(instance, env->GetFieldID(instanceClass, key, "I")); + else if constexpr(std::is_same()) + return env->GetLongField(instance, env->GetFieldID(instanceClass, key, "J")); + else if constexpr(std::is_same()) + return env->GetFloatField(instance, env->GetFieldID(instanceClass, key, "F")); + else if constexpr(std::is_same()) + return env->GetDoubleField(instance, env->GetFieldID(instanceClass, key, "D")); + } + + /** + * @brief Retrieves a specific field from the activity as a jobject + * @param key The name of the field in the activity class + * @param signature The signature of the field + * @return A jobject of the contents of the field + */ + jobject GetField(const char *key, const char *signature); + + /** + * @brief Checks if a specific field from the activity is null or not + * @param key The name of the field in the activity class + * @param signature The signature of the field + * @return If the field is null or not + */ + bool CheckNull(const char *key, const char *signature); + }; +} diff --git a/app/src/main/cpp/skyline/nce.cpp b/app/src/main/cpp/skyline/nce.cpp index d2a97691..38dae7c2 100644 --- a/app/src/main/cpp/skyline/nce.cpp +++ b/app/src/main/cpp/skyline/nce.cpp @@ -1,9 +1,11 @@ #include #include #include -#include +#include "os.h" +#include "jvm.h" extern bool Halt; +extern std::mutex jniMtx; namespace skyline { void NCE::ReadRegisters(user_pt_regs ®isters, pid_t pid) const { @@ -32,6 +34,7 @@ namespace skyline { void NCE::Execute() { int status = 0; while (!Halt && !state.os->processMap.empty()) { + jniMtx.lock(); for (const auto &process : state.os->processMap) { state.os->thisProcess = process.second; state.os->thisThread = process.second->threadMap.at(process.first); @@ -88,6 +91,7 @@ namespace skyline { state.os->serviceManager.Loop(); state.gpu->Loop(); Halt = state.jvmManager->GetField("halt"); + jniMtx.unlock(); } for (const auto &process : state.os->processMap) { state.os->KillThread(process.first); diff --git a/app/src/main/java/emu/skyline/GameActivity.kt b/app/src/main/java/emu/skyline/GameActivity.kt index 478fa0b2..2ae5602e 100644 --- a/app/src/main/java/emu/skyline/GameActivity.kt +++ b/app/src/main/java/emu/skyline/GameActivity.kt @@ -28,6 +28,8 @@ class GameActivity : AppCompatActivity(), SurfaceHolder.Callback, InputQueue.Cal private var halt: Boolean = false private external fun executeRom(romString: String, romType: Int, romFd: Int, preferenceFd: Int, logFd: Int) + private external fun lockMutex() + private external fun unlockMutex() override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -62,7 +64,9 @@ class GameActivity : AppCompatActivity(), SurfaceHolder.Callback, InputQueue.Cal override fun surfaceCreated(holder: SurfaceHolder?) { Log.d("surfaceCreated", "Holder: ${holder.toString()}") + lockMutex() surface = holder!!.surface + unlockMutex() } override fun surfaceChanged(holder: SurfaceHolder?, format: Int, width: Int, height: Int) { @@ -71,7 +75,9 @@ class GameActivity : AppCompatActivity(), SurfaceHolder.Callback, InputQueue.Cal override fun surfaceDestroyed(holder: SurfaceHolder?) { Log.d("surfaceDestroyed", "Holder: ${holder.toString()}") + lockMutex() surface = null + unlockMutex() } override fun onInputQueueCreated(queue: InputQueue?) { diff --git a/app/src/main/java/emu/skyline/adapter/HeaderAdapter.kt b/app/src/main/java/emu/skyline/adapter/HeaderAdapter.kt index 0cf1f204..b41b0eb4 100644 --- a/app/src/main/java/emu/skyline/adapter/HeaderAdapter.kt +++ b/app/src/main/java/emu/skyline/adapter/HeaderAdapter.kt @@ -73,6 +73,7 @@ internal abstract class HeaderAdapter input.close() fileObj.close() @@ -126,7 +127,7 @@ internal abstract class HeaderAdapter avgScore) @@ -139,6 +140,7 @@ internal abstract class HeaderAdapter) { + @Suppress("UNCHECKED_CAST") visibleArray = results.values as ArrayList notifyDataSetChanged() }