-
Notifications
You must be signed in to change notification settings - Fork 505
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 #438: Check for overflow in Duration
and Timestamp
processing
#439
Fix #438: Check for overflow in Duration
and Timestamp
processing
#439
Conversation
let cases = [ | ||
// --- Table of test cases --- | ||
// test seconds test nanos expected seconds expected nanos | ||
(line!(), 0, 0, 0, 0), |
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.
I've never seen this line!()
trick before - it is absolutely brilliant.
Great PR, thanks for the thorough test cases (and for teaching me about the |
I don't believe that's possible. Trait implementations cannot be deprecated. Thanks, by the way. 😁 |
This will fix https://github.com/danburkert/prost/issues/423 as well. |
@danburkert @argv-minus-one is there anything I can do to help get this into a released version of prost-types? This bug has a security impact for services that can handle durations & timestamps from untrusted clients. |
Whether or not such an overflow is possible depends on the platform. It does not appear possible on `x86_64-unknown-linux-gnu` (which represents time with a 64-bit signed integer for seconds, same as `Timestamp`), but it is possible on `i686-unknown-linux-gnu` (which represents time with a 32-bit signed integer for seconds) and `x86_64-pc-windows-msvc` (which has a different representation of time altogether).
75c20b8
to
1b7007a
Compare
@olix0r: I've rebased against current |
Couple of thoughts on this PR:
|
Okay. Is there a particular style or HTML pattern or something that you'd like me to use for that?
That doesn't help, no.
|
Perhaps something like this comment in tracing? /// **This implementation is deprecated.**
///
/// ... explanation ... |
This PR fixes the issues described in #438.
Breaking change: Because conversion from
Timestamp
toSystemTime
can overflow by a platform-dependent, non-trivial amount, this PR removesimpl From<Timestamp> for SystemTime
and replaces it with aTryFrom
implementation that fails on overflow.