Repository navigation
Reduce and isolate unsafe code in TLS record layer - #19
Conversation
This PR reduces and isolates the unsafe code introduced in apple#18 to a minimum. This helps with ensuring that the TLS record layer implementation is as safe as possible.
68c8f4a to
5150b98
Compare
| } | ||
| initializedCount = bytes.byteCount | ||
| self.init(capacity: bytes.byteCount) { output in | ||
| output.append(contentsOf: bytes) |
There was a problem hiding this comment.
Since append still has an unsafe operation and iterates in a for loop, is it worth changing this? Versus the single unsafe memcpy here?
There was a problem hiding this comment.
Or could we change the append implementation to avoid a loop?
claude's suggestion is this, but I haven't checked if it works:
@available(SwiftTLS 0.1.0, *)
extension OutputSpan where Element == UInt8 {
mutating func append(contentsOf bytes: RawSpan) {
guard !bytes.isEmpty else { return }
append(contentsOf: bytes.unsafeView(as: UInt8.self)) // Span<UInt8>, bulk overload
}
}
There was a problem hiding this comment.
The suggestion doesn't work because unsafeView is not real public API and there are no other bulk operation methods on the spans right now. I did some measurement and on release builds the loop is fully vectorized and equal in performance to a memcpy. However, in debug builds it is significantly slower. Now while we don't optimize for debug build performance I think this here is important enough to also ensure it is fast in debug builds. I pushed a commit that switches back to memcpy
There was a problem hiding this comment.
I did some measurement
If there are specific use cases that would make sense to model, we could make benchmarking reproducible with https://github.com/ordo-one/benchmark
There was a problem hiding this comment.
Yeah we should totally setup benchmarking here. I have another follow up PR that I would like to open at some point that allows us to produce record views with ~Escapable types to avoid copying where possible.
baumanl
left a comment
baumanl
left a comment
There was a problem hiding this comment.
LGTM, left a few comments but not blocking.
| // place. The tag is copied into stack storage so that it doesn't | ||
| // overlap the buffer being decrypted. | ||
| var message = [UInt8](copying: encryptedRecord.extracting(0..<ciphertextLength)) | ||
| let tag = [16 of UInt8](copying: encryptedRecord.extracting(ciphertextLength..<encryptedRecord.byteCount)) |
There was a problem hiding this comment.
Can we typealias [16 of UInt8] to make this more readable?
4 participants
This PR reduces and isolates the unsafe code introduced in #18 to a minimum. This helps with ensuring that the TLS record layer implementation is as safe as possible.