[SR-4756] If NSCopyObject is called on a Swift class with stored properties, it can cause a crash

[CharlesJS]

Previous ID	SR-4756
Radar	rdar://problem/32336877
Original Reporter	@CharlesJS
Type	Bug

Attachment: Download

Environment

macOS 16E195
Xcode 8E2002

Additional Detail from JIRA

Votes	0
Component/s
Labels	Bug, RunTimeCrash
Assignee	None
Priority	Medium

md5: 1d10337d50eac288bfc2d3eeb82fca3f

Issue Description:

When an NSTextFieldCell subclass contains Objective-C-compatible reference-type properties, and is loaded from a .nib file in which its containing text field has its Baseline aligned to some other object via autolayout, its properties get over-released when the cell is deallocated. A simple example that will cause the crash is:

@objc(Foo) class Foo: NSObject {}

@objc(CustomTextFieldCell) class CustomTextFieldCell: NSTextFieldCell {
    let foo = Foo()
}

The equivalent code in Objective-C works properly and does not crash:

#import <Cocoa/Cocoa.h>

@interface Foo: NSObject
@end

@implementation Foo
@end

@interface CustomTextFieldCell: NSTextFieldCell

@property (nonatomic, strong) Foo *foo;

@end

@implementation CustomTextFieldCell

- (instancetype)initWithCoder:(NSCoder *)coder {
    self->_foo = [Foo new];
    
    return [super initWithCoder:coder];
}

@end

The problem seems to occur, as far as I can tell, because when the Baseline layout relation is applied, AppKit copies the text field’s cell. Subsequently, NSCell’s -copyWithZone: method calls NSCopyObject, which in turn calls a private function named “fixUpCopiedIvars.” With an Objective-C cell class, fixUpCopiedIvars calls class_getIvarLayout, and retains all its instance variables, so both the original cell and the copy have an owning reference to all of them. This retain is then balanced by a release when the cell is deallocated. With a Swift cell class, however, class_getIvarLayout returns NULL, so the ivars are never retained; however, this nonexistent retain is still balanced by a release when the cell is deallocated. The result is that the program accesses freed memory, leading to a crash or worse.

I have attached a sample project which demonstrates the problem.

Steps to Reproduce:

1. Compile and run the attached project with zombies enabled.

2. Open a new document, and then close it.

3. Notice that you crash when the program tries to over-release the zombie object.

4. Now disable CustomTextFieldCell.swift, enable CustomTextFieldCell.m, and notice that everything now works.

Expected Results:
Even though NSCopyObject is old and deprecated, it may be called on Swift subclasses of Objective-C by legacy code, including code in Apple's own frameworks. Therefore, it should probably interact with it in ways that don't cause crashes.

Actual Results:
If NSCopyObject is called on a Swift subclass of an Objective-C class which has an Objective-C-compatible stored property, the property in the copy is over-released, leading to a crash.

Note:
Most workarounds don't work.

Implementing one's own copy(with:) method fails, because the crash occurs when calling super's implementation.

Using a weak variable to the containing NSControl doesn't work, because NSCopyObject doesn't play nice with weak variables either, causing errors to be logged to the console. Using unowned doesn't work, because the property cannot be null, and the connection will not be made until runtime.

Storing the property using objc_setAssociatedObject does work, but this is a cumbersome solution.

[belkadan]

I'm not sure there's anything we can do about this. Not all Swift properties are compatible with objc_retain, so we'd have to provide some kind of new entry point that knew how to do the retain (or really to do a copy). @gparker42, should we just close this as NTBF?

[gparker42]

I don't think we can get away with "NTBF, you can't subclass NSCell in Swift".

The NSCell path (which ought to be the only use of NSCopyObject that we care about) goes through -copyWithZone: first. We can do in Swift what non-ARC code had to do: emit an implementation of -copyWithZone: that calls super and then fixes up all of the memcpy'd ivars. That ought to be something practical for the compiler.

[gparker42]

Similarly: if we require the Swift author to implement something like copy() then the compiler can emit ivar-cleaning code after the call to super (by zero-filling and un-retaining as necessary).

[gparker42]

Alternatively, we may be able to use this to motivate some plan to get new NSCell subclasses off of NSCopyObject. But any solution there will have backward-deployment restrictions.

[belkadan]

We'd have to handle both handwritten copy(zone:) as well as an inherited one, but I guess that's doable. Yuck, though.

[belkadan]

@swift-ci create

[swift-ci]

Comment by Daniel Jalkut (JIRA)

I ran into a variation of this today. In my case a custom copyWithZone implementation in an NSCell subclass tries to ensure that one of its own custom properties is re-initialized, and in nil'ing out the property on the copy it inadvertently causes the property on the source to be zombied.

For example, given this subclass of NSCell:

class MyCell: NSCell {
    @objc var value: NSObject? = nil

    override func copy(with zone: NSZone? = nil) -> Any {
        let copiedCell = super.copy(with: zone) as! MyCell

        // Nilling out the copy target's instance variable...
        copiedCell.value = nil

        // Makes my access of my own value crash...
        if let thisValue = self.value {
            // Whatever
        }

        return copiedCell
    }
}

