Fix crash with DateInterval.
#20106
Fix crash with DateInterval.
#20106
Conversation
Steps to repro: ``` DateInterval(start: Date(timeIntervalSince1970: 0.0), end: Date(timeIntervalSince1970: 0.0)).hashValue ```
|
@swift-ci please test |
|
@kzaher Thanks for the patch. Unfortunately, only certain people can trigger the CI :-) |
|
CC: @moiseev |
|
@swift-ci please test |
|
Build failed |
|
Thanks, @kzaher! /cc @phausler and @millenomi |
| return withUnsafeMutablePointer(to: &buf) { | ||
| $0.withMemoryRebound(to: UInt8.self, capacity: 2 * MemoryLayout<UInt>.size / MemoryLayout<UInt8>.size) { | ||
| return Int(bitPattern: CFHashBytes($0, CFIndex(MemoryLayout<UInt>.size * 2))) | ||
| $0.withMemoryRebound(to: UInt8.self, capacity: 2 * MemoryLayout<TimeInterval>.size / MemoryLayout<UInt8>.size) { |
There was a problem hiding this comment.
I don't believe this is a sound usage of withMemoryRebound(to:capacity:) (and neither was original code's usage) – the method requires that you rebind to a type that has the same size and alignment as the current Pointee type (e.g UInt to Int). The reasoning for this is because it uses the capacity you pass in order to bind the memory back to the original type, which in this case would result in too many instances being re-bound back to TimeInterval.
One alternative is to use withUnsafeBytes(of:), for example (untested):
return withUnsafeBytes(of: &buf) { buffer in
Int(bitPattern: CFHashBytes(Array(buffer), buffer.count))
}There was a problem hiding this comment.
Array would not be ideal since that will deallocate after passing in, I would suggest to either bit cast since we are going directly to C or use , and also the internal function CFHashBytes takes a uint8_t * not a const uint8_t * because of "historical reasons"... however it does not mutate the pointer in any way so it is safe to cast to a mutable type.
There was a problem hiding this comment.
this would be reasonable:
return withUnsafeMutableBytes(of: &buf) {
return CFHashBytes($0.baseAddress?.assumingMemoryBound(to: UInt8.self), $0.count)
}
There was a problem hiding this comment.
The array-to-pointer conversion should keep the Array valid for the duration of the call to CFHashBytes, so assuming CFHashBytes doesn't escape the pointer, there should be no problem there.
And ah, okay, if CFHashBytes requires a mutable pointer, you could do:
return withUnsafeBytes(of: &buf) { buffer in
var array = Array(buffer)
return Int(bitPattern: CFHashBytes(&array, buffer.count))
}Edit: Actually, yeah your suggestion to use assumingMemoryBound(to:) is better if the pointer is going straight to C.
There was a problem hiding this comment.
So if I understand correctly:
- I should change it to:
return withUnsafeMutableBytes(of: &buf) {
return CFHashBytes($0.baseAddress?.assumingMemoryBound(to: UInt8.self), $0.count)
}
- Change https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/DateInterval.swift#L153 also.
- Add unit test.
Is this correct?
There was a problem hiding this comment.
Ok, I'll try to do these changes tomorrow. Can somebody point me where should I add those tests?
I was only be able to found Date tests.
https://github.com/apple/swift-corelibs-foundation/blob/master/TestFoundation/TestDate.swift
Are there any DateInterval tests, am I missing something?
There was a problem hiding this comment.
https://github.com/apple/swift/blob/master/test/stdlib/TestCalendar.swift has a few DateInterval related test cases.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm a bit confused. I'm not sure why there are tests in both apple/swift and apple/swift-corelibs-foundation. What determines does it go into one or the other?
Where should these tests live? I'm assuming that I should only call hashValue for some old dateinterval and make sure that it doesn't crash.
There was a problem hiding this comment.
On Darwin, Swift depends on platform Foundation. Tests for the overlay that target Darwin should go in apple/swift as a result.
On non-Darwin, swift-corelibs-foundation depends on Swift. Tests for swift-corelibs-foundation outside of Darwin should go in apple/swift-corelibs-foundation.
|
I'd like to bump this issue, storing and handling dates and date intervals should be imperturbable core functionality. Is there a better workaround than (1) building my own |
|
The new Closing this out. Thank you for the change @kzaher! |
|
Fot anyone passing through, I've filed feedback FB7405252 with Apple in October 2019, they've now responded that I should recheck with macOS 10.15.4 and my original testing playground with dates before reference date (e.g. 01/01/1981) no longer crashes now. Apparently, this is now in fact fixed and resolved. |
Fixes crash with DateInterval.
Steps to repro:
@swift-ci Please smoke test