Add .noOpRetain() to all elided types
- Dominant language
- Swift
- Stars
- 0
- Forks
- 2
- PR merge metrics
- No merged PRs in 30d
Description
Swift is very eager to deallocate objects, even before their last use. For example, take this code block in Listen.swift:
```swift
/// Notifies the listener that a block was added at the given height.
public override func blockConnected(block: [UInt8], height: UInt32) {
// native call variable prep
let blockPrimitiveWrapper = u8slice(value: block)
// native method call
let nativeCallResult = self.cType!.block_connected(self.cType!.this_arg, blockPrimitiveWrapper.cType!, height)
// cleanup
// for elided types, we need this
blockPrimitiveWrapper.noOpRetain()
// return value (do some wrapping)
let returnValue = nativeCallResult
return returnValue
}
```
Here, `blockPrimitiveWrapper` gets deinit before the `let nativeCallResult = self.cType!.block_connected` call. Presumably Swift simply store the value of `blockPrimitiveWrapper.cType` behind the scenes, and then just passes that. Because a deinit cleans up the contents, it invalidates the cType, and we have an access of deallocated memory.
This is only a concern when we instantiate the wrapper inside the method, i. e. it's a concern for all elided types. However, if we then take a look at this method:
```swift
public class func swiftFindRoute(ourNodePubkey: [UInt8], routeParams: RouteParameters, networkGraph: NetworkGraph, firstHops: [ChannelDetails]?, logger: Logger, scorer: Score, randomSeedBytes: [UInt8]) -> Result_RouteLightningErrorZ {
// native call variable prep
let ourNodePubkeyPrimitiveWrapper = PublicKey(value: ourNodePubkey)
var firstHopsVectorPointer: UnsafeMutablePointer? = nil
if let firstHops = firstHops {
let firstHopsVector = Vec_ChannelDetailsZ(array: firstHops).dangle()
firstHopsVectorPointer = UnsafeMutablePointer.allocate(capacity: 1)
firstHopsVectorPointer!.initialize(to: firstHopsVector.cType!)
}
let tupledRandomSeedBytes = Bindings.arrayToUInt8Tuple32(array: randomSeedBytes)
// native method call
let nativeCallResult =
withUnsafePointer(to: routeParams.dynamicallyDangledClone().cType!) { (routeParamsPointer: UnsafePointer) in
withUnsafePointer(to: networkGraph.dangle().cType!) { (networkGraphPointer: UnsafePointer) in
withUnsafePointer(to: scorer.activate().cType!) { (scorerPointer: UnsafePointer) in
withUnsafePointer(to: tupledRandomSeedBytes) { (tupledRandomSeedBytesPointer: UnsafePointer) in
find_route(ourNodePubkeyPrimitiveWrapper.cType!, routeParamsPointer, networkGraphPointer, firstHopsVectorPointer, logger.activate().cType!, scorerPointer, tupledRandomSeedBytesPointer)
}
}
}
}
// cleanup
// for elided types, we need this
ourNodePubkeyPrimitiveWrapper.noOpRetain()
firstHopsVector.noOpRetain()
// return value (do some wrapping)
let returnValue = Result_RouteLightningErrorZ(cType: nativeCallResult)
return returnValue
}
```
We find that we're trying to do a noOpRetain on `firstHopsVector`, which of course we should be doing, but we can't, because its argument was an optional, and it therefore only exists inside the `if let` block. Additionally, it doesn't really make a difference because the object was dangled prior to being passed, so whether or not Swift calls `deinit`, nothing actually happens to the underlying C value, and we're safe.
However, for the purposes of future-proofing, or if we should choose not to dangle the object, we should instead be able to do a `noOpRetain` on that object, as well. The same applies to tuples, of course.
As of today, it just so happens that we don't have any primitive wrappers being passed as optionals, so we can punt on that for the time being, but we should fix that in the medium term.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the generated code shown in Listen.swift and the swiftFindRoute entry point. Trace how cleanup is emitted for locally created wrappers, optional values, and tuples, then verify that each required noOpRetain remains valid at the native call boundary. Done means all elided types receive safe retention handling without referencing values outside their scope.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100