Copying a MyCell instance zombies the source object's `value` property:

let oneCell = MyCell()
oneCell.value = NSObject()
let copiedCell = oneCell.copy() as! MyCell

[belkadan]

Workaround: Unmanaged.passUnretained(copiedCell.value).retain(). But it's a pretty lousy workaround since it's depending on NSCopyObject having happened and happened poorly. (ObjC ARC would have the same problem.)

[gparker42]

ObjC ARC would have the same problem, except that the implementation of NSCopyObject() has been taught how to handle ARC ivars correctly.

[swift-ci]

Comment by Daniel Jalkut (JIRA)

Mm, and I was thinking of tackling it from the other direction: find a way to zero out the bytes without releasing them, but I guess that also depends upon the NSCopyObject not having arranged for them to be retained.

It seems like the only reasonable workaround is to keep NSCell subclasses in Objective-C for now, because that is the only place where a contract exists to keep 3rd party code and Apple code in sync.

[swift-ci]

Comment by Daniel Jalkut (JIRA)

I have another workaround, which seems to work for my trivial example: if the Swift-based properties are nil'd before calling super.copy..., then the copy will naturally be nil as well. If I save a reference to each affected property, copy with super, and then reset the affected property values, then I end up with a copy that is suitably zeroed.

The only uncertainty I have is whether a Swift subclass of NSCell can still count on its superclass's instance variables to be fixed up correctly? In the analysis above @CharlesJS says: "With a Swift cell class, however, class_getIvarLayout returns NULL, so the ivars are never retained" ... but does this apply only to the Swift subclass? Is class_getIvarLayout called for each class in the inheritance hierarchy?

[gparker42]

Identification of an ivar's memory management is dependent on the compilation of the ivar's implementing class. If you have a Swift-compiled subclass of an ARC-compiled superclass then the ARC ivars will be fixed up correctly independent of what Swift does.

class_getIvarLayout() describes the ivars implemented in that class itself. object_copy() calls class_getIvarLayout() on each class in the class hierarchy.

[belkadan]

Ah right. …and we don't bother to do it for Swift because there are plenty of Swift types that can't be described by the class_getIvarLayout format (just like C++), and exposing just some of the properties was decided to be confusing.

[CharlesJS]

This doesn't seem to be the case in my testing. When I define the `CustomTextFieldCell` class from the example above, set a breakpoint on `class_getIvarLayout`, and call `copy()`, `class_getIvarLayout` only gets called once, and `$rdi` is set to the `CustomTextFieldCell` class.

[gparker42]

Strictly speaking there is an optimization: there are bits in the class structure that say whether that class contains any ivar layout data, and the runtime skips class_getIvarLayout() at that level if all of those bits are unset.

[gparker42]

FWIW C++ ivars have a similar problem. NSCopyObject() makes no attempt to run C++ copy constructors.

[swift-ci]

Comment by Daniel Jalkut (JIRA)

Interesting. So if I am reading everything correctly, I think the workaround for my case might also serve as a general workaround for any Swift developer who wants to write a class that inherits from NSCell, and has its own reference properties:

1. Implement copy(with: ), even if you don't think you need to.
2. In the implementation, take care to first save a local reference to every reference property that was added in Swift.
3. Nil out every property in the source object that you have just saved local references to.
4. Call super.copy(with: ), creating a copy with expected nil properties.
5. Re-assign every local reference to the copy.
6. Re-assign every local reference to the source.

Anybody spot problems with this? The main gotcha that jumps out to me is that you will trigger any KVO or didSet type notification that is tied to the source object's properties.

[swift-ci]

Comment by Daniel Jalkut (JIRA)

Hah, of course I overlooked the obvious flaw in my reasoning: it won't work if the pertinent properties are not nullable. So, add to the recipe: make all Swift-added reference properties nullable, even if you don't think they should be 😉

[CharlesJS]

I don't think it's the optimization; class-dump reveals that NSTextFieldCell contains quite a few ivars, but class_getIvarLayout() isn't getting called for it. Perhaps returning NULL causes something to short-circuit?

[CharlesJS]

jalkut@red-sweater.com (JIRA User), that workaround looks more painful than just using objc_setAssociatedObject() for one's instance variables. 😉

[gparker42]

@CharlesJS the optimization shortcut looks for ARC ivars (and also ARC-compatible weak ivars from non-ARC code). Presumably NSTextFieldCell's @implementation was built without ARC in your OS version.

[swift-ci]

Comment by corbin dunn (JIRA)

FWIW, I do something like this: (sorry for the poor formatting)

class TableViewTextFieldCell: NSTextFieldCell {
private var previousTextColor: NSColor?

override func copy(with zone: NSZone? = nil) -\> Any {  
    let result: TableViewTextFieldCell = super .copy(with: zone) as! TableViewTextFieldCell  
    if let previousTextColor = result.previousTextColor {  
      let \_ = Unmanaged\<NSColor\>.passRetained(previousTextColor)  
    }  
    return result  
}  

}

[swift-ci]

Comment by corbin dunn (JIRA)

FYI: https://www.corbinstreehouse.com/blog/2019/04/implementing-nscell-copywithzone-in-swift-to-avoid-crashes-in-appkit/