From 9c7a002eb06ab318b627559c0a69c75979f2d110 Mon Sep 17 00:00:00 2001 From: Svyatoslav Scherbina Date: Wed, 7 Oct 2020 14:42:00 +0300 Subject: [PATCH] Prevent Obj-C dealloc accessing Kotlin counterpart #KT-41811 Fixed. Also add GC assertion for add ref for reclaimed object. --- .../tests/interop/objc/tests/kt41811.h | 22 +++++ .../tests/interop/objc/tests/kt41811.kt | 83 +++++++++++++++++++ .../tests/interop/objc/tests/kt41811.m | 37 +++++++++ .../tests/objcexport/deallocRetain.kt | 12 +++ .../tests/objcexport/deallocRetain.swift | 67 +++++++++++++++ .../tests/objcexport/expectedLazy.h | 13 +++ runtime/src/main/cpp/Memory.cpp | 2 + runtime/src/main/cpp/MemorySharedRefs.cpp | 13 +++ runtime/src/main/cpp/MemorySharedRefs.hpp | 4 +- runtime/src/main/cpp/ObjCInterop.mm | 18 ++++ runtime/src/objc/cpp/ObjCExportClasses.mm | 16 ++++ 11 files changed, 286 insertions(+), 1 deletion(-) create mode 100644 backend.native/tests/interop/objc/tests/kt41811.h create mode 100644 backend.native/tests/interop/objc/tests/kt41811.kt create mode 100644 backend.native/tests/interop/objc/tests/kt41811.m create mode 100644 backend.native/tests/objcexport/deallocRetain.kt create mode 100644 backend.native/tests/objcexport/deallocRetain.swift diff --git a/backend.native/tests/interop/objc/tests/kt41811.h b/backend.native/tests/interop/objc/tests/kt41811.h new file mode 100644 index 00000000000..7f64f6fc699 --- /dev/null +++ b/backend.native/tests/interop/objc/tests/kt41811.h @@ -0,0 +1,22 @@ +#import + +extern BOOL deallocRetainReleaseDeallocated; + +@interface DeallocRetainRelease : NSObject +@end; + +@protocol WeakReference +@required +@property (weak) id referent; +@end; + +@interface ObjCWeakReference : NSObject +@property (weak) id referent; +@end; + +extern id weakDeallocLoadWeak; +extern BOOL deallocLoadWeakDeallocated; + +@interface DeallocLoadWeak : NSObject +-(void)checkWeak; +@end; diff --git a/backend.native/tests/interop/objc/tests/kt41811.kt b/backend.native/tests/interop/objc/tests/kt41811.kt new file mode 100644 index 00000000000..5106473b0e1 --- /dev/null +++ b/backend.native/tests/interop/objc/tests/kt41811.kt @@ -0,0 +1,83 @@ +import kotlin.native.ref.WeakReference +import kotlinx.cinterop.* +import kotlin.test.* +import objcTests.* + +// Note: these tests rely on GC assertions: without the fix and the assertions it won't actually crash. +// GC should fire an assertion if it obtains a reference to Kotlin object that is being (or has been) deallocated. + +@Test +fun testKT41811() { + // Attempt to make the state predictable: + kotlin.native.internal.GC.collect() + + deallocRetainReleaseDeallocated = false + assertFalse(deallocRetainReleaseDeallocated) + + createGarbageDeallocRetainRelease() + + // Runs [DeallocRetainRelease dealloc]: + kotlin.native.internal.GC.collect() + + assertTrue(deallocRetainReleaseDeallocated) + + // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object: + kotlin.native.internal.GC.collect() +} + +private fun createGarbageDeallocRetainRelease() { + autoreleasepool { + object : DeallocRetainRelease() {} + } +} + +@Test +fun testKT41811LoadWeak() { + testKT41811LoadWeak(ObjCWeakReference()) +} + +@Test +fun testKT41811LoadKotlinWeak() { + val kotlinWeak = object : NSObject(), WeakReferenceProtocol { + lateinit var weakReference: WeakReference + + override fun referent(): Any? { + return weakReference.value + } + + override fun setReferent(referent: Any?) { + weakReference = WeakReference(referent!!) + } + } + + testKT41811LoadWeak(kotlinWeak) +} + +private fun testKT41811LoadWeak(weakRef: WeakReferenceProtocol) { + // Attempt to make the state predictable: + kotlin.native.internal.GC.collect() + + deallocLoadWeakDeallocated = false + assertFalse(deallocLoadWeakDeallocated) + + createGarbageDeallocLoadWeak(weakRef) + + // Runs [DeallocLoadWeak dealloc]: + kotlin.native.internal.GC.collect() + + assertTrue(deallocLoadWeakDeallocated) + + // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object: + kotlin.native.internal.GC.collect() + + weakDeallocLoadWeak = null +} + +private fun createGarbageDeallocLoadWeak(weakRef: WeakReferenceProtocol) { + autoreleasepool { + val obj = object : DeallocLoadWeak() {} + weakDeallocLoadWeak = weakRef.apply { referent = obj } + obj.checkWeak() + assertSame(obj, weakDeallocLoadWeak!!.referent) + } +} diff --git a/backend.native/tests/interop/objc/tests/kt41811.m b/backend.native/tests/interop/objc/tests/kt41811.m new file mode 100644 index 00000000000..f54e05a6e30 --- /dev/null +++ b/backend.native/tests/interop/objc/tests/kt41811.m @@ -0,0 +1,37 @@ +#import "kt41811.h" +#import "assert.h" + +id retainObject = nil; +BOOL deallocRetainReleaseDeallocated = NO; + +@implementation DeallocRetainRelease +-(void)dealloc { + retainObject = self; + assert(retainObject == self); + retainObject = nil; + + assert(!deallocRetainReleaseDeallocated); + deallocRetainReleaseDeallocated = YES; +} +@end; + +@implementation ObjCWeakReference +@end; + +id weakDeallocLoadWeak = nil; +BOOL deallocLoadWeakDeallocated = NO; + +@implementation DeallocLoadWeak +-(void)checkWeak { + assert(weakDeallocLoadWeak != nil); + assert(weakDeallocLoadWeak.referent == self); +} + +-(void)dealloc { + assert(weakDeallocLoadWeak != nil); + assert(weakDeallocLoadWeak.referent == nil); + + assert(!deallocLoadWeakDeallocated); + deallocLoadWeakDeallocated = YES; +} +@end; diff --git a/backend.native/tests/objcexport/deallocRetain.kt b/backend.native/tests/objcexport/deallocRetain.kt new file mode 100644 index 00000000000..540e3776b62 --- /dev/null +++ b/backend.native/tests/objcexport/deallocRetain.kt @@ -0,0 +1,12 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +package deallocretain + +open class DeallocRetainBase + +fun garbageCollect() = kotlin.native.internal.GC.collect() + +fun createWeakReference(value: Any) = kotlin.native.ref.WeakReference(value) diff --git a/backend.native/tests/objcexport/deallocRetain.swift b/backend.native/tests/objcexport/deallocRetain.swift new file mode 100644 index 00000000000..3677337b667 --- /dev/null +++ b/backend.native/tests/objcexport/deallocRetain.swift @@ -0,0 +1,67 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +import Kt + +// Note: these tests rely on GC assertions: without the fix and the assertions it won't actually crash. +// GC should fire an assertion if it obtains a reference to Kotlin object that is being (or has been) deallocated. + +private func test1() throws { + // Attempt to make the state predictable: + DeallocRetainKt.garbageCollect() + + DeallocRetain.deallocated = false + try assertFalse(DeallocRetain.deallocated) + + try autoreleasepool { + let obj = DeallocRetain() + try obj.checkWeak() + } + + // Runs DeallocRetain.deinit: + DeallocRetainKt.garbageCollect() + + try assertTrue(DeallocRetain.deallocated) + + // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object: + DeallocRetainKt.garbageCollect() +} + +private class DeallocRetain : DeallocRetainBase { + static var deallocated = false + static var retainObject: DeallocRetain? = nil + static weak var weakObject: DeallocRetain? = nil + static var kotlinWeakRef: KotlinWeakReference? = nil + + override init() { + super.init() + DeallocRetain.weakObject = self + DeallocRetain.kotlinWeakRef = DeallocRetainKt.createWeakReference(value: self) + } + + func checkWeak() throws { + try assertSame(actual: DeallocRetain.weakObject, expected: self) + try assertSame(actual: DeallocRetain.kotlinWeakRef!.value, expected: self) + } + + deinit { + DeallocRetain.retainObject = self + DeallocRetain.retainObject = nil + + try! assertNil(DeallocRetain.weakObject) + try! assertNil(DeallocRetain.kotlinWeakRef!.value) + + try! assertFalse(DeallocRetain.deallocated) + DeallocRetain.deallocated = true + } +} + +class DeallocRetainTests : SimpleTestProvider { + override init() { + super.init() + + test("Test1", test1) + } +} diff --git a/backend.native/tests/objcexport/expectedLazy.h b/backend.native/tests/objcexport/expectedLazy.h index 43f80af0daa..3373278b0a5 100644 --- a/backend.native/tests/objcexport/expectedLazy.h +++ b/backend.native/tests/objcexport/expectedLazy.h @@ -318,6 +318,19 @@ __attribute__((swift_name("CoroutinesKt"))) + (void)invoke1Block:(id)block argument:(id _Nullable)argument completionHandler:(void (^)(id _Nullable, NSError * _Nullable))completionHandler __attribute__((swift_name("invoke1(block:argument:completionHandler:)"))); @end; +__attribute__((swift_name("DeallocRetainBase"))) +@interface KtDeallocRetainBase : KtBase +- (instancetype)init __attribute__((swift_name("init()"))) __attribute__((objc_designated_initializer)); ++ (instancetype)new __attribute__((availability(swift, unavailable, message="use object initializers instead"))); +@end; + +__attribute__((objc_subclassing_restricted)) +__attribute__((swift_name("DeallocRetainKt"))) +@interface KtDeallocRetainKt : KtBase ++ (void)garbageCollect __attribute__((swift_name("garbageCollect()"))); ++ (KtKotlinWeakReference *)createWeakReferenceValue:(id)value __attribute__((swift_name("createWeakReference(value:)"))); +@end; + __attribute__((swift_name("FHolder"))) @interface KtFHolder : KtBase - (instancetype)init __attribute__((swift_name("init()"))) __attribute__((objc_designated_initializer)); diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index a73d78b35b1..b84aa23315e 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -1578,10 +1578,12 @@ inline void addHeapRef(ContainerHeader* container) { case CONTAINER_TAG_STACK: break; case CONTAINER_TAG_LOCAL: + RuntimeAssert(container->refCount() > 0, "add ref for reclaimed object"); incrementRC(container); break; /* case CONTAINER_TAG_FROZEN: case CONTAINER_TAG_SHARED: */ default: + RuntimeAssert(container->refCount() > 0, "add ref for reclaimed object"); incrementRC(container); break; } diff --git a/runtime/src/main/cpp/MemorySharedRefs.cpp b/runtime/src/main/cpp/MemorySharedRefs.cpp index eb5a5e67099..71180cf38d8 100644 --- a/runtime/src/main/cpp/MemorySharedRefs.cpp +++ b/runtime/src/main/cpp/MemorySharedRefs.cpp @@ -127,6 +127,8 @@ void BackRefFromAssociatedObject::addRef() { static_assert(errorPolicy != ErrorPolicy::kDefaultValue, "Cannot use default return value here"); if (atomicAdd(&refCount, 1) == 1) { + if (obj_ == nullptr) return; // E.g. after [detach]. + // There are no references to the associated object itself, so Kotlin object is being passed from Kotlin, // and it is owned therefore. ensureRefAccessible(obj_, context_); // TODO: consider removing explicit verification. @@ -144,6 +146,8 @@ template bool BackRefFromAssociatedObject::tryAddRef() { static_assert(errorPolicy != ErrorPolicy::kDefaultValue, "Cannot use default return value here"); + if (obj_ == nullptr) return false; // E.g. after [detach]. + // Suboptimal but simple: ensureRefAccessible(obj_, context_); @@ -165,6 +169,8 @@ template bool BackRefFromAssociatedObject::tryAddRef(); void BackRefFromAssociatedObject::releaseRef() { ForeignRefContext context = context_; if (atomicAdd(&refCount, -1) == 0) { + if (obj_ == nullptr) return; // E.g. after [detach]. + // Note: by this moment "subsequent" addRef may have already happened and patched context_. // So use the value loaded before refCount update: DeinitForeignRef(obj_, context); @@ -173,8 +179,15 @@ void BackRefFromAssociatedObject::releaseRef() { } } +void BackRefFromAssociatedObject::detach() { + RuntimeAssert(atomicGet(&refCount) == 0, "unexpected refCount") + obj_ = nullptr; // Handled in addRef/tryAddRef/releaseRef/ref. +} + template ObjHeader* BackRefFromAssociatedObject::ref() const { + RuntimeAssert(obj_ != nullptr, "no valid Kotlin object found"); + if (!ensureRefAccessible(obj_, context_)) { return nullptr; } diff --git a/runtime/src/main/cpp/MemorySharedRefs.hpp b/runtime/src/main/cpp/MemorySharedRefs.hpp index f81a6945f40..c6d8fc8c82c 100644 --- a/runtime/src/main/cpp/MemorySharedRefs.hpp +++ b/runtime/src/main/cpp/MemorySharedRefs.hpp @@ -54,12 +54,14 @@ class BackRefFromAssociatedObject { void releaseRef(); + void detach(); + // Error if called from the wrong worker with non-frozen obj_. template ObjHeader* ref() const; private: - ObjHeader* obj_; + ObjHeader* obj_; // May be null before [initAndAddRef] or after [detach]. ForeignRefContext context_; volatile int refCount; }; diff --git a/runtime/src/main/cpp/ObjCInterop.mm b/runtime/src/main/cpp/ObjCInterop.mm index 95d67d160d6..9316a24d638 100644 --- a/runtime/src/main/cpp/ObjCInterop.mm +++ b/runtime/src/main/cpp/ObjCInterop.mm @@ -118,6 +118,24 @@ void releaseImp(id self, SEL _cmd) { } void releaseAsAssociatedObjectImp(id self, SEL _cmd) { + // This function is called by the GC. It made a decision to reclaim Kotlin object, and runs + // deallocation hooks at the moment, including deallocation of the "associated object" ([self]) + // using the [super release] call below. + + // The deallocation involves running [self dealloc] which can contain arbitrary code. + // In particular, this code can retain and release [self]. Obj-C and Swift runtimes handle this + // gracefully (unless the object gets accessed after the deallocation of course), but Kotlin doesn't. + // For example, this happens in https://youtrack.jetbrains.com/issue/KT-41811, provoked by + // UIViewController.dealloc (which retains-releases self._view._viewDelegate == self) and UIView.dealloc. + // Generally retaining and releasing Kotlin object that is being deallocated would lead to + // use-after-dispose and double-dispose problems (with unpredictable consequences) or to an assertion failure. + // To workaround this, detach the back ref from the Kotlin object: + getBackRef(self)->detach(); + // So retain/release/etc. on [self] won't affect the Kotlin object, and an attempt to get + // the reference to it (e.g. when calling Kotlin method on [self]) would crash. + // The latter is generally ok, because by the time superclass dealloc gets launched, subclass state + // should already be deinitialized, and Kotlin methods operate on the subclass. + // [super release] Class clazz = object_getClass(self); struct objc_super s = {self, clazz}; diff --git a/runtime/src/objc/cpp/ObjCExportClasses.mm b/runtime/src/objc/cpp/ObjCExportClasses.mm index ff38fd4f6ed..c1bd9a206e0 100644 --- a/runtime/src/objc/cpp/ObjCExportClasses.mm +++ b/runtime/src/objc/cpp/ObjCExportClasses.mm @@ -133,6 +133,22 @@ static void injectToRuntime(); } -(void)releaseAsAssociatedObject { + // This function is called by the GC. It made a decision to reclaim Kotlin object, and runs + // deallocation hooks at the moment, including deallocation of the "associated object" ([self]) + // using the [super release] call below. + + // The deallocation involves running [self dealloc] which can contain arbitrary code. + // In particular, this code can retain and release [self]. Obj-C and Swift runtimes handle this + // gracefully (unless the object gets accessed after the deallocation of course), but Kotlin doesn't. + // Generally retaining and releasing Kotlin object that is being deallocated would lead to + // use-after-dispose and double-dispose problems (with unpredictable consequences) or to an assertion failure. + // To workaround this, detach the back ref from the Kotlin object: + refHolder.detach(); + // So retain/release/etc. on [self] won't affect the Kotlin object, and an attempt to get + // the reference to it (e.g. when calling Kotlin method on [self]) would crash. + // The latter is generally ok because can be triggered only by user-defined Swift/Obj-C + // subclasses of Kotlin classes. + [super release]; }