PaulStoffregen / Time

Time library for Arduino
http://playground.arduino.cc/code/time
1.26k stars 670 forks source link

now() bug when sysTime is about to overflow #158

Open ghost opened 3 years ago

ghost commented 3 years ago

Consider the following snippet from now():

  if (nextSyncTime <= sysTime) {
    if (getTimePtr != 0) {
      time_t t = getTimePtr();
      if (t != 0) {
        setTime(t);
      } else {
        nextSyncTime = sysTime + syncInterval;
        Status = (Status == timeNotSet) ?  timeNotSet : timeNeedsSync;
      } 
    }
  } 

This code fails when sysTime+syncInterval overflows 2^32. What happens is then nextSyncTime will be assigned a small number guaranteeing the test on the first line will be true and getTimePtr() will be called for every single call of now() until sysTime itself overflows.

One way to fix this is to change the first line so it protects against the case when nextSyncTime has overflowed but sysTime has not, such as by assuming syncInterval will never be larger than 2^31 as follows:

if (sysTime - nextSyncTime < 2147483648U) {

I found this when I discovered my IoT was making calls to NTP continuously for an hour about every 50 days. You see 50 days is 2^32 seconds and I had set syncInterval to 3600.

Cheers!

garudaonekh commented 1 year ago

hi, I am not sure if my case is the same as you or not. now() return minus value after a few hours.

rob040 commented 1 year ago

You see 50 days is 2^32 seconds

Only if a systick was 1 millisecond, but that is not the case. You are off by a factor 1000.

For decennia, Unix uses 32-bit number to represent time since 1-jan-1970. In 2038 it will pass the 2^31 value, which supposedly some systems would indicate as a negative time. In another 68 years, in 2106, the 32-bit time rollover will take place.

karelv commented 12 months ago

Yes, the sysTime variable is 32 bit only, luckily it is unsigned, so we have till 2106! It's a pitty, as time_t is 8 byte on Teensy 4.1...