-
Notifications
You must be signed in to change notification settings - Fork 35
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
Fix a division-by-zero crash in the VPN code #823
Fix a division-by-zero crash in the VPN code #823
Conversation
rx = Double(newer.rxBytes - older.rxBytes) / deltaSeconds | ||
tx = Double(newer.txBytes - older.txBytes) / deltaSeconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want to divide by zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably that should work, unless you do a horse kick test 😀
let d = Date()
let (rx, tx) = NetworkProtectionConnectionBandwidthAnalyzer.bytesPerSecond(newer: .init(rxBytes: 9_999_999_999_999, txBytes: 9_999_999_999_999, date: d.addingTimeInterval(0.0000001)),
older: .init(rxBytes: 0, txBytes: 0, date: d))
let rxThreshold = 100 * 1024 // 100k
let txThreshold = 100 * 1024 // 100k
let uint_rx = UInt64(rx) // this will overflow
let uint_tx = UInt64(tx)
But probably this condition is not something that has really happened, because lesser numbers work. Probably the crash was really about the division by zero leading to a NaN to UInt64 conversion.
Anyways, consider adding couple of small sanity unit tests
Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionBandwidthAnalyzer.swift
Outdated
Show resolved
Hide resolved
Sources/NetworkProtection/Diagnostics/NetworkProtectionConnectionBandwidthAnalyzer.swift
Outdated
Show resolved
Hide resolved
I made the method static and added some tests to validate all is good and prevent regressions in the future. I also changed the line that was suffering in readability, I agree. Thanks for the suggestions. The only suggestion I'm skipping is the larger inline code suggestion in your comment because IMO it favors a lower line count in detriment of readability and maintainability. I want to make sure the reader can skim through the code and understand quickly, and I think this line isn't good to that end: let (rx, tx) = NetworkProtectionConnectionBandwidthAnalyzer.bytesPerSecond(newer: .init(rxBytes: 9_999_999_999_999, txBytes: 9_999_999_999_999, date: d.addingTimeInterval(0.0000001)),
older: .init(rxBytes: 0, txBytes: 0, date: d)) |
Task/Issue URL: https://app.asana.com/0/1203137811378537/1207299387361586/f macOS PR: duckduckgo/macos-browser#2785 BSK PR: duckduckgo/BrowserServicesKit#823 ## Description Fixes a crash caused by a division by zero.
Task/Issue URL: https://app.asana.com/0/1203137811378537/1207299387361586/f iOS PR: duckduckgo/iOS#2862 BSK PR: duckduckgo/BrowserServicesKit#823 ## Description Fixes a crash caused by a division by zero.
Task/Issue URL: https://app.asana.com/0/1203137811378537/1207299387361586/f
iOS PR: duckduckgo/iOS#2862
macOS PR: duckduckgo/macos-browser#2785
What kind of version bump will this require?: Patch
Description
Fixes a crash caused by a division by zero.
Testing
See platform-specific PRs for testing instructions.
Internal references:
Software Engineering Expectations
Technical Design Template