Skip to content

Mitigate panics due to falsely monotonic clocks#104

Closed
cbranch wants to merge 1 commit into
masterfrom
cbranch/nonmonotonicity
Closed

Mitigate panics due to falsely monotonic clocks#104
cbranch wants to merge 1 commit into
masterfrom
cbranch/nonmonotonicity

Conversation

@cbranch

@cbranch cbranch commented Sep 27, 2019

Copy link
Copy Markdown
Member

Example backtrace on arm64:

/lib/arm64/libnativetunnel.so (core::option::expect_failed::h4b77ebe6e62ec3a1+64)
/lib/arm64/libnativetunnel.so (std::time::Instant::duration_since::h632e3fc95ad5458d+68)
/lib/arm64/libnativetunnel.so (boringtun::noise::timers::_$LT$impl$u20$boringtun..noise..Tunn$GT$::update_timers::hc9bb6fc49d2aed16+2688)

This should never happen, but yet it does - so apply some defensive
programming.

Example backtrace on arm64:

/lib/arm64/libnativetunnel.so (core::option::expect_failed::h4b77ebe6e62ec3a1+64)
/lib/arm64/libnativetunnel.so (std::time::Instant::duration_since::h632e3fc95ad5458d+68)
/lib/arm64/libnativetunnel.so (boringtun::noise::timers::_$LT$impl$u20$boringtun..noise..Tunn$GT$::update_timers::hc9bb6fc49d2aed16+2688)

This should never happen, but yet it does - so apply some defensive
programming.
@cbranch cbranch force-pushed the cbranch/nonmonotonicity branch from 50a6866 to 36e4aaf Compare September 27, 2019 13:18
@kornelski

Copy link
Copy Markdown
Contributor

It's odd that this is happening, because Rust is supposed to have a workaround for it already: rust-lang/rust#56988

Comment thread src/noise/timers.rs
// This should be unnecessary, but we observe actual panics in the wild
// where both clock monotonicity *and* the Rust stdlib protections
// against nonmonotonic clocks violate their contracts.
let time = if time > timers.time_started {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: could be golfed down to time.max(timers.time_started)

@vkrasnov

Copy link
Copy Markdown
Contributor

My understanding it is an Android only problem?

@cbranch

cbranch commented Sep 30, 2019

Copy link
Copy Markdown
Member Author

We see this on Android, and Android is one of the platforms that is subject to the fix in rust-lang/rust#56988

Now my understanding of that fix is it should make what we observe here impossible. If you agree with this, then this suggests UB in the Android client.

@vkrasnov

Copy link
Copy Markdown
Contributor

It looks like we see the same issue on iOS13, prior iOS versions are fine.

@vkrasnov

Copy link
Copy Markdown
Contributor

Apparently there is a race between taking a new Instance and reading the Instance the handshake started. The fix is to read the handshake first, than call Instance::now().

@vkrasnov vkrasnov closed this Oct 25, 2019
@vkrasnov vkrasnov deleted the cbranch/nonmonotonicity branch April 5, 2020 11:54
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.

3 participants