From ab7c66d3fbd7d62ff41ad9a437172fd78edf4a84 Mon Sep 17 00:00:00 2001 From: Petro Korienev Date: Sun, 22 Oct 2017 12:43:17 +0100 Subject: [PATCH 1/3] - Fixed integer overflows for Foundation extensions (#506) - Added overflow test case - Removed note on value limits for DispatchTimeInterval `*` operator - Updated CHANGELOG.md --- CHANGELOG.md | 1 + Sources/FoundationExtensions.swift | 30 ++++++++----------- .../FoundationExtensionsSpec.swift | 7 +++++ 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a38c001f..04a7cd663 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # master *Please add new entries at the top.* +Fixed integer overflow for `DispatchTimeInterval` in FoundationExtensions.swift (#506) # 3.0.0-alpha.1 # 3.0.0-alpha.1 diff --git a/Sources/FoundationExtensions.swift b/Sources/FoundationExtensions.swift index ba0c1b948..07d5a7a6c 100644 --- a/Sources/FoundationExtensions.swift +++ b/Sources/FoundationExtensions.swift @@ -93,7 +93,7 @@ extension DispatchTimeInterval { case let .milliseconds(ms): return TimeInterval(TimeInterval(ms) / 1000.0) case let .microseconds(us): - return TimeInterval(Int64(us) * Int64(NSEC_PER_USEC)) / TimeInterval(NSEC_PER_SEC) + return TimeInterval(Int64(us)) * TimeInterval(NSEC_PER_USEC) / TimeInterval(NSEC_PER_SEC) case let .nanoseconds(ns): return TimeInterval(ns) / TimeInterval(NSEC_PER_SEC) case .never: @@ -120,23 +120,19 @@ extension DispatchTimeInterval { /// Scales a time interval by the given scalar specified in `rhs`. /// - /// - note: This method is only used internally to "scale down" a time - /// interval. Specifically it's used only to scale intervals to 10% - /// of their original value for the default `leeway` parameter in - /// `Scheduler.schedule(after:action:)` schedule and similar - /// other methods. - /// - /// If seconds is over 200,000, 10% is ~2,000, and hence we end up - /// with a value of ~2,000,000,000. Not quite overflowing a signed - /// integer on 32-bit platforms, but close. - /// - /// Even still, 200,000 seconds should be a rarely (if ever) - /// specified interval for our APIs. And even then, folks should be - /// smart and specify their own `leeway` parameter. - /// - /// - returns: Scaled interval in microseconds + /// - returns: Scaled interval in minimal appropriate unit internal static func *(lhs: DispatchTimeInterval, rhs: Double) -> DispatchTimeInterval { let seconds = lhs.timeInterval * rhs - return .microseconds(Int(seconds * 1000 * 1000)) + var result: DispatchTimeInterval = .never + if let integerTimeInterval = Int(exactly: (seconds * 1000 * 1000 * 1000).rounded()) { + result = .nanoseconds(integerTimeInterval) + } else if let integerTimeInterval = Int(exactly: (seconds * 1000 * 1000).rounded()) { + result = .microseconds(integerTimeInterval) + } else if let integerTimeInterval = Int(exactly: (seconds * 1000).rounded()) { + result = .milliseconds(integerTimeInterval) + } else if let integerTimeInterval = Int(exactly: (seconds).rounded()) { + result = .seconds(integerTimeInterval) + } + return result } } diff --git a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift index 3546974d8..f05cb6161 100644 --- a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift +++ b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift @@ -94,6 +94,13 @@ class FoundationExtensionsSpec: QuickSpec { expect((DispatchTimeInterval.seconds(5) * 0.5).timeInterval).to(beCloseTo(DispatchTimeInterval.milliseconds(2500).timeInterval)) expect((DispatchTimeInterval.seconds(1) * 0.25).timeInterval).to(beCloseTo(DispatchTimeInterval.milliseconds(250).timeInterval)) } + + it("should not introduce integer overflow upon scale") { + expect((DispatchTimeInterval.seconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.milliseconds(Int.max).timeInterval, within: 1)) + expect((DispatchTimeInterval.milliseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.microseconds(Int.max).timeInterval, within: 1)) + expect((DispatchTimeInterval.microseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.nanoseconds(Int.max).timeInterval, within: 1)) + expect(DispatchTimeInterval.seconds(Int.max) * 10).to(equal(DispatchTimeInterval.never)) + } it("should produce the expected TimeInterval values") { expect(DispatchTimeInterval.seconds(1).timeInterval).to(beCloseTo(1.0)) From 0ead35c7b431b72c6bdb3b60b5def7624f72e0ae Mon Sep 17 00:00:00 2001 From: Petro Korienev Date: Sun, 22 Oct 2017 13:13:58 +0100 Subject: [PATCH 2/3] - Fixed linux build https://travis-ci.org/ReactiveCocoa/ReactiveSwift/jobs/291102863 - Update CHANGELOG.md --- CHANGELOG.md | 2 +- Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04a7cd663..177fbbfeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # master *Please add new entries at the top.* -Fixed integer overflow for `DispatchTimeInterval` in FoundationExtensions.swift (#506) +1. Fixed integer overflow for `DispatchTimeInterval` in FoundationExtensions.swift (#506) # 3.0.0-alpha.1 # 3.0.0-alpha.1 diff --git a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift index f05cb6161..7a240d430 100644 --- a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift +++ b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift @@ -99,7 +99,7 @@ class FoundationExtensionsSpec: QuickSpec { expect((DispatchTimeInterval.seconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.milliseconds(Int.max).timeInterval, within: 1)) expect((DispatchTimeInterval.milliseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.microseconds(Int.max).timeInterval, within: 1)) expect((DispatchTimeInterval.microseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.nanoseconds(Int.max).timeInterval, within: 1)) - expect(DispatchTimeInterval.seconds(Int.max) * 10).to(equal(DispatchTimeInterval.never)) + expect((DispatchTimeInterval.seconds(Int.max) * 10).timeInterval).to(equal(Double.infinity)) } it("should produce the expected TimeInterval values") { @@ -110,7 +110,7 @@ class FoundationExtensionsSpec: QuickSpec { expect(DispatchTimeInterval.milliseconds(500).timeInterval).to(beCloseTo(0.5)) expect(DispatchTimeInterval.milliseconds(250).timeInterval).to(beCloseTo(0.25)) - expect(DispatchTimeInterval.never.timeInterval) == Double.infinity + expect(DispatchTimeInterval.never.timeInterval).to(equal(Double.infinity)) } it("should negate as you'd hope") { @@ -118,7 +118,7 @@ class FoundationExtensionsSpec: QuickSpec { expect((-DispatchTimeInterval.milliseconds(1)).timeInterval).to(beCloseTo(-0.001)) expect((-DispatchTimeInterval.microseconds(1)).timeInterval).to(beCloseTo(-0.000001, within: 0.0000001)) expect((-DispatchTimeInterval.nanoseconds(1)).timeInterval).to(beCloseTo(-0.000000001, within: 0.0000000001)) - expect((-DispatchTimeInterval.never).timeInterval) == Double.infinity + expect((-DispatchTimeInterval.never).timeInterval).to(equal(Double.infinity)) } } } From 17fac4980c18743cf333fef24e5eb881b0c7d418 Mon Sep 17 00:00:00 2001 From: Petro Korienev Date: Mon, 23 Oct 2017 09:00:21 +0100 Subject: [PATCH 3/3] Bringing back `==` syntax for equality matchers in FoundationExtensionsSpec.swift --- Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift index 7a240d430..c8ed66a8c 100644 --- a/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift +++ b/Tests/ReactiveSwiftTests/FoundationExtensionsSpec.swift @@ -99,7 +99,7 @@ class FoundationExtensionsSpec: QuickSpec { expect((DispatchTimeInterval.seconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.milliseconds(Int.max).timeInterval, within: 1)) expect((DispatchTimeInterval.milliseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.microseconds(Int.max).timeInterval, within: 1)) expect((DispatchTimeInterval.microseconds(Int.max) * 0.01).timeInterval).to(beCloseTo(10 * DispatchTimeInterval.nanoseconds(Int.max).timeInterval, within: 1)) - expect((DispatchTimeInterval.seconds(Int.max) * 10).timeInterval).to(equal(Double.infinity)) + expect((DispatchTimeInterval.seconds(Int.max) * 10).timeInterval) == Double.infinity } it("should produce the expected TimeInterval values") { @@ -110,7 +110,7 @@ class FoundationExtensionsSpec: QuickSpec { expect(DispatchTimeInterval.milliseconds(500).timeInterval).to(beCloseTo(0.5)) expect(DispatchTimeInterval.milliseconds(250).timeInterval).to(beCloseTo(0.25)) - expect(DispatchTimeInterval.never.timeInterval).to(equal(Double.infinity)) + expect(DispatchTimeInterval.never.timeInterval) == Double.infinity } it("should negate as you'd hope") { @@ -118,7 +118,7 @@ class FoundationExtensionsSpec: QuickSpec { expect((-DispatchTimeInterval.milliseconds(1)).timeInterval).to(beCloseTo(-0.001)) expect((-DispatchTimeInterval.microseconds(1)).timeInterval).to(beCloseTo(-0.000001, within: 0.0000001)) expect((-DispatchTimeInterval.nanoseconds(1)).timeInterval).to(beCloseTo(-0.000000001, within: 0.0000000001)) - expect((-DispatchTimeInterval.never).timeInterval).to(equal(Double.infinity)) + expect((-DispatchTimeInterval.never).timeInterval) == Double.infinity } } }