Crashes due to race condition in swift bindings X.deinit vs X_copyFromCType/X_moveFromCType

[derolf]

Let's look at gluecodium/src/test/resources/smoke/basic_types/output/swift/smoke/BasicTypes.swift as an example

gluecodium/gluecodium/src/test/resources/smoke/basic_types/output/swift/smoke/BasicTypes.swift

Line 13 in 17d604a

smoke_BasicTypes_remove_swift_object_from_wrapper_cache(c_instance)

   deinit {
        smoke_BasicTypes_remove_swift_object_from_wrapper_cache(c_instance)   <--- HERE
        smoke_BasicTypes_release_handle(c_instance)
    }

gluecodium/gluecodium/src/test/resources/smoke/basic_types/output/swift/smoke/BasicTypes.swift

Line 101 in 17d604a

if let swift_pointer = smoke_BasicTypes_get_swift_object_from_wrapper_cache(handle),

internal func BasicTypes_copyFromCType(_ handle: _baseRef) -> BasicTypes {
    if let swift_pointer = smoke_BasicTypes_get_swift_object_from_wrapper_cache(handle),   <--- HERE
        let re_constructed = Unmanaged<AnyObject>.fromOpaque(swift_pointer).takeUnretainedValue() as? BasicTypes {
        return re_constructed
    }

• Thread A is just in deinit, but has not called smoke_BasicTypes_remove_swift_object_from_wrapper_cache yet, we have a dangling pointer to a dead swift object in the cache.
• Thread B is just calling smoke_BasicTypes_get_swift_object_from_wrapper_cache at the same time and gets the dead swift object
• Thread B will crash

To repro, have two threads doing:

  while true {
    // get some class from native side
    let foo = gluecodium.foo()
    // call some function on it 
    foo.bar()
    // foo.deinit() will now be called here due to ARC
  }

[derolf]

How to fix it:

The WrapperCache is quite simple on the C++ side, it just manages void* -> void*, so that can be done as well one the swift side. So we create a WrapperCache that manages weak refs on the swift side:

import Foundation

// WeakBox holds both a weak reference (for normal use) and an unowned identity reference
final class WeakBox<T: AnyObject> {
    weak var value: T?
    unowned let identity: T

    init(_ value: T) {
        self.value = value
        self.identity = value
    }
}

// Thread-safe, identity-aware cache for Swift wrapper objects
final class WrapperCache {
    private static var cache: [UInt64: WeakBox<AnyObject>] = [:]
    private static let lock = DispatchQueue(label: "com.gluecodium.wrapperCache")

    /// Retrieves a cached object if available, otherwise creates and stores it
    static func getOrCreate<T: AnyObject>(handle: UInt64, create: () -> T) -> T {
        return lock.sync {
            if let box = cache[handle], let value = box.value as? T {
                return value
            }
            let newValue = create()
            cache[handle] = WeakBox(newValue)
            return newValue
        }
    }

    /// Removes the cached object only if it's still the expected one (by identity)
    static func remove(handle: UInt64, expected: AnyObject) {
        lock.sync {
            if let box = cache[handle], box.identity === expected {
                cache.removeValue(forKey: handle)
            }
        }
    }
}

Notes:

• One WrapperCache.getOrCreate instead of two get + put makes sure that we don't create two wrappers if two threads race to BasicTypes_copyFromCType.
• WeakBox.identity is used to verify that we only remove the wrapper from the wrapper cache if it's still us and not a newer one (due to racing)
• Instead of having a global singleton for all types, we could also have one instance for each type.

So, let's rewrite

deinit {
    WrapperCache.remove(handle: c_instance, expected: self)
    smoke_BasicTypes_release_handle(c_instance)
}

internal func BasicTypes_copyFromCType(_ handle: _baseRef) -> BasicTypes {
    return WrapperCache.getOrCreate(handle: handle) {
        let copiedHandle = smoke_BasicTypes_copy_handle(handle)
        return BasicTypes(cBasicTypes: copiedHandle)
    }
}

Why it works:

If thread A is in deinit and thread B is in BasicTypes_copyFromCType, the weak reference in WeakBox is already null. So BasicTypes_copyFromCType will create and put a new wrapper. Then deinit will remove the old one.

[Hsilgos]

Hi, Daniel. Thanks for reporting and potential fix. We will discuss it internally.