-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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-74] Passing reference of uninitialized lazy to C function leads to memory issues during deinit #42696
Comments
I don't actually think this is supposed to be ok. Once a Swift object has entered deinit, no other code should have access to the object. Also, storing a pointer to |
Comment by Etan Kissling (JIRA) Thanks for the comment! First of all, passing The rest of this comment addresses your concerns about the validity of the issue.
Yes, and my example does not violate this. Only During
Could you please quote relevant sections of sources that support this statement? While I also lack any official reference that describes how exactly Swift's memory model works, I wrote down the practical implications that your statement would have:
|
The fact that it appears to work in practice does not mean that it actually works according to the language semantics. Just look at C, where there's practically infinite ways to write code that works in practice with current compilers but actually invokes undefined behavior. The semantics of Swift explicitly state that when passing an inout reference to a function that takes an
Yes it does. You're passing a pointer into the memory of
Not strictly true. They're valid and can be accessed from your code inside of
Certainly. The Swift Programming Language, Language Reference, Declarations, In-Out Parameters:
(emphasis mine)
It's not that there is "no reliable way", it's that it's strictly illegal to dereference that pointer after the function ends if it was created from an inout reference. If you need a pointer that lasts longer, you need to actually alloc it on the heap and manage the memory.
Not really; in all my years of doing Mac/iOS programming, I've never seen the
Here the pointed-to data is irrelevant. It's using its own pointer as its value simply in order to guarantee uniqueness. And the actual usage of this value in the code is with a simple pointer equality comparison:
I fail to see how the semantics of inout parameters has any bearing on C compatibility. There's nothing about C compatibility that requires the ability to pass pointers into the memory managed by a particular value. If you want to pass a pointer into a C function that it needs to hold on to and dereference later, you're free to allocate your own memory with `UnsafeMutablePointer.alloc` and use that.
Swift doesn't have a garbage collector. And even the deprecated Obj-C garbage collector wasn't a compacting garbage collector, which means the notion of "fixed" doesn't even apply to the language (either Swift or Obj-C). |
Comment by Etan Kissling (JIRA) I really appreciate the insightful discussion going on here! Much better than the black hole that bugreport.apple.com was ^^ Indeed, your reference to the specification explaining how inout parameters should be assumed to work make this issue fall into the category of undefined behaviour. That said, I still want to find a solution to the original problem, that is, passing a pointer to a Swift object into a C API that survives the immediate function call. Even for the KVO pattern, when it's used like everyone uses it, it's broken. Because of copy-in copy-out behaviour, one has to assume that deinit { removeObserver(self, forKeyPath: "somePath", context: &kvoContext) }
init() { addObserver(self, forKeyPath: "somePath", options: [.New, .Old, .Initial], context: &kvoContext) }
override func observeValueForKeyPath(
keyPath: String?,
ofObject object: AnyObject?,
change: [String : AnyObject]?,
context: UnsafeMutablePointer<Void>
) {
guard &kvoContext == context else { return }
// Handle event.
} In the example of KVO, one could generate a random number into an instance variable, then unsafebitcast it to a void *, then pass that in as the context. This way, the context stays stable for the lifetime of the object – as opposed to the duration of the individual function calls. A better example of C APIs where such a trick cannot be applied is a selection of CoreFoundation APIs (e.g. the FSEvents API). struct CContext {
var version: CFIndex
var info: UnsafeMutablePointer<Void>
var retain: CFAllocatorRetainCallBack?
var release: CFAllocatorReleaseCallBack?
var copyDescription: CFAllocatorCopyDescriptionCallBack?
}
typealias CCallback = @convention(c) (obj: CObjRef, info: UnsafeMutablePointer<Void>, /* args */) -> Void
func CObjCreate(
allocator: CFAllocator?,
_ callback: CCallback,
_ context: UnsafeMutablePointer<CContext>,
/* args */
) -> CObjRef
func CCObjRelease(obj: CObjRef)
==> How can we safely get back to our Swift object from a One really hackish workaround could involve maintaining a Dictionary of all |
Comment by Etan Kissling (JIRA) BTW: There is something like C# fixed with I'll try building an example with |
Comment by Etan Kissling (JIRA) I've attached an example that uses |
You are correct in that the common usage of KVO is technically in violation. It is rather unfortunate. Also, Using Regarding your source example, I think you're doing more work than necessary. You've basically reinvented
(note: I haven't actually tried running this) |
I can confirm that at this time the only pointers guaranteed to be valid passed the lifetime of a call are global stored variables without observers. Class properties without accessors are "likely" to "work" but are not guaranteed to. Lazy properties almost certainly will not work, since they are implemented with accessors under the hood. Either |
Is this actually part of the language semantics? The Swift book doesn't list any exceptions to the rule saying that you should not rely on the difference between copy-in copy-out and call by reference. If we're ok with committing to global properties without accessors working like this, it would be great to have that documented (and it sounds like a good idea too, as that fixes the issue with the common practice of KVO).
How about |
Comment by Etan Kissling (JIRA) I've built a working solution based on import Cocoa
@NSApplicationMain
final class AppDelegate: NSObject, NSApplicationDelegate {
func applicationDidFinishLaunching(notification: NSNotification) {
_ = SwiftObj()
}
} final class Info<T: AnyObject> {
private(set) weak var object: T?
init(_ object: T) {
print("Info: init -- Swift object reference: \(object).")
self.object = object
}
deinit {
print("Info: deinit -- Swift object reference: \(object).")
}
} final class SwiftObj: CustomStringConvertible {
private typealias InfoType = Info<SwiftObj>
private var cFuncRef: CFuncRef!
init() {
print("SwiftObj: init.")
withExtendedLifetime(Info(self)) { (info: InfoType) in
print("SwiftObj: Creating context.")
var context = CFuncContext(
info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
retain: {
print("Callback: retain.")
return UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque())
},
release: {
print("Callback: release.");
Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release()
}
)
print("SwiftObj: Creating CFunc.")
cFuncRef = CFuncCreate(&context)
}
print("SwiftObj: init done.")
}
deinit {
print("SwiftObj: deinit.")
CFuncRelease(cFuncRef)
print("SwiftObj: deinit done.")
}
var description: String {
return "<SwiftObj: description>"
}
} producing this log: |
Bear in mind that Swift doesn't have any knowledge of concurrency or atomicity, so if you're accessing the object from two threads concurrently you have a problem. I hope that the actual access pattern here properly accesses it from one and only one thread at any given moment.
Strictly speaking, if you can guarantee that the
Good idea. Alternatively, you could create the
Ouch! If this isn't already documented somewhere, you should file a bug on it. Either the compiler should disallow it, or it should compile, it certainly shouldn't crash.
Since
I think the answer to that is "yes". The |
Comment by Etan Kissling (JIRA)
Yep, if there are multiple threads, proper synchronization is required e.g. via
Yeah, Are generic
You can write Seems like we have a stable solution now, though![]( Hopefully we can get official confirmation by @belkadan that this is indeed the way to go, so that the pattern can be pushed into the various code bases. Then, the crusade to update all Google results that distribute incorrect KVO pattern information can also begin) |
I just tested and I get an error if I change
It doesn't really, but the |
Comment by Etan Kissling (JIRA) About Try this class: final class Info<T: AnyObject> {
private(set) weak var object: T?
init(_ object: T) { self.object = object }
deinit { print("Info: deinit -- Swift object reference: \(object).") }
class func retain(info: UnsafePointer<Void>) -> UnsafePointer<Void> {
return UnsafePointer(Unmanaged<Info>.fromOpaque(COpaquePointer(info)).retain().toOpaque())
}
} This compiles although using |
That compiles because it's inside of the class definition, and references to a generic type from inside that type's definition typically assumes you want the same type parameters that |
Comment by Etan Kissling (JIRA) I see 🙂 Makes perfectly sense. So, the only thing missing is an official confirmation that the pattern posted above is okay, then. |
I really don't like the explicit calls to "Is the global variable thing actually part of the language semantics?" No, it is not—we don't actually have a formal memory model yet. (And we're not likely to have that discussion until we're ready to talk about concurrency.) That said, I can promise we won't break using a global stored variable (in the same module) as a KVO context until we have such a model, and most likely it won't break then either. We've recommended this pattern on the Apple developer forums before. |
Comment by Etan Kissling (JIRA) Thanks for the clarifications on the global variable behaviour 🙂 This means that we simply have to promote It's also very nice to hear that concurrency specification is somewhere on the roadmap! I'm a bit confused about your dislike for var context = CFuncContext(
info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
retain: { UnsafePointer(Unmanaged.passRetained(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeUnretainedValue()).toOpaque()) },
release: { _ = Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeRetainedValue() }
) instead of var context = CFuncContext(
info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
retain: { UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque()) },
release: { Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release() }
) If your comment relates to the actual callbacks in the C API: yeah, I agree that they are painful to use, but can't change those, unfortunately 🙁 Obviously, in the minimal example posted above where everything is single threaded and we know that the C API completes synchronously, we could simply link the lifetime of final class SwiftObj {
private var cFuncRef: CFuncRef!
init() {
var context = CFuncContext(
info: UnsafePointer(Unmanaged.passUnretained(self).toOpaque()),
retain: nil,
release: nil
)
cFuncRef = CFuncCreate(&context)
}
deinit {
CFuncRelease(cFuncRef)
}
} |
I guess you're right about FSEvents, but from what I've seen the common case for C APIs is merely a single destructor parameter, like pthread_key_create. In these cases having to manually do retain and release is more error-prone. I still get the feeling that your solution is way overcomplicated even for FSEvents. I'll come back and comment more on that later. |
If the API exposes a single destructor parameter, I agree that |
Okay, it took longer than I meant it to to come back to this. Here's what I came up with, deliberately avoiding the need for a box class:
|
Comment by Etan Kissling (JIRA) Taking longer is fine :-) The answer timing is still way better than I'm used to in general. You're doing a great job here![]( Thanks again for looking into this) A few comments about your proposal:
I've attached my proposal for the full FSEvents API usage (non-minified): FSEvents.zip Here are the relevant code parts: private final class Info<T: AnyObject> {
private(set) weak var object: T?
init(_ object: T) {
self.object = object
}
}
final class FileSystemWatcher {
private typealias InfoType = Info<FileSystemWatcher>
private let rootPath: String
private lazy var eventStream: FSEventStreamRef = {
return withExtendedLifetime(Info(self)) { (info: InfoType) in
var context = FSEventStreamContext(
version: 0,
info: UnsafeMutablePointer(Unmanaged.passUnretained(info).toOpaque()),
retain: { UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque()) },
release: { Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release() },
copyDescription: {
let object = Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeUnretainedValue().object
return Unmanaged<CFString>.passRetained(String(object))
}
)
return FSEventStreamCreate(
/* allocator: */ nil,
/* callback: */ { streamRef, clientCallBackInfo, numEvents, eventPaths, eventFlags, eventIds in
let info = Unmanaged<InfoType>.fromOpaque(COpaquePointer(clientCallBackInfo)).takeUnretainedValue()
guard let object = info.object else { return }
let paths = Unmanaged<CFArrayRef>.fromOpaque(COpaquePointer(eventPaths)).takeUnretainedValue()
as NSArray as! [CFStringRef] as! [String]
for i in 0 ..< numEvents {
object.eventStream(
streamRef,
didOutputEventForPath: paths[i],
flags: eventFlags[i],
id: eventIds[i]
)
}
},
/* context: */ &context,
/* pathsToWatch: */ [self.rootPath],
/* sinceWhen: */ FSEventStreamEventId(kFSEventStreamEventIdSinceNow),
/* latency: */ 0.01,
/* flags: */ FSEventStreamCreateFlags(0 | // The 0 | here is just to work around Xcode formatter bug.
kFSEventStreamCreateFlagUseCFTypes |
kFSEventStreamCreateFlagNoDefer |
kFSEventStreamCreateFlagWatchRoot |
kFSEventStreamCreateFlagIgnoreSelf |
kFSEventStreamCreateFlagFileEvents
)
)
}
}()
init(rootPath: String) {
self.rootPath = (rootPath as NSString).stringByStandardizingPath
FSEventStreamScheduleWithRunLoop(eventStream, CFRunLoopGetCurrent(), kCFRunLoopDefaultMode)
}
deinit {
stop()
FSEventStreamUnscheduleFromRunLoop(eventStream, CFRunLoopGetCurrent(), kCFRunLoopDefaultMode)
FSEventStreamInvalidate(eventStream)
FSEventStreamRelease(eventStream)
}
private var started = false
func start() {
guard !started else { return } // If you start multiple time, API misuse warning appears.
started = true
FSEventStreamStart(eventStream)
}
func stop() {
guard started else { return }
started = false
FSEventStreamStop(eventStream)
}
private func eventStream(
eventStream: ConstFSEventStreamRef,
didOutputEventForPath path: String,
flags: FSEventStreamEventFlags,
id: FSEventStreamEventId)
{
guard eventStream == self.eventStream else { return }
// Process event.
let allFlags = [
kFSEventStreamEventFlagNone: "None",
kFSEventStreamEventFlagMustScanSubDirs: "MustScanSubDirs",
kFSEventStreamEventFlagUserDropped: "UserDropped",
kFSEventStreamEventFlagKernelDropped: "KernelDropped",
kFSEventStreamEventFlagEventIdsWrapped: "EventIdsWrapped",
kFSEventStreamEventFlagHistoryDone: "HistoryDone",
kFSEventStreamEventFlagRootChanged: "RootChanged",
kFSEventStreamEventFlagMount: "Mount",
kFSEventStreamEventFlagUnmount: "Unmount",
kFSEventStreamEventFlagItemCreated: "ItemCreated",
kFSEventStreamEventFlagItemRemoved: "ItemRemoved",
kFSEventStreamEventFlagItemInodeMetaMod: "ItemInodeMetaMod",
kFSEventStreamEventFlagItemRenamed: "ItemRenamed",
kFSEventStreamEventFlagItemModified: "ItemModified",
kFSEventStreamEventFlagItemFinderInfoMod: "ItemFinderInfoMod",
kFSEventStreamEventFlagItemChangeOwner: "ItemChangeOwner",
kFSEventStreamEventFlagItemXattrMod: "ItemXattrMod",
kFSEventStreamEventFlagItemIsFile: "ItemIsFile",
kFSEventStreamEventFlagItemIsDir: "ItemIsDir",
kFSEventStreamEventFlagItemIsSymlink: "ItemIsSymlink"
].map { (FSEventStreamEventFlags($0.0), $0.1) }
print("========================================================================")
print("FSEvent ID: \(id)")
print(" - Path: \(path)")
print(" - Flags: \(allFlags.filter { flags & $0.0 != 0 }.map { $0.1 })")
let unknownFlags = allFlags.reduce(flags) { $0 & ~$1.0 }
if unknownFlags != 0 {
print(" - Unknown flags: \(unknownFlags))")
}
}
} |
Has this been resolved? If so, it should be closed. |
Attachment: Download
Environment
OS X 10.11.1 (15B42)
Xcode Version 7.1.1 (7B1005)
Swift version 2.1 (700.1.101.6 700.1.76)
Additional Detail from JIRA
md5: 45a7d69794999ab90b93c6c258f5fb14
Issue Description:
To understand this issue, a C API is necessary with two C functions. One to store a function and an info pointer - a second one to call the stored function with the stored pointer as argument.
A simple info object and C callback function are implemented in Swift to access this C API.
Now, a Swift object is defined.
On init(), an Info instance is created and registered with the C API.
On deinit, invoke CFuncCall is invoked. This should be fine, as the Swift object still holds a reference to the info object while deinit is in progress.
An instance of SwiftObj is created and immediately destroyed, producing this log:
The text was updated successfully, but these errors were encountered: