From a39b496938463fdd3ca9a8e8e4ccc3e589cd24c1 Mon Sep 17 00:00:00 2001
From: Vladimir Ivanov <knebekaizer@users.noreply.github.com>
Date: Wed, 22 Apr 2020 11:10:06 +0300
Subject: [PATCH] [runtime] Terminate on IncorrectDereferenceException from
 ObjC weak property (#4126)

Issue: ObjC runtime gets a os_unfair_lock when accessing weak property.
If exception is thrown from _tryRetainImp this lock left unlocked,
so it will hang on `__ulock_wait` on the next access.
Interim workaround: terminate instead of exception propagating.

However TerminateWithUnhandledException causes stacktrace dump to fail
in debug mode because of CSSymbolOwnerGetSymbolWithAddress failed on
the same recursive os_unfair_lock attempt.
---
 runtime/src/main/cpp/Memory.h            |  1 +
 runtime/src/main/cpp/Natives.h           |  1 +
 runtime/src/main/cpp/ObjCExportErrors.mm |  2 +-
 runtime/src/main/cpp/ObjCInterop.mm      | 20 +++++++++++++++++++-
 runtime/src/main/cpp/SourceInfo.h        |  7 ++++++-
 runtime/src/release/cpp/SourceInfo.cpp   |  2 +-
 6 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/runtime/src/main/cpp/Memory.h b/runtime/src/main/cpp/Memory.h
index 9768b5a1840..6e651448346 100644
--- a/runtime/src/main/cpp/Memory.h
+++ b/runtime/src/main/cpp/Memory.h
@@ -601,6 +601,7 @@ class ObjHolder {
    ObjHeader* obj_;
 };
 
+//! TODO Follow the Rule of Zero to prevent dangling on unintented copy ctor
 class ExceptionObjHolder {
  public:
    explicit ExceptionObjHolder(const ObjHeader* obj) {
diff --git a/runtime/src/main/cpp/Natives.h b/runtime/src/main/cpp/Natives.h
index 3b8dddad3ca..f7405679875 100644
--- a/runtime/src/main/cpp/Natives.h
+++ b/runtime/src/main/cpp/Natives.h
@@ -19,6 +19,7 @@
 
 #include "Types.h"
 #include "Exceptions.h"
+#include "Memory.h"
 
 constexpr size_t alignUp(size_t size, size_t alignment) {
   return (size + alignment - 1) & ~(alignment - 1);
diff --git a/runtime/src/main/cpp/ObjCExportErrors.mm b/runtime/src/main/cpp/ObjCExportErrors.mm
index f85cc940f43..d7c0a273500 100644
--- a/runtime/src/main/cpp/ObjCExportErrors.mm
+++ b/runtime/src/main/cpp/ObjCExportErrors.mm
@@ -46,7 +46,7 @@ extern "C" RUNTIME_NORETURN void Kotlin_ObjCExport_trapOnUndeclaredException(KRe
 static char kotlinExceptionOriginChar;
 
 static bool isExceptionOfType(KRef exception, const TypeInfo** types) {
-  for (int i = 0; types[i] != nullptr; ++i) {
+  if (types) for (int i = 0; types[i] != nullptr; ++i) {
     // TODO: use fast instance check when possible.
     if (IsInstance(exception, types[i])) return true;
   }
diff --git a/runtime/src/main/cpp/ObjCInterop.mm b/runtime/src/main/cpp/ObjCInterop.mm
index 817e885dbf1..87f4927e03a 100644
--- a/runtime/src/main/cpp/ObjCInterop.mm
+++ b/runtime/src/main/cpp/ObjCInterop.mm
@@ -16,6 +16,9 @@
 
 #if KONAN_OBJC_INTEROP
 
+#import <Foundation/NSException.h>
+#import <objc/objc-exception.h>
+
 #include <objc/objc.h>
 #include <objc/runtime.h>
 #include <objc/message.h>
@@ -95,7 +98,22 @@ BOOL _tryRetainImp(id self, SEL _cmd) {
   // this is a regression for instances of Kotlin subclasses of Obj-C classes:
   // loading a reference to such an object from Obj-C weak reference now fails on "wrong" thread
   // unless the object is frozen.
-  return getBackRef(self)->tryAddRef();
+  try {
+    return getBackRef(self)->tryAddRef();
+  } catch (ExceptionObjHolder& e) {
+    // TODO: check for IncorrectDereferenceException and possible weak property access
+    @try {
+      // try and catch with objc_terminate: this is a workaround to terminate immediately
+      // with libc default_terminate_handler() to be called instead of custom `TerminateWithUnhandledException`.
+      // See `KonanTerminateHandler`.
+      // TerminateWithUnhandledException shall not be used here because in debug mode it uses
+      // CoreSymbolication framework (CSSymbolOwnerGetSymbolWithAddress) which fails at recursive retain lock.
+      [NSException raise:NSGenericException
+                  format:@"Possible illegal attempt to access weak property from non-owning thread"];
+    } @catch (...) {
+      objc_terminate();
+    }
+  }
 }
 
 void releaseImp(id self, SEL _cmd) {
diff --git a/runtime/src/main/cpp/SourceInfo.h b/runtime/src/main/cpp/SourceInfo.h
index 1a72a831a0d..5e92548a1cb 100644
--- a/runtime/src/main/cpp/SourceInfo.h
+++ b/runtime/src/main/cpp/SourceInfo.h
@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+#ifndef RUNTIME_SOURCEINFO_H
+#define RUNTIME_SOURCEINFO_H
+
 struct SourceInfo {
     const char* fileName;
     int lineNumber;
@@ -28,4 +31,6 @@ struct SourceInfo Kotlin_getSourceInfo(void* addr);
 
 #ifdef __cplusplus
 } // extern "C"
-#endif
\ No newline at end of file
+#endif
+
+#endif // RUNTIME_SOURCEINFO_H
diff --git a/runtime/src/release/cpp/SourceInfo.cpp b/runtime/src/release/cpp/SourceInfo.cpp
index be607b4a762..653cea5050f 100644
--- a/runtime/src/release/cpp/SourceInfo.cpp
+++ b/runtime/src/release/cpp/SourceInfo.cpp
@@ -16,6 +16,6 @@
 
 #include "SourceInfo.h"
 
-extern "C" struct SourceInfo Kotlin_getSourceInfo(void* addr) {
+struct SourceInfo Kotlin_getSourceInfo(void* addr) {
   return (SourceInfo) { .fileName = nullptr, .lineNumber = -1, .column = -1 };
 }
\ No newline at end of file