[overlays] Clean up some sources of undefined behaviour #18799
Conversation
While experimenting with forbidding temporary argument conversions for `UnsafePointer` initialiser parameters (amongst others), I found a few cases where we're currently making such unsound calls in the SDK overlays. This commit removes such usages by performing the temporary argument conversions directly without going through any `UnsafePointer` initialiser calls first. In the case of `MTLCopyAllDevicesWithObserver`, we were even over-releasing the observer through the usage of: ```swift AutoreleasingUnsafeMutablePointer<NSObjectProtocol?>(&resultTuple.observer) ``` As we're treating +1 memory as having +0 semantics (this didn't appear to cause issues for users though as Metal itself appears to keep the observer retained).
| let devices = __MTLCopyAllDevicesWithObserver(&observer, handler) | ||
| // FIX-ME: The force cast here isn't great – ideally we would return the | ||
| // observer as an NSObjectProtocol. | ||
| return (devices, observer as! NSObject) |
There was a problem hiding this comment.
As said in the comment, the force cast here isn't ideal, but it's better than what the code was previously doing – which was type punning an NSObject pointer to a NSObjectProtocol? pointer.
There was a problem hiding this comment.
Also, can I confirm that __MTLCopyAllDevicesWithObserver is guaranteed to set observer? The previous code was initially assigning NSObject(), but I'm not sure whether that was specifically because it could actually return that as the observer.
There was a problem hiding this comment.
Pinged Metal team about that.
There was a problem hiding this comment.
Here is the reply:
MTLCopyAllDevicesWithObserver(id <NSObject>* observer, MTLDeviceNotificationHandler handler)allocates a framework object to return via the observer pointer (specifically a_MTLDeviceNotifierobject), so the pointer passed in can be set tonullptr.
airspeedswift
changed the title
|
Ouch.. This is the first time I see this PR. Taking a look now. |
stdlib/public/SDK/Metal/Metal.swift
Outdated
| return resultTuple | ||
| var observer: NSObjectProtocol? | ||
| let devices = __MTLCopyAllDevicesWithObserver(&observer, handler) | ||
| // FIX-ME: The force cast here isn't great – ideally we would return the |
There was a problem hiding this comment.
Nitpick: the - in fixme makes it undiscoverable. All other FIXME's in the codebase don't have it.
There was a problem hiding this comment.
Good spot, thanks!
|
@swift-ci Please test macOS platform |
|
@swift-ci Please smoke test Linux platform |
|
@swift-ci Please test macOS platform |
|
@swift-ci Please smoke test Linux platform |
|
Build failed |
|
Thanks Max! I have filed a bug over the observer being returned as an |
While experimenting with forbidding temporary argument conversions for
UnsafePointerinitialiser parameters (amongst others), I found a few cases where we're currently making such unsound calls in the SDK overlays (unsound because such temporary argument conversions produce pointers valid only for the duration of the call).This PR removes such usages by performing the temporary argument conversions directly without going through any
UnsafePointerinitialiser calls first.In the case of
MTLCopyAllDevicesWithObserver, such usage led to us over-releasing the observer, as the call toAutoreleasingUnsafeMutablePointer's initialiser:treats the +1 memory as having +0 semantics, leading to an unbalanced autorelease (this didn't appear to cause issues for users though as Metal itself keeps the observer retained).