[Swift language feature] Implement Swift.Array support #2964
Conversation
kotlarmilos
requested review from
vitek-karas,
mcumming,
matouskozak and
jkurdek
| /// </summary> | ||
| unsafe SwiftArray(SwiftHandle handle) | ||
| { | ||
| this.buffer = *(ArrayBuffer*)(handle); |
There was a problem hiding this comment.
Should this just copy the memory? What about arrays of reference types?
There was a problem hiding this comment.
I would remove this constructor as it goes against the expected behavior on Swift side. Even if payload value is copied, it would point to the same instance.
There was a problem hiding this comment.
Can't be removed, it is used in the ISwiftObject.NewFromPayload. It should perform a shallow copy of the ArrayBuffer struct.
Anyway, it doesn't have the same semantics as in Swift.
There was a problem hiding this comment.
Ok, maybe this is a future problem once we start supporting reference types. We might need to call InitializeWithCopy here
There was a problem hiding this comment.
maybe this is a future problem once we start supporting reference types
Could you provide an example?
We might need to call InitializeWithCopy here
Added to #2930.
There was a problem hiding this comment.
I don't have anything specific in my mind yet. I was think about having an array of reference types. If we just make a shallow copy of the array the ref count of those references will not be updated.
…melab into swift-bindings/swift-array
| static ProtocolConformanceDescriptor ISwiftObject.GetProtocolConformanceDescriptor<TProtocol>() | ||
| where TProtocol : class | ||
| { | ||
| return ProtocolConformanceDescriptor.Zero; |
There was a problem hiding this comment.
This will have to be able to obtain the descriptor for array : Collection
| { | ||
| if (_buffer.storage != IntPtr.Zero) | ||
| { | ||
| Arc.Release(*(IntPtr*)_buffer.storage); |
There was a problem hiding this comment.
Did you have any discussions about thread-safety and security of the Swift projections?
For example, with this implementation - user code calling Dispose method on multiple threads can lead to releasing the _buffer.storage twice (double-free security bug). .NET core libraries typically try to avoid turning race conditions in user code into memory safety violations. Are we ok with Swift projections not providing this hardening?
There was a problem hiding this comment.
Great point - created #2975 to track this to revisit all projections and fix it there (if we have any other really). In any case which is something we might want to have a helper for.
There was a problem hiding this comment.
Thanks @vitek-karas for creating the tracking issues. We need to revisit this.
On the same note, we've already encountered a multi-threading scenario — in-app purchase request can't be performed on the main thread obviously.
| { | ||
| fixed (void* _payloadPtr = &_buffer) | ||
| { | ||
| metadata.ValueWitnessTable->InitializeWithCopy((void*)swiftDest, (void*)_payloadPtr, metadata); |
There was a problem hiding this comment.
General comment about current design of MarshalToSwift - this feels very prone to buffer overruns.
It's a virtual call, so the caller technically doesn't know what the callee will be precisely. But at the same time, the caller MUST now the size of the buffer to allocate for swiftDest. That feels wrong.
What is the scenario where we have a preexisting buffer and need to marshal to it? So far the callers I've seen all need to allocate before calling this method. And they have to get the size right, otherwise we'll get buffer overruns at runtime.
I think the minimum should be that we don't pass a raw pointer, but some representation of pointer + size. Or better yet, this method would be responsible for allocating the buffer and returning it.
I also find it weird that the method takes a destination buffer, but it may choose to return another buffer. What is the memory ownership? We should at least precisely document this in comments, but ideally redesign the API to make it "secure by default".
There was a problem hiding this comment.
Maybe we could use MemoryManager<byte> (our own implementation) which would handle the memory ownership. But it's also possible that having a simple struct of our own would be good enough.
There was a problem hiding this comment.
I agree that we should review the MarshalToSwift design. I would not tackle this in this PR though.
There was a problem hiding this comment.
Absolutely - created #2974 to track that.
kotlarmilos
changed the title
Description
This PR is a follow-up to #2948. It introduces support for
Swift.Array.In Swift, Array is defined as
@frozen struct Array<Element>. It is projected into a SwiftArray class with a helper structArrayBuffer, which is used at the ABI level to ensure correct lowering and PInvoke signatures.Fixes #2833