Mitigate panics due to falsely monotonic clocks #104
Conversation
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
force-pushed
the
cbranch/nonmonotonicity
branch
from
50a6866 to
36e4aaf
Compare
|
It's odd that this is happening, because Rust is supposed to have a workaround for it already: rust-lang/rust#56988 |
| // 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 { |
There was a problem hiding this comment.
nit: could be golfed down to time.max(timers.time_started)
|
My understanding it is an Android only problem? |
|
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. |
|
It looks like we see the same issue on iOS13, prior iOS versions are fine. |
|
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(). |
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.