[runtime] Fix possible race in terminate handler (#4194)

2020-09-28 15:18:32 +03:00
parent b134b042bd
commit 0058928e67
10 changed files with 216 additions and 56 deletions
@@ -3596,6 +3596,14 @@ createInterop("auxiliaryCppSources") {
    it.extraOpts "-Xsource-compiler-option", "-std=c++17"
 }

+createInterop("concurrentTerminate") {
+    it.defFile 'interop/concurrentTerminate/concurrentTerminate.def'
+    it.headers "$projectDir/interop/concurrentTerminate/async.h"
+    // TODO: Using `-Xcompile-source` does not imply dependency on that source, so the task will no re-run when the source is updated.
+    it.extraOpts "-Xcompile-source", "$projectDir/interop/concurrentTerminate/async.cpp"
+    it.extraOpts "-Xsource-compiler-option", "-std=c++11"
+}
+
 createInterop("incomplete_types") {
    it.defFile 'interop/incomplete_types/library.def'
    it.headers "$projectDir/interop/incomplete_types/library.h"
@@ -3857,14 +3865,21 @@ interopTest("interop_auxiliarySources") {
    interop = 'auxiliaryCppSources'
 }

+interopTest("interop_concurrentTerminate") {
+    disabled = (project.testTarget == 'wasm32') // No interop for wasm yet.
+    source = "interop/concurrentTerminate/main.kt"
+    interop = 'concurrentTerminate'
+    goldValue = "Reporting error!\n"
+    outputChecker = { str -> str.endsWith(goldValue) }
+    expectedExitStatus = 99
+}
+
 interopTest("interop_incompleteTypes") {
    disabled = (project.testTarget == 'wasm32') // No interop for wasm yet.
    source = "interop/incomplete_types/main.kt"
    interop = 'incomplete_types'
 }

-
-
 interopTest("interop_embedStaticLibraries") {
    disabled = (project.testTarget == 'wasm32') // No interop for wasm yet.
    source = "interop/embedStaticLibraries/main.kt"
@@ -4175,6 +4190,16 @@ dynamicTest("produce_dynamic") {
                "Error handler: kotlin.Error: Expected error\n"
 }

+dynamicTest("interop_concurrentRuntime") {
+    disabled = (project.testTarget != null && project.testTarget != project.hostName)
+    source = "interop/concurrentTerminate/reverseInterop.kt"
+    cSource = "$projectDir/interop/concurrentTerminate/main.cpp"
+    clangTool = "clang++"
+    goldValue = "Uncaught Kotlin exception: kotlin.RuntimeException: Example"
+    outputChecker = { str -> str.startsWith(goldValue) }
+    expectedExitStatus = 99
+}
+
 task library_mismatch(type: KonanDriverTest) {
    // Does not work for cross targets yet.
    enabled = !(project.testTarget != null && project.target.name != project.hostName)
@@ -0,0 +1,29 @@
+#include <thread>
+#include <future>
+#include <chrono>
+#include <vector>
+#include <csignal>  // signal.h
+
+#include "async.h"
+
+int test_ConcurrentTerminate() {
+    signal(SIGABRT, *[](int){ exit(99); }); // Windows does not have sigaction
+
+    std::vector<std::future<void>> futures;
+#ifdef __linux__
+    // TODO: invalid terminate handler called from bridge on non-main thread on Linux X64
+    throw std::runtime_error("Reporting error!");
+#endif
+
+    for (size_t i = 0; i < 100; ++i) {
+        futures.emplace_back(std::async(std::launch::async,
+                [](size_t param) {
+                    std::this_thread::sleep_for(std::chrono::milliseconds(param));
+                    throw std::runtime_error("Reporting error!");
+                },
+                200 - i));
+    }
+
+    for (auto &future : futures) future.get();
+    return 0;
+}
@@ -0,0 +1,9 @@
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int test_ConcurrentTerminate();
+
+#ifdef __cplusplus
+}
+#endif
@@ -0,0 +1,5 @@
+package async
+
+---
+
+int test_ConcurrentTerminate();
@@ -0,0 +1,44 @@
+#include "testlib_api.h"
+
+#include <iostream>
+#include <thread>
+#include <future>
+#include <chrono>
+#include <vector>
+#include <csignal>  // signal.h
+
+using namespace std;
+
+static
+int runConcurrent() {
+
+    std::vector<std::future<void>> futures;
+
+    for (size_t i = 0; i < 100; ++i) {
+        futures.emplace_back(std::async(std::launch::async,
+                [](auto delay) {
+                    std::this_thread::sleep_for(std::chrono::milliseconds(delay));
+                    testlib_symbols()->kotlin.root.testTerminate();
+                },
+                100));
+    }
+
+    for (auto &future : futures) future.get();
+    return 0;
+}
+
+int main() {
+    signal(SIGABRT, *[](int){ exit(99); }); // Windows does not have sigaction
+
+    set_terminate([](){
+        cout << "This is the original terminate handler\n" << flush;
+        std::abort();
+    });
+
+    try {
+        runConcurrent();
+    } catch(...) {
+        std::cerr << "Unknown exception caught\n" << std::flush;
+    }
+    return 0;
+}
@@ -0,0 +1,7 @@
+import async.*
+import kotlinx.cinterop.*
+
+fun main() {
+    test_ConcurrentTerminate()
+    println("This is not expected.")
+}
@@ -0,0 +1,5 @@
+import kotlin.system.exitProcess
+
+fun testTerminate() {
+    throw RuntimeException("Example")
+}
@@ -418,6 +418,8 @@ open class KonanDynamicTest : KonanStandaloneTest() {
    @Input
    lateinit var cSource: String

+    var clangTool = "clang"
+
    // Replace testlib_api.h and all occurrences of the testlib with the actual name of the test
    private fun processCSource(): String {
        val sourceFile = File(cSource)
@@ -439,7 +441,7 @@ open class KonanDynamicTest : KonanStandaloneTest() {
        val plugin = project.convention.getPlugin(ExecClang::class.java)
        val execResult = plugin.execKonanClang(project.testTarget, Action<ExecSpec> {
            it.workingDir = File(outputDirectory)
-            it.executable = "clang"
+            it.executable = clangTool
            val artifactsDir = "$outputDirectory/${project.testTarget}"
            it.args = listOf(processCSource(),
                    "-o", executable,
@@ -19,6 +19,7 @@
 #include <stdint.h>

 #include <exception>
+#include <unistd.h>

 #if KONAN_NO_EXCEPTIONS
 #define OMIT_BACKTRACE 1
@@ -113,8 +114,6 @@ SourceInfo getSourceInfo(KConstRef stackTrace, int index) {

 }  // namespace

-extern "C" {
-
 // TODO: this implementation is just a hack, e.g. the result is inexact;
 // however it is better to have an inexact stacktrace than not to have any.
 NO_INLINE OBJ_GETTER0(Kotlin_getCurrentStackTrace) {
@@ -137,7 +136,7 @@ NO_INLINE OBJ_GETTER0(Kotlin_getCurrentStackTrace) {

  int size = backtrace(buffer, maxSize);
  if (size < kSkipFrames)
-      return AllocArrayInstance(theNativePtrArrayTypeInfo, 0, OBJ_RESULT);
+    return AllocArrayInstance(theNativePtrArrayTypeInfo, 0, OBJ_RESULT);

  ObjHolder resultHolder;
  ObjHeader* result = AllocArrayInstance(theNativePtrArrayTypeInfo, size - kSkipFrames, resultHolder.slot());
@@ -235,71 +234,104 @@ void OnUnhandledException(KRef throwable) {
  }
 }

+namespace {
+
+class {
+    /**
+     * Timeout 5 sec for concurrent (second) terminate attempt to give a chance the first one to finish.
+     * If the terminate handler hangs for 5 sec it is probably fatally broken, so let's do abnormal _Exit in that case.
+     */
+    unsigned int timeoutSec = 5;
+    int terminatingFlag = 0;
+  public:
+    template <class Fun> RUNTIME_NORETURN void operator()(Fun block) {
+      if (compareAndSet(&terminatingFlag, 0, 1)) {
+        block();
+        // block() is supposed to be NORETURN, otherwise go to normal abort()
+        konan::abort();
+      } else {
+        sleep(timeoutSec);
+        // We come here when another terminate handler hangs for 5 sec, that looks fatally broken. Go to forced exit now.
+      }
+      _Exit(EXIT_FAILURE); // force exit
+    }
+} concurrentTerminateWrapper;
+
+//! Process exception hook (if any) or just printStackTrace + write crash log
+void processUnhandledKotlinException(KRef throwable) {
+  OnUnhandledException(throwable);
 #if KONAN_REPORT_BACKTRACE_TO_IOS_CRASH_LOG
-static bool terminating = false;
-static SimpleMutex terminatingMutex;
+  ReportBacktraceToIosCrashLog(throwable);
 #endif
+}
+
+} // namespace

 RUNTIME_NORETURN void TerminateWithUnhandledException(KRef throwable) {
-  OnUnhandledException(throwable);
-
-#if KONAN_REPORT_BACKTRACE_TO_IOS_CRASH_LOG
-  {
-    LockGuard<SimpleMutex> lock(terminatingMutex);
-    if (!terminating) {
-      ReportBacktraceToIosCrashLog(throwable);
-    }
-  }
-#endif
-
-  konan::abort();
+  concurrentTerminateWrapper([=]() {
+      processUnhandledKotlinException(throwable);
+    konan::abort();
+  });
 }

 // Some libstdc++-based targets has limited support for std::current_exception and other C++11 functions.
 // This restriction can be lifted later when toolchains will be updated.
 #if KONAN_HAS_CXX11_EXCEPTION_FUNCTIONS

-static void (*oldTerminateHandler)() = nullptr;
+namespace {
+class TerminateHandler {

-static void callOldTerminateHandler() {
-#if KONAN_REPORT_BACKTRACE_TO_IOS_CRASH_LOG
-  {
-    LockGuard<SimpleMutex> lock(terminatingMutex);
-    terminating = true;
+  // In fact, it's safe to call my_handler directly from outside: it will do the job and then invoke original handler,
+  // even if it has not been initialized yet. So one may want to make it public and/or not the class member
+  RUNTIME_NORETURN static void kotlinHandler() {
+    concurrentTerminateWrapper([]() {
+      if (auto currentException = std::current_exception()) {
+        try {
+          std::rethrow_exception(currentException);
+        } catch (ExceptionObjHolder& e) {
+          processUnhandledKotlinException(e.obj());
+          konan::abort();
+        } catch (...) {
+          // Not a Kotlin exception - call default handler
+          instance().queuedHandler_();
+        }
+      }
+      // Come here in case of direct terminate() call or unknown exception - go to default terminate handler.
+      instance().queuedHandler_();
+    });
  }
-#endif

-  RuntimeCheck(oldTerminateHandler != nullptr, "Underlying exception handler is not set.");
-  oldTerminateHandler();
-}
+  using QH = __attribute__((noreturn)) void(*)();
+  QH queuedHandler_;

-static void KonanTerminateHandler() {
-  auto currentException = std::current_exception();
-  if (!currentException) {
-    // No current exception.
-    callOldTerminateHandler();
-  } else {
-    try {
-      std::rethrow_exception(currentException);
-    } catch (ExceptionObjHolder& e) {
-      TerminateWithUnhandledException(e.obj());
-    } catch (...) {
-      // Not a Kotlin exception.
-      callOldTerminateHandler();
-    }
+  /// Use machinery like Meyers singleton to provide thread safety
+  TerminateHandler()
+    : queuedHandler_((QH)std::set_terminate(kotlinHandler)) {}
+
+  static TerminateHandler& instance() {
+    static TerminateHandler singleton [[clang::no_destroy]];
+    return singleton;
  }
-}

-static SimpleMutex konanTerminateHandlerInitializationMutex;
+  // Copy, move and assign would be safe, but not much useful, so let's delete all (rule of 5)
+  TerminateHandler(const TerminateHandler&) = delete;
+  TerminateHandler(TerminateHandler&&) = delete;
+  TerminateHandler& operator=(const TerminateHandler&) = delete;
+  TerminateHandler& operator=(TerminateHandler&&) = delete;
+  // Dtor might be in use to restore original handler. However, consequent install
+  // will not reconstruct handler anyway, so let's keep dtor deleted to avoid confusion.
+  ~TerminateHandler() = delete;
+public:
+  /// First call will do the job, all consequent will do nothing.
+  static void install() {
+    instance(); // Use side effect of warming up
+  }
+};
+} // anon namespace

+// Use one public function to limit access to the class declaration
 void SetKonanTerminateHandler() {
-  if (oldTerminateHandler != nullptr) return; // Already initialized.
-
-  LockGuard<SimpleMutex> lockGuard(konanTerminateHandlerInitializationMutex);
-
-  if (oldTerminateHandler != nullptr) return; // Already initialized.
-
-  oldTerminateHandler = std::set_terminate(&KonanTerminateHandler);
+  TerminateHandler::install();
 }

 #else // KONAN_OBJC_INTEROP
@@ -310,8 +342,6 @@ void SetKonanTerminateHandler() {

 #endif // KONAN_OBJC_INTEROP

-} // extern "C"
-
 void DisallowSourceInfo() {
  disallowSourceInfo = true;
 }
@@ -24,13 +24,17 @@ extern "C" {
 #endif

 // Returns current stacktrace as Array<String>.
-OBJ_GETTER0(GetCurrentStackTrace);
+OBJ_GETTER0(Kotlin_getCurrentStackTrace);

 OBJ_GETTER(GetStackTraceStrings, KConstRef stackTrace);

+OBJ_GETTER(Kotlin_setUnhandledExceptionHook, KRef hook);
+
 // Throws arbitrary exception.
 void ThrowException(KRef exception);

+void OnUnhandledException(KRef throwable);
+
 RUNTIME_NORETURN void TerminateWithUnhandledException(KRef exception);

 void SetKonanTerminateHandler();