From 1916a7ed08ce577961bbf192e2916789aef83122 Mon Sep 17 00:00:00 2001 From: Alexander Shabalin Date: Thu, 11 Jun 2020 12:05:05 +0300 Subject: [PATCH] Fix stacktrace printing during _tryRetain termination (#4213) Avoids using objc runtime for termination handling if illegal sharing is detected in _tryRetain, and adds a test for it. ObjC runtime uses shared non-recursive locks for object lifetime management methods, which include _tryRetain and object destruction. There are several locks per runtime divided between groups of objects based on their addresses. _tryRetain takes a lock, and if we terminate with unhandled exception inside its stack, we are not allowed to make ObjC runtime to destruct any object, because that object may take the same lock and consequently fail since locks are not recursive. Therefore, avoid touching ObjC runtime when terminating inside _tryRetain. This includes avoiding using ObjC's default unhandled exception handler, and our own usage of CoreSymbolication framework. --- backend.native/tests/build.gradle | 18 +++++++++++- .../objc/illegal_sharing_with_weak/main.kt | 28 +++++++++++++++++++ .../illegal_sharing_with_weak/objclib.def | 2 ++ .../objc/illegal_sharing_with_weak/objclib.h | 13 +++++++++ runtime/src/main/cpp/Exceptions.cpp | 16 +++++++++-- runtime/src/main/cpp/Exceptions.h | 3 ++ runtime/src/main/cpp/ObjCInterop.mm | 19 ++++++------- 7 files changed, 85 insertions(+), 14 deletions(-) create mode 100644 backend.native/tests/interop/objc/illegal_sharing_with_weak/main.kt create mode 100644 backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.def create mode 100644 backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.h diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index 6141a58535c..c810bb16651 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -3560,6 +3560,10 @@ if (PlatformInfo.isAppleTarget(project)) { it.headers "$projectDir/framework/gh3343/objclib.h" it.linkerOpts "-lobjcgh3343" } + createInterop("objc_illegal_sharing_with_weak") { + it.defFile 'interop/objc/illegal_sharing_with_weak/objclib.def' + it.headers "$projectDir/interop/objc/illegal_sharing_with_weak/objclib.h" + } } createInterop("withSpaces") { @@ -3913,6 +3917,18 @@ if (PlatformInfo.isAppleTarget(project)) { interop = 'objcKt34467' goldValue = "OK\n42\n" } + + interopTest("interop_objc_illegal_sharing_with_weak") { + source = "interop/objc/illegal_sharing_with_weak/main.kt" + interop = 'objc_illegal_sharing_with_weak' + + expectedExitStatusChecker = { it != 0 } + outputChecker = { + it.startsWith("Before") && // Should crash after "Before" + !it.contains("After") && // But before "After" + it.contains("isObjectAliveShouldCrash") // And should contain stack trace. + } + } } standaloneTest("jsinterop_math") { @@ -4479,4 +4495,4 @@ if (target.family == Family.IOS && (target.architecture == ARM32 || target.archi it instanceof KonanLinkTest } .forEach { it.enabled = false } -} \ No newline at end of file +} diff --git a/backend.native/tests/interop/objc/illegal_sharing_with_weak/main.kt b/backend.native/tests/interop/objc/illegal_sharing_with_weak/main.kt new file mode 100644 index 00000000000..d615e3bbc88 --- /dev/null +++ b/backend.native/tests/interop/objc/illegal_sharing_with_weak/main.kt @@ -0,0 +1,28 @@ +import kotlin.native.concurrent.* +import kotlinx.cinterop.autoreleasepool +import objclib.* + +fun main() { + autoreleasepool { + run() + } +} + +private class NSObjectImpl : NSObject() { + var x = 111 +} + +fun run() = withWorker { + val obj = NSObjectImpl() + setObject(obj) + + println("Before") + val isAlive = try { + execute(TransferMode.SAFE, {}) { + isObjectAliveShouldCrash() + }.result + } catch (e: Throwable) { + false + } + println("After $isAlive") +} diff --git a/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.def b/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.def new file mode 100644 index 00000000000..758f44a49a2 --- /dev/null +++ b/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.def @@ -0,0 +1,2 @@ +language = Objective-C +headerFilter = **/objclib.h diff --git a/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.h b/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.h new file mode 100644 index 00000000000..b44bffe7dc3 --- /dev/null +++ b/backend.native/tests/interop/objc/illegal_sharing_with_weak/objclib.h @@ -0,0 +1,13 @@ +#import + +static NSObject* __weak globalObject = nil; + +void setObject(NSObject* obj) { + globalObject = obj; +} + +// Make sure this function persists, because the test expects to find this function in the stack trace. +__attribute__((noinline)) +bool isObjectAliveShouldCrash() { + return globalObject != nil; +} diff --git a/runtime/src/main/cpp/Exceptions.cpp b/runtime/src/main/cpp/Exceptions.cpp index ad535102cc5..6ae34e90756 100644 --- a/runtime/src/main/cpp/Exceptions.cpp +++ b/runtime/src/main/cpp/Exceptions.cpp @@ -103,6 +103,14 @@ _Unwind_Reason_Code unwindCallback( } #endif +THREAD_LOCAL_VARIABLE bool disallowSourceInfo = false; + +SourceInfo getSourceInfo(KConstRef stackTrace, int index) { + return disallowSourceInfo + ? SourceInfo { .fileName = nullptr, .lineNumber = -1, .column = -1 } + : Kotlin_getSourceInfo(*PrimitiveArrayAddressOfElementAt(stackTrace->array(), index)); +} + } // namespace extern "C" { @@ -172,7 +180,7 @@ OBJ_GETTER(GetStackTraceStrings, KConstRef stackTrace) { RuntimeCheck(symbols != nullptr, "Not enough memory to retrieve the stacktrace"); for (int index = 0; index < size; ++index) { - auto sourceInfo = Kotlin_getSourceInfo(*PrimitiveArrayAddressOfElementAt(stackTrace->array(), index)); + auto sourceInfo = getSourceInfo(stackTrace, index); const char* symbol = symbols[index]; const char* result; char line[1024]; @@ -302,4 +310,8 @@ void SetKonanTerminateHandler() { #endif // KONAN_OBJC_INTEROP -} // extern "C" \ No newline at end of file +} // extern "C" + +void DisallowSourceInfo() { + disallowSourceInfo = true; +} diff --git a/runtime/src/main/cpp/Exceptions.h b/runtime/src/main/cpp/Exceptions.h index 4455f5d61d9..e71cabe6622 100644 --- a/runtime/src/main/cpp/Exceptions.h +++ b/runtime/src/main/cpp/Exceptions.h @@ -67,4 +67,7 @@ void PrintThrowable(KRef); } // extern "C" #endif +// It's not always safe to extract SourceInfo during unhandled exception termination. +void DisallowSourceInfo(); + #endif // RUNTIME_NAMES_H diff --git a/runtime/src/main/cpp/ObjCInterop.mm b/runtime/src/main/cpp/ObjCInterop.mm index 87f4927e03a..f74a85433a3 100644 --- a/runtime/src/main/cpp/ObjCInterop.mm +++ b/runtime/src/main/cpp/ObjCInterop.mm @@ -102,17 +102,14 @@ BOOL _tryRetainImp(id self, SEL _cmd) { 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(); - } + // Cannot use SourceInfo here, because CoreSymbolication framework (CSSymbolOwnerGetSymbolWithAddress) + // fails at recursive retain lock. Similarly, cannot use objc exception here, because its unhandled + // exception handler might fail at recursive retain lock too. + // TODO: Refactor to be more explicit. Instead of relying on an unhandled exception termination + // (and effectively setting a global to alter its behavior), just call an appropriate termination + // function by hand. + DisallowSourceInfo(); + std::terminate(); } }