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

Fix overflow in calculate_offset #25

Merged
merged 3 commits into from
Aug 17, 2024
Merged

Fix overflow in calculate_offset #25

merged 3 commits into from
Aug 17, 2024

Conversation

robertbastian
Copy link
Contributor

If client and server clock are very far out of sync (i.e. clients just booted and has clock 0), the theta calculation would overflow in the wrapping_add step, which is not correct. Pushing the / 2 onto the subterms avoids the overflow.

@vpetrigo
Copy link
Owner

vpetrigo commented Aug 2, 2024

@robertbastian thank you for the PR! Would you please tell if you can take a look for formatting issues reported?

@robertbastian
Copy link
Contributor Author

I fixed the formatting issue, but cannot reproduce the test failure locally 🤷

@vpetrigo
Copy link
Owner

vpetrigo commented Aug 10, 2024

Hm, I am able to do that. There is the test log:

attempt to add with overflow
thread 'sntpc_tests::test_ntp_request_sntpv4_supported' panicked at src\lib.rs:623:9:
attempt to add with overflow
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\std\src\panicking.rs:652
   1: core::panicking::panic_fmt
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\core\src\panicking.rs:72
   2: core::panicking::panic_const::panic_const_add_overflow
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\core\src\panicking.rs:179
   3: sntpc::offset_calculate
             at .\src\lib.rs:623
   4: sntpc::process_response
             at .\src\lib.rs:559
   5: sntpc::sntp_process_response<ref$<ref$<str$> >,std::net::udp::UdpSocket,sntpc::types::sup::StdTimestampGen>
             at .\src\lib.rs:476
   6: sntpc::get_time<ref$<ref$<str$> >,std::net::udp::UdpSocket,sntpc::types::sup::StdTimestampGen>
             at .\src\lib.rs:232
   7: sntpc::sntpc_tests::test_ntp_request_sntpv4_supported
             at .\src\lib.rs:852
   8: sntpc::sntpc_tests::test_ntp_request_sntpv4_supported::closure$0
             at .\src\lib.rs:836
   9: core::ops::function::FnOnce::call_once<sntpc::sntpc_tests::test_ntp_request_sntpv4_supported::closure_env$0,tuple$<> >
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23\library\core\src\ops\function.rs:250
  10: core::ops::function::FnOnce::call_once
             at /rustc/3f5fd8dd41153bc5fdca9427e9e05be2c767ba23/library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

And here are the patch that prevent that behavior:

Subject: [PATCH] sntpc: Update library crate
---
Index: src/lib.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/lib.rs b/src/lib.rs
--- a/src/lib.rs	(revision 609f8059d340325ee052c05f8b40c03ea15e6873)
+++ b/src/lib.rs	(date 1723294272616)
@@ -619,8 +619,8 @@
 }
 
 fn offset_calculate(t1: u64, t2: u64, t3: u64, t4: u64, units: Units) -> i64 {
-    let theta =
-        (t2.wrapping_sub(t1) / 2) as i64 + (t3.wrapping_sub(t4) / 2) as i64;
+    let theta = ((t2.wrapping_sub(t1) / 2) as i64)
+        .wrapping_add((t3.wrapping_sub(t4) / 2) as i64);
     let theta_sec = (theta.unsigned_abs() & SECONDS_MASK) >> 32;
     let theta_sec_fraction = theta.unsigned_abs() & SECONDS_FRAC_MASK;

@robertbastian
Copy link
Contributor Author

A wrapping add would avoid the panic, but I don't think it produces a correct value. It should either be a checked add, a saturating add, or use i128.

@vpetrigo
Copy link
Owner

I am not sure extending the result to i128 would work because NTP timestamp format is defined only for i64:

1                   2                   3

0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|                          Seconds                             |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|                 Seconds Fraction (0-padded)                  |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Let's switch to saturating_add then, because that seems the most reasonable way to report the offset in the case time difference between clocks is soo big.

@vpetrigo vpetrigo self-requested a review August 17, 2024 12:42
@vpetrigo vpetrigo merged commit ce3f4e8 into vpetrigo:master Aug 17, 2024
4 checks passed
@robertbastian
Copy link
Contributor Author

Works for me, thanks!

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.

2 participants