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-14164] Unexpected crash while using Array(unsafeUninitializedCapacity:) #56543

Open
cltnschlosser opened this issue Feb 7, 2021 · 2 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella

Comments

@cltnschlosser
Copy link
Collaborator

Previous ID SR-14164
Radar rdar://problem/74108115
Original Reporter @cltnschlosser
Type Bug
Environment

Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28)
Target: x86_64-apple-darwin20.3.0

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

md5: bd27733c794bd22f1e3fa53e590d95bd

Issue Description:

While trying to make a small improvement to SwiftLint by using https://github.com/apple/swift-evolution/blob/master/proposals/0223-array-uninitialized-initializer.md I came across an unexpected and interesting edge case.
This is the code that the evolution proposal uses for it's simple example:

var myArray = Array<Int>(unsafeUninitializedCapacity: 10) { buffer, initializedCount in
    for x in 1..<5 {
        buffer[x] = x
    }
    buffer[0] = 10
    initializedCount = 5
}
// myArray == [10, 1, 2, 3, 4]

On it's own this code is fine. What ends up not being fine is that when the Array.Element type is a class (or an unspecialized generic) `buffer[x] = x` crashes. It seems like it's doing an assignWithTake to release the old value, which makes sense for a class, but not for uninitialized data. I'm not entirely sure how the compiler would fix this, because there is nothing preventing you from doing something like:

buffer[0] = TestClass()
buffer[0] = TestClass()

and for the second write at an index the compiler would need to release for original instance.

This is an unsafe initializer after all, so if it can't be fixed it should at least be documented.
The documentation for this function is a bit light, doesn't contain any examples, so people are likely to go to the evolution proposal and try the first example. It would be better if the standard library documented the danger with using the subscript and instead directed people towards something like:

(buffer.baseAddress! + index).initialize(to: value)

SwiftLint Original PR here: realm/SwiftLint#3515

Reproduction code:

extension Array {
    func myMap<T>(transform: (Element) -> T) -> [T] {
        return Array<T>(unsafeUninitializedCapacity: count) { buffer, initializedCount in
            for index in indices {
                buffer[index] = transform(self[index])
            }
        }
    }
}

var array = [0, 1, 2, 3]

array.myMap { $0 + 10 }

final class TestClass {
    let index: Int
    
    init(index: Int) {
        self.index = index
    }
}

struct TestStruct {
    let index: Int
}

array.myMap { TestStruct(index: $0) }

array.myMap { TestClass(index: $0) } // This line crashes
@typesanitizer
Copy link

@swift-ci create

@lorentey
Copy link
Member

The underlying issue here is that `UnsafeMutableBufferPointer` conforms to `Collection`, even though it doesn't guarantee that the underlying memory is in an initialized state. (Indeed, in the case of the `unsafeUninitialized` initializer, the opposite is true.)

I agree with Colton's assessment above – while we cannot remove the Collection conformance at this point, but the standard library ought to do a better job of documenting the circumstances of when it is safe to use collection APIs on `UnsafeMutableBufferPointer`. This just needs a PR to update the API docs in the repo.

We should also add convenience methods to directly initialize/deinitialize specific slots in an `UnsafeMutableBufferPointer` value, without having to go through `UnsafeMutablePointer` and losing debug-mode bounds checks. (This will require a Swift Evolution pitch.)

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. standard library Area: Standard library umbrella
Projects
None yet
Development

No branches or pull requests

3 participants