[SR-704] EXC_BAD_ACCESS on returning nil from a failable initializer of NSObject subclass

[swift-ci]

Previous ID	SR-704
Radar	rdar://problem/25300543
Original Reporter	vlas (JIRA User)
Type	Bug
Status	Resolved
Resolution	Done

Attachment: Download

Environment

Xcode 7.3 beta 3:
Apple Swift version 2.2 (swiftlang-703.0.6.5 clang-703.0.21)
Target: x86_64-apple-macosx10.9

Also affects Snapshot 2016-02-08:
Apple Swift version 3.0-dev (LLVM a7663bb722, Clang 4ca3c7fa28, Swift 1c2f40e)

Additional Detail from JIRA

Votes	9
Component/s
Labels	Bug, RunTimeCrash
Assignee	@slavapestov
Priority	Medium

md5: dc88259bd369e3e8b2a14b1c97eabe8d

is duplicated by:

• SR-1097 Failable convenience initializer in extension of objc class crashes

Issue Description:

Returning nil from a failable initializer of an NSObject subclass results in a runtime crash in swift_deallocPartialClassInstance call. This doesn't happen if a class method is used instead of a convenience initializer, or if the class doesn't inherit from NSObject. The issue does not depend on optimization level. This is a regression from Swift 2.1 / Xcode 7.2 where the same setup doesn't crash.

Code example

import Foundation

class MyClass: NSObject {

    let property: String

    required init(value: String) {
        self.property = value

        super.init()
    }

    convenience init?(failableValue: String) {
        if failableValue != "" {
            self.init(value: failableValue)
        } else {
            return nil
        }
    }

    static func instanceWithFailableValue(failableValue: String) -> Self? {
        if failableValue != "" {
            return self.init(value: failableValue)
        } else {
            return nil
        }
    }

}

let goodValue = "Don't crash"
let badValue = ""

// This works correctly
let goodObject = MyClass(failableValue: goodValue)
print(goodObject)

// This also works correctly
let workaround = MyClass.instanceWithFailableValue(badValue)
print(workaround)

// This crashes upon returning from the initializer
let badObject = MyClass(failableValue: badValue)
print(badObject)

Stacktrace

* thread #​1: tid = 0x1792d1, 0x0000000100259f40 DemoProject`swift::Metadata::getClassObject() const, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #​0: 0x0000000100259f40 DemoProject`swift::Metadata::getClassObject() const
    frame #​1: 0x000000010025353e DemoProject`swift_deallocPartialClassInstance + 62
    frame #​2: 0x000000010026b9e5 DemoProject`MyClass.init(failableValue="") -> MyClass? + 357 at main.swift:0
    frame #​3: 0x000000010026ba42 DemoProject`MyClass.__allocating_init(failableValue : String) -> MyClass? + 66 at main.swift:0
    frame #​4: 0x000000010026c70b DemoProject`testFailableInitializer() -> () + 603 at main.swift:42
    frame #​5: 0x000000010026b67b DemoProject`main + 75 at main.swift:46
    frame #​6: 0x00007fff8edcb5ad libdyld.dylib`start + 1

[belkadan]

cc @slavapestov

[slavapestov]

So the immediate regression here is that swift_deallocPartialInstance() doesn't handle non-pointer isa, but in fact the old code was wrong too – it only looked like it worked when asserts were disabled. We would just go ahead and use a swift reference counting operation on an Objective-C object, which just clobbers some random memory before calling free().

@belkadan, should we just hack around this by directly calling free()?

[belkadan]

We can't guarantee that +alloc returned memory that came from malloc unless we emit a +alloc for any Swift subclass that has a failable init, but maybe it's close enough. Any such class would also have to override -dealloc, I think, and that could cause all sorts of havoc. @gparker42?

[slavapestov]

Yeah, I think we already had this discussion and the conclusion was that we would have a 'whitelist' of types that cannot allow -release without -init (like NSTimer), and call free() or -dealloc directly for those. In the future, we should try to define a proper protocol for this type of thing.

But in the short term, what would be a reasonable fix? It seems unfortunate that assertions enabled breaks something that used to "work", even if "work" wasn't perfect.

[belkadan]

Wait, how is this a regression from Swift 2.1? I thought we didn't support return-before-super until 2.2.

(@jckarter should probably be in this conversation too, especially if Greg's around.)

[jckarter]

We had supported early exit from convenience initializers, since there's no partial-deinitialization problem in that case.

[slavapestov]

We support 'return before self.init' in convenience inits in 2.1. Here the instance is not partially initialized, it's completely uninitialized – so for Swift objects, we can just free the memory.

For Objective-C objects, this works on accident if assertions are off, because swift_deallocObjectInstance() crashes if the object doesn't have a Swift refcount.

So it sounds like people were relying on this behavior in 2.1, with Objective-C objects, which is unfortunate.

[jckarter]

It definitely seems like a regression that we'd try to deallocPartialInstance after a convenience initializer failure.

[slavapestov]

Joe, deallocPartialInstance() takes a metaclass and an instance; the metaclass is the most derived class for which the instance is not initialized.

Convenience init failure before self.init() will call deallocPartialInstance() with the object and its isa. This effectively means it is completely uninitialized. This is the same behavior as before, except we had a special entry point for that.

Convenience init failure after self.init() just releases the instance as you would expect.

[slavapestov]

Oh, I guess the one regression is that deallocPartialInstance() doesn't support non-pointer isa. If we fix that, then an asserts-disabled build will have the same behavior as Swift 2.1. Should we do that?

[jckarter]

That sounds like a good bug to fix regardless. Preserving the 2.1 behavior seems like a good short-term plan for 2.2.

[slavapestov]

Alright. I'll fix the non-pointer isa thing, and also have it skip the assertion if we have an objc instance. That will give us the 2.1-without-asserts behavior in 2.2-with-asserts builds. For 3.0 we can look into a more comprehensive fix involving changing the class of the instance and sending a -release.

[jtbandes]

I just ran into this as well. FWIW, you can make the reproducible case even smaller:

import Foundation

extension NSObject {
    convenience init?(value: Int) {
        guard value < 5 else { return nil }
        self.init()
    }
}

if let c = NSObject(value: 5) {
    print("got \(c)")
}

[swift-ci]

Comment by Vlas Voloshin (JIRA)

As of Xcode 7.3 (7D175) with Swift 2.2 (swiftlang-703.0.18.1 clang-703.0.29), this issue no longer occurs with reproduction scenario in the issue description. However, scenario posted above by @jtbandes with a failable convenience initialiser on NSObject would still crash. Defining a failable convenience initialiser for an NSObject subclass in an extension and calling it doesn't crash though. Maybe base NSObject class is a special case?

[swift-ci]

Comment by Christopher Bryan Henderson (JIRA)

An initializer that throws is also broken. Code below crashes with release version of Xcode 7.3:

enum MyError: ErrorType {
    case Example
}

extension UIColor {
    convenience init(fail: Bool) throws {
        guard !fail else {
            throw MyError.Example
        }
        self.init(red: 1, green: 1, blue: 1, alpha: 1)
    }
}

let color = try? UIColor(fail: true)