Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-7734] libxpc and libdispatch objects returned to Swift from C will leak #653

Closed
CharlesJS opened this issue May 21, 2018 · 9 comments
Closed

Comments

@CharlesJS
Copy link

Previous ID SR-7734
Radar None
Original Reporter @CharlesJS
Type Bug
Status Resolved
Resolution Invalid

Attachment: Download

Environment

Xcode 9.3

Xcode 9.4 beta 2

Apple Swift version 4.1 (swiftlang-902.0.48 clang-902.0.37.1)

Apple Swift version 4.1.1 (swiftlang-902.0.53 clang-902.0.39.2)

Apple Swift version 4.2-dev (LLVM 072be1cf17, Clang c7422ffe24, Swift 3843d39297)

Additional Detail from JIRA
Votes 0
Component/s libdispatch
Labels Bug
Assignee None
Priority Medium

md5: 706f548fe2f336dd74986ef871d8fddc

Issue Description:

If Swift encounters a C header containing functions which return libxpc or libdispatch objects, like so:

#ifndef MakeStuff_h
#define MakeStuff_h

#include <xpc/xpc.h>
#include <dispatch/dispatch.h>

xpc_object_t CreateXPCDictionary(void);
dispatch_queue_t CreateDispatchQueue(void);

#endif /* MakeStuff_h */

It interprets them thusly:

import XPC
import Dispatch

public func CreateXPCDictionary() -> xpc_object_t!
public func CreateDispatchQueue() -> DispatchQueue!

Unfortunately, in each case, it assumes the returned object has a retain count of +0. However, as the function is in straight C, it cannot return less than +1 without the object being deallocated. This results in Swift generating a retain for the returned objects upon receiving them, which results in them being leaked.

Swift code:

import Foundation

func leak() {
    let xpcDict = CreateXPCDictionary()
    let queue = CreateDispatchQueue()
    
    print("xpcDict is \(String(describing: xpcDict))")
    print("queue is \(String(describing: queue))")
}

leak()

print("Check Instruments to see the leaked objects")
print("(Leaks will, for some reason, only detect the XPC dict)")

CFRunLoopRun()

Instruments history for the XPC dictionary:

0    Malloc    +1    1    00:02.264.064    MoreLeaks    CreateXPCDictionary
1    Retain    +1    2    00:02.264.104    MoreLeaks    leak()
2    Retain    +1    3    00:02.265.682    MoreLeaks    leak()
3    Retain    +1    4    00:02.265.972    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
4    Retain    +1    5    00:02.266.685    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
5    Retain    +1    6    00:02.266.753    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
6    Retain    +1    7    00:02.266.790    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
7    Release    -1    6    00:02.266.801    MoreLeaks    _T0Sq16debugDescriptionSSvg
8    Retain    +1    7    00:02.266.837    MoreLeaks    swift::metadataimpl::BufferValueWitnesses<swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>, 8ul, 8ul, (swift::metadataimpl::FixedPacking)1>::initializeBufferWithCopyOfBuffer(swift::ValueBuffer*, swift::ValueBuffer*, swift::TargetMetadata<swift::InProcess> const*)
9    Retain    +1    8    00:02.267.362    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
10    Release    -1    7    00:02.267.443    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
11    Retain    +1    8    00:02.267.511    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
12    Retain    +1    9    00:02.267.546    MoreLeaks    _T0So8NSObjectCs23CustomStringConvertible10FoundationsACP11descriptionSSvgTWTm
13    Release    -1    8    00:02.268.057    MoreLeaks    _T0So8NSObjectCs23CustomStringConvertible10FoundationsACP11descriptionSSvgTWTm
14    Release    -1    7    00:02.268.057    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
15    Release    -1    6    00:02.268.199    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
16    Release    -1    5    00:02.268.266    MoreLeaks    swift_arrayDestroy
17    Release    -1    4    00:02.268.301    MoreLeaks    specialized _print_unlocked<A, B>(_:_:)
18    Release    -1    3    00:02.268.302    MoreLeaks    specialized _print_unlocked<A, B>(_:_:)
19    Release    -1    2    00:02.268.302    MoreLeaks    specialized String.init<A>(describing:)
20    Release    -1    1    00:02.268.812    MoreLeaks    leak()

Instruments history for the dispatch queue:

0    Malloc    +1    1    00:02.264.111    MoreLeaks    CreateDispatchQueue
1    Retain    +1    2    00:02.264.124    MoreLeaks    leak()
2    Retain    +1    3    00:02.268.725    MoreLeaks    leak()
3    Retain    +1    4    00:02.268.729    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
4    Retain    +1    5    00:02.268.731    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
5    Retain    +1    6    00:02.268.731    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
6    Retain    +1    7    00:02.268.732    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
7    Release    -1    6    00:02.268.734    MoreLeaks    _T0Sq16debugDescriptionSSvg
8    Retain    +1    7    00:02.268.734    MoreLeaks    swift::metadataimpl::BufferValueWitnesses<swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>, 8ul, 8ul, (swift::metadataimpl::FixedPacking)1>::initializeBufferWithCopyOfBuffer(swift::ValueBuffer*, swift::ValueBuffer*, swift::TargetMetadata<swift::InProcess> const*)
9    Retain    +1    8    00:02.268.737    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
10    Release    -1    7    00:02.268.744    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
11    Retain    +1    8    00:02.268.744    MoreLeaks    swift::metadataimpl::ValueWitnesses<swift::metadataimpl::ObjCRetainableBox>::initializeWithCopy(swift::OpaqueValue*, swift::OpaqueValue*, swift::TargetMetadata<swift::InProcess> const*)
12    Retain    +1    9    00:02.268.745    MoreLeaks    _T0So8NSObjectCs23CustomStringConvertible10FoundationsACP11descriptionSSvgTWTm
13    Release    -1    8    00:02.268.791    MoreLeaks    _T0So8NSObjectCs23CustomStringConvertible10FoundationsACP11descriptionSSvgTWTm
14    Release    -1    7    00:02.268.792    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
15    Release    -1    6    00:02.268.797    MoreLeaks    _debugPrint_unlocked<A, B>(_:_:)
16    Release    -1    5    00:02.268.798    MoreLeaks    swift_arrayDestroy
17    Release    -1    4    00:02.268.799    MoreLeaks    specialized _print_unlocked<A, B>(_:_:)
18    Release    -1    3    00:02.268.799    MoreLeaks    specialized _print_unlocked<A, B>(_:_:)
19    Release    -1    2    00:02.268.799    MoreLeaks    specialized String.init<A>(describing:)
20    Release    -1    1    00:02.268.812    MoreLeaks    leak()

To solve this, both of these types of objects should probably be imported as Unmanaged, as CFType objects are.

I've attached a sample project which will demonstrate the problem.

@swift-ci
Copy link

Comment by Pierre Habouzit (JIRA)

Your sample project is bogus as it violates ARC rules.

You use ARC conventions through the importer, however you implemented obj-c returning functions in a C file that doesn't understand ARC, so it is your responsibility to make sure the header and implementation match, and they don't.

Please read https://clang.llvm.org/docs/AutomaticReferenceCounting.html#retained-return-values you are returning an obj-c type, so the Create naming rule doesn't apply (see it described here: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#c-retainable-pointer-types it only applies to CF types).

With this information, there are two ways to fix your project:

  • add `__attribute((ns_returns_retained))` on your two Create functions to teach the importer the convention C uses (note it'll likely be the most efficient solution in terms of performance)

  • make the C file an obj-c one with ARC, so that the compiler autoreleases the returned objects with a +0 as the headers implies they should be

@swift-ci
Copy link

Comment by Pierre Habouzit (JIRA)

F*![](@#*(&)@#(*&@!# stupid markdown I can't type code in that BS

@CharlesJS
Copy link
Author

With respect, this objection misses the mark. The types used by libxpc and libdispatch are not strictly Objective-C types in the sense that they are only available to Objective-C (and indeed, at least in the case of libdispatch, its types originally weren't Objective-C objects, as that compliance was only added in OS X 10.8 and iOS 6.0). Both libraries are available to plain C code, where it's valid to use them, and in which case their types can be manually retained and released via xpc_retain(), xpc_release(), dispatch_retain(), dispatch_release(), etc. In this case, the objects cannot be returned with a +0 retain count, as the plain C interface does not provide any way of which I am aware to do that.

Furthermore, it is possible that you do not control the C code you have to interact with, in which case modifying the C file's headers is not a valid option. This code then becomes impossible to use in Swift without leaking memory.

@CharlesJS
Copy link
Author

Furthermore, even if you do control the original C code, this isn't a good solution, because adding __attribute__((ns_returns_retained)) to the MoreLeaks project generates a compiler warning: "'ns_returns_retained' attribute only applies to functions that return an Objective-C object"

@swift-ci
Copy link

Comment by Pierre Habouzit (JIRA)

No it doesn't miss the mark, these types are objective-C objects when used in objective-C exactly so that the importer can reason about them being refcounted.
The fact that you decide to back your implementation with C is your choice, and it is then your responsibility to adhere to what the compiler sees.
Swift import would not work if the compliance for 6 years ago wasn't there, it is a pre-requisite.

As of the warning, I think it's easy enough to

```C
#if OBJC
#define RETURNS_RETAINED attribute((ns_returns_retained))
#else
#define RETURNS_RETAINED
#endif
```

Bridging C backed objective-C objects into Swift does require this kind of jumping through hoops, and if you don't like it, then use pure obj-c please.

@CharlesJS
Copy link
Author

1. Swift's importer works just fine with CFType objects, which are also technically Objective-C objects under the hood thanks to the toll-free bridge, and it does the right thing with them. Also as I said before, sometimes you are working with someone else's code and do not have the ability to change the headers.

2. Is this level of rudeness really necessary?

@belkadan
Copy link

Pierre is correct. Dispatch and XPC types aren't "Objective-C types under the hood"; they're Objective-C types in any context where Objective-C is enabled. (You can see the implementation of this in the <os/object.h> header.) It was a decision to let that stand in Swift, and one we're not going to change at this point. From the compiler's point of view, this is the same as declaring a function like this:

typedef NSObject *MyObject;
MyObject CreateMyObject(void);

and asking for a way to treat that as +1.

@belkadan
Copy link

Er, to sum it up:

  • I see how this would be useful.

  • But it would also be source-breaking, and there's no good way to implement it, and the problem only really affects sources you don't control. So we probably won't do anything here.

@CharlesJS
Copy link
Author

Thank you, Jordan, for your calm and diplomatic response, here and in general. I apologize for getting my feathers ruffled earlier, but it does take a non-trivial amount of time and effort to think up, test, and write reports for corner cases like this, and I appreciate your respectful and non-dismissive tone even in situations where we don't agree on our conclusions.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants