Very inaccurate client fails to request NTP data

[dansebcar]

When the client thinks the current time is after the year 2037, the library fails to make an NTP request.

This can be easily demonstrated using the faketime library:

$ faketime '2037-01-01 00:00:00' python3 -c 'import ntplib; ntplib.NTPClient().request("0.pool.ntp.org")'
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/ntplib.py", line 170, in to_data
    packed = struct.pack(
struct.error: 'I' format requires 0 <= number <= 4294967295

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python3.10/site-packages/ntplib.py", line 313, in request
    s.sendto(query_packet.to_data(), sockaddr)
  File "/usr/local/lib/python3.10/site-packages/ntplib.py", line 189, in to_data
    raise NTPException("Invalid NTP packet fields.")
ntplib.NTPException: Invalid NTP packet fields.

Before the year 2036 the above succeeds as expected. Tested on the python:3.10 docker image with ntplib 0.4.0. I'm not sure if the problem is just due to the ~15 year inaccuracy, or this version of the library will stop working at some point in 2036.

[cf-natali]

Hi @dansebcar ,

Thanks for this interesting issue :).

Indeed, NTPv2 has a 32-bit timestamp for seconds and the epoch is in 1900, so it suffers from rollover sometime in 2036: https://en.wikipedia.org/wiki/Network_Time_Protocol#Timestamps

The 64-bit timestamps used by NTP consist of a 32-bit part for seconds and a 32-bit part for fractional second, giving a time scale that rolls over every 232 seconds (136 years) and a theoretical resolution of 2−32 seconds (233 picoseconds). NTP uses an epoch of January 1, 1900. Therefore, the first rollover occurs on February 7, 2036.[29][30]

This is fixed by NTPv4, which is not supported - I guess I could look into it when I have time.

One could try to be smart and account for rollover however it's very likely more trouble than it's worth and would very likely break around the rollover timepoint.

[dansebcar]

Thank you for the explanation. This means if I want to reliably check my local time, I might write:

import ntplib


def is_local_time_accurate():
    client = ntplib.NTPClient()
    try:
        response = client.request("0.pool.ntp.org")
    except ntplib.NTPException:
        return False
    return response.offset < 1

But that might return False even if something else failed within the library. Would it be suitable for ntplib to check before building the struct if the local time is not supported, and raise a more specific exception in that case?

[cf-natali]

Hm, I'm not sure about adding a more specific exception than ntplib.NTPException, it feels really like a very specific case for something which should not really happen...

[dansebcar]

I disagree. Over the next 14 years, more and more computers with drifting real-time-clocks or software bugs will think it's 2036 (or later). I only started looking at this library when I logged on to a server that thought it was 2049.

Also, I would argue that the user experience would be improved by making this change. If the user is informed that their local time is not supported, they know what corrective action to take. The message "Invalid NTP packet fields." is less helpful.

[cf-natali]

See https://github.com/cf-natali/ntplib/pull/22 which should raise a new exception:

$ faketime '2036-02-07 12:00:00' python3 -c 'import ntplib; print(ntplib.NTPClient().request("pool.ntp.org").offset)'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/cf/src/ntplib/ntplib.py", line 315, in request
    tx_timestamp=system_to_ntp_time(time.time())
  File "/home/cf/src/ntplib/ntplib.py", line 403, in system_to_ntp_time
    raise NTPRolloverException("Timestamp %s is beyond NTPv3 rollover" %
ntplib.NTPRolloverException: Timestamp 2085998400.817515 is beyond NTPv3 rollover

I'll merge in a few days, reviews welcome.