Fix overflow traps in DispatchTime/DispatchWallTime/DispatchTimeInterval #11927

ktopley-apple · 2017-09-14T20:16:27Z

Fixes overflows in DispatchTime/DispatchWallTime/DispatchTimeInterval.

Examples:

DispatchTime.now() + Date.distantFuture.timeIntervalSinceNow // traps
DispatchTime.now() + Date.distantPast.timeIntervalSinceNow. // traps
let t = DispatchTimeInterval.seconds(Int.max)
t == t // Traps due to conversion to nanoseconds.

Also added tests for these and similar cases.

ktopley-apple · 2017-09-14T20:16:54Z

@swift-ci please smoke test

ktopley-apple · 2017-09-14T20:26:02Z

@swift-ci please smoke test

jrose-apple

I'm not sure why these properties are desirable. You're going to say that DispatchTimeInterval.seconds(Int.max) == .milliseconds(Int.max), which is silly. And the other traps do seem like they should trap.

jrose-apple · 2017-09-14T20:35:47Z

stdlib/public/SDK/Dispatch/Time.swift

+	if m1 == 0 { return Int64(0) }
+	if m1 > Int64.max/m2 { return Int64.max }
+	if m1 < Int64.min/m2 { return Int64.min }
+	return m1 * m2;


You could use multiplyWithOverflow for this instead…

Do you mean multipliedReportingOverflow()? I didn't know about that :-)

Regarding the change overall, the general rule in libdispatch is that operations on dispatch_time_t that overflow are clamped to DISPATCH_TIME_FOREVER. The intent of this change is to make the same true in Swift. For the change to DispatchTimeInterval, you are right that it might seem odd that DispatchTimeInterval.seconds(Int.max) == .milliseconds(Int.max), but DispatchTimeInterval is used as an offset to form a DispatchTime or DispatchWallTime and the result of using either of these values is going to be clamped to Dispatch[Wall]Time.distantFuture. Currently, you can't say "DispatchTime.now() + DispatchTimeInterval.seconds(Int.max)" because it traps. Of course Int.max is a pathological example - in reality, there have been cases where large values have been used that have caused traps and this had to be worked around by checking the value before passing it to dispatch and using DispatchTime.distantFuture instead. I probably should add some comments to DispatchTimeInterval that explain this behavior.

jrose-apple · 2017-09-14T20:36:18Z

stdlib/public/SDK/Dispatch/Time.swift

+// Returns m1 * m2, clamped to the range [Int64.min, Int64.max].
+// Because of the way this function is used, we can always assume
+// that m2 > 0.
+private func clampedInt64Product(_ m1: Double, _ m2: Double) -> Int64 {


…and I'm not sure why this isn't just a transformation on a single Double.

Can you explain what you mean by that?

It looks like it's equivalent to toInt64Clamped(m1 * m2).

ktopley-apple · 2017-09-14T22:43:09Z

@swift-ci please smoke test

ktopley-apple · 2017-09-14T22:43:42Z

Updated based on Jordan's comments.

ktopley-apple · 2017-09-14T22:57:29Z

@swift-ci please smoke test

jrose-apple · 2017-09-15T00:37:06Z

stdlib/public/SDK/Dispatch/Time.swift

+private func toInt64Clamped(_ value: Double) -> Int64 {
+	if value.isNaN { return Int64.max }
+	if value > Double(Int64.max) { return Int64.max }
+	if value < Double(Int64.min) { return Int64.min }


Sorry, I realized this check still isn't quite correct because Double doesn't have enough precision to represent Int64.max. One way to do it would be to check against Double(bitPattern: Double(Int64.max).bitPattern-1) instead, but that's black magic. @stephentyrone, what's the right way to write this?

I agree it's not 100% correct, but it I think it is the closest I can get without "black magic". Maybe a version of this that can use the black magic should be added to the standard library. Anyway, it should be OK as it stands because we don't need to be completely precise - we just need to catch times that are far enough in the future that we should treat them as "forever".

We should add a clamping init from Int to Double, but that's outside the scope of this change. As a temporary hack, it suffices to make this if value >= Double(Int64.max); that does the right thing without black magic.

@jrose-apple More generally, instead of Double(bitPattern: x.bitPattern-1) use x.nextDown if you know x is positive.

Oh, I was looking for nextDown and didn't see it. Thanks, Steve.

ktopley-apple · 2017-09-15T15:12:39Z

@swift-ci please smoke test

ktopley-apple · 2017-09-15T16:59:39Z

@swift-ci please smoke test

ktopley-apple · 2017-09-18T15:26:20Z

@swift-ci please smoke test

…val. rdar://problem/32678302

ktopley-apple · 2017-09-20T19:17:06Z

Squashed commits.

ktopley-apple · 2017-09-20T19:17:19Z

@swift-ci please smoke test

jrose-apple · 2017-10-02T17:49:41Z

Looks like we've still got a problem here on a 32-bit iOS device.

02:22:19 [ RUN      ] DispatchAPI.DispatchTime.addSubtract
02:22:19 stderr>>> CRASHED: SIGTRAP
02:22:19 the test crashed unexpectedly
02:22:19 [     FAIL ] DispatchAPI.DispatchTime.addSubtract

Let me know if you want more information.

jrose-apple · 2017-10-02T17:51:09Z

(This is rdar://problem/34751238, by the way.)

ktopley-apple self-assigned this Sep 14, 2017

ktopley-apple requested review from das, moiseev and jrose-apple September 14, 2017 20:16

jrose-apple reviewed Sep 14, 2017

View reviewed changes

jrose-apple reviewed Sep 15, 2017

View reviewed changes

ktopley-apple mentioned this pull request Sep 15, 2017

Fix overflow traps in DispatchTime/DispatchWallTime/DispatchTimeInterval swiftlang/swift-corelibs-libdispatch#301

Merged

daagaak approved these changes Sep 20, 2017

View reviewed changes

moiseev approved these changes Sep 20, 2017

View reviewed changes

mwwa approved these changes Sep 20, 2017

View reviewed changes

Fix overflow traps in DispatchTime/DispatchWallTime/DispatchTimeInter…

c5af279

…val. rdar://problem/32678302

ktopley-apple merged commit 0dc6201 into swiftlang:master Sep 20, 2017

Fix overflow traps in DispatchTime/DispatchWallTime/DispatchTimeInterval #11927

Fix overflow traps in DispatchTime/DispatchWallTime/DispatchTimeInterval #11927

Uh oh!

Conversation

ktopley-apple commented Sep 14, 2017

Uh oh!

ktopley-apple commented Sep 14, 2017

Uh oh!

ktopley-apple commented Sep 14, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktopley-apple commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktopley-apple commented Sep 14, 2017

Uh oh!

ktopley-apple commented Sep 14, 2017

Uh oh!

jrose-apple Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktopley-apple commented Sep 15, 2017

Uh oh!

ktopley-apple commented Sep 15, 2017

Uh oh!

ktopley-apple commented Sep 18, 2017

Uh oh!

ktopley-apple commented Sep 20, 2017

Uh oh!

ktopley-apple commented Sep 20, 2017

Uh oh!

jrose-apple commented Oct 2, 2017

Uh oh!

jrose-apple commented Oct 2, 2017

Uh oh!

Uh oh!

ktopley-apple commented Sep 14, 2017 •

edited

Loading

jrose-apple Sep 15, 2017 •

edited

Loading