Skip to content
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

Avoid panics in uses of Instant #2746

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented Jan 31, 2022

We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in hyper, but #2385 reports a similar issue.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of Instant that are prone to this
bug.

This change replaces uses of Instant::elapsed and Instant::sub with
calls to Instant::saturating_duration_since to prevent this class of
panic.

We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in hyper, but hyperium#2385 reports a similar issue.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic.
@@ -721,7 +723,8 @@ impl Expiration {

fn expires(&self, instant: Instant) -> bool {
match self.0 {
Some(timeout) => instant.elapsed() > timeout,
// Avoid `Instant::elapsed` to avoid issues like rust-lang/rust#86470.
Some(timeout) => Instant::now().saturating_duration_since(instant) > timeout,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely feels like Instant::elapsed() should handle this, or otherwise it's not really safe to use. Looks like rust-lang/rust#89926 agrees.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. It will probably take at least a few months for those changes to be available in a stable release (the RFC still appears to be open). This change would eliminate these problems well in advance of that, even for projects that are not on the latest stable Rust version.

Is there something I can do to make the temporary nature of this hack/fix better documented?

olix0r added a commit to tower-rs/tower that referenced this pull request Jan 31, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in hyper, but #2385 reports a similar issue.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic. These fixes should ultimately be made in the standard library,
but this change lets us avoid this problem while we wait for those
fixes.

See also hyperium/hyper#2746
olix0r added a commit to tower-rs/tower that referenced this pull request Jan 31, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in tower, but we have some potentialy flawed `Instant`
arithmetic that could panic in this way.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic. These fixes should ultimately be made in the standard library,
but this change lets us avoid this problem while we wait for those
fixes.

See also hyperium/hyper#2746
olix0r added a commit to tower-rs/tower that referenced this pull request Jan 31, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in tower, but we have some potentialy flawed `Instant`
arithmetic that could panic in this way.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic. These fixes should ultimately be made in the standard library,
but this change lets us avoid this problem while we wait for those
fixes.

See also hyperium/hyper#2746
olix0r added a commit to hyperium/h2 that referenced this pull request Jan 31, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in h2, but there is one use of `Instant::sub` that
could panic in this way.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug. These fixes should ultimately be made in the standard library, but
this change lets us avoid this problem while we wait for those fixes.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic.

See also hyperium/hyper#2746
seanmonstar pushed a commit to hyperium/h2 that referenced this pull request Feb 1, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in h2, but there is one use of `Instant::sub` that
could panic in this way.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug. These fixes should ultimately be made in the standard library, but
this change lets us avoid this problem while we wait for those fixes.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic.

See also hyperium/hyper#2746
seanmonstar pushed a commit to tower-rs/tower that referenced this pull request Feb 1, 2022
We have reports of runtime panics (linkerd/linkerd2#7748) that sound a
lot like rust-lang/rust#86470. We don't have any evidence that these
panics originate in tower, but we have some potentialy flawed `Instant`
arithmetic that could panic in this way.

Even though this is almost definitely a bug in Rust, it seems most
prudent to actively avoid the uses of `Instant` that are prone to this
bug.

This change replaces uses of `Instant::elapsed` and `Instant::sub` with
calls to `Instant::saturating_duration_since` to prevent this class of
panic. These fixes should ultimately be made in the standard library,
but this change lets us avoid this problem while we wait for those
fixes.

See also hyperium/hyper#2746
@seanmonstar seanmonstar merged commit dcdd6d1 into hyperium:master Feb 1, 2022
@olix0r olix0r deleted the ver/hyper-no-instant-panics branch February 1, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants