Fix #438: Check for overflow in Duration and Timestamp processing

[argv-minus-one]

This PR fixes the issues described in #438.

Breaking change: Because conversion from Timestamp to SystemTime can overflow by a platform-dependent, non-trivial amount, this PR removes impl From<Timestamp> for SystemTime and replaces it with a TryFrom implementation that fails on overflow.

[danburkert]

I've never seen this line!() trick before - it is absolutely brilliant.

[danburkert]

Great PR, thanks for the thorough test cases (and for teaching me about the line!() trick!). See https://github.com/danburkert/prost/issues/438#issuecomment-803671538 for question about approach - if possible I'd like to retain a deprecated version of the From impl for now (which can just call the TryFrom and unwrap).

[argv-minus-one]

I don't believe that's possible. Trait implementations cannot be deprecated.

Thanks, by the way. 😁

[olix0r]

This will fix https://github.com/danburkert/prost/issues/423 as well.

[olix0r]

@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.

[argv-minus-one]

@olix0r: I've rebased against current master, but that's about all I can think of. It's up to @danburkert to decide whether to merge this and whether there's anything else that needs to change first.

[danburkert]

Couple of thoughts on this PR:

1. Overall, I think the TryFrom impl in this PR should definitely land.
2. It may make sense to continue to provide a 'comment deprecated' From impl until the 0.8.0 release, even if it can't be tagged as deprecated through the normal rust annotation.
3. Long term, I think we should look into whether it would make sense to have Timestamp and Duration uphold the range contract, documented upstream to be 0001-01-01T00:00:00Z to 9999-12-31T23:59:59Z for timestamp and -315,576,000,000 to +315,576,000,000 for duration. This will require a hand-rolled impl Message, but if 'simply' defers to a derived private type it's not too bad. It will also require making the fields private and providing a ctor fn which checks the range. I haven't checked against all platforms, but hopefully these ranges are tight enough that the non-fallible From impl can remain?

[argv-minus-one]

It may make sense to continue to provide a 'comment deprecated' From impl until the 0.8.0 release, even if it can't be tagged as deprecated through the normal rust annotation.

Okay. Is there a particular style or HTML pattern or something that you'd like me to use for that?

I haven't checked against all platforms, but hopefully these ranges are tight enough that the non-fallible From impl can remain?

That doesn't help, no.

• On platforms that use 64-bit Unix time, the conversion is infallible, but it doesn't need that restriction. prost_types::Timestamp's native representation of time is 64-bit Unix time, so the conversion is infallible anyway.
• On platforms that use 32-bit Unix time, time begins in 1902 (long after year 1) and ends in 2038 (long before year 9999), so the conversion is fallible.
• On Windows, time begins in 1601 and ends in 60055. Protobuf's upper bound is within that range, but the lower bound is not, so the conversion is fallible.
• Other platforms may have their own idea of when time begins and ends, which the Protobuf range may or may not fit into.

[olix0r]

Okay. Is there a particular style or HTML pattern or something that you'd like me to use for that?

Perhaps something like this comment in tracing?

/// **This implementation is deprecated.**
///
/// ... explanation ...