RFC 5905, "Network Time Protocol Version 4: Protocol and Algorithms Specification", June 2010

Note: This RFC has been updated by RFC 7822, RFC 8573, RFC 9109, RFC 9748

Errata ID: 8215
Status: Reported
Type: Technical
Publication Format(s) : TEXT
Reported By: Manuel Bergler
Date Reported: 2024-12-18

Section A.5.5.4 says:

        /*
         * Combine the survivor offsets and update the system clock; the
         * local_clock() routine will tell us the good or bad news.
         */
        s.t = p->t;
        clock_combine();
        switch (local_clock(p, s.offset)) {
.....
        }

It should say:

        /*
         * Combine the survivor offsets and update the system clock; the
         * local_clock() routine will tell us the good or bad news.
         */
        clock_combine();
        switch (local_clock(p, s.offset)) {
.....
        }
        s.t = p->t;

Notes:

The `clock_update` function sets `s.t` to the value of `p->t` before calling `local_clock`. This causes the assignment `mu = p->t - s.t;` inside the `local_clock` function to always set `mu` to zero, which in turn causes a division by zero when evaluating `freq = (offset - c.offset) / mu;`

Luckily, neither `clock_combine()` nor any code inside the switch's cases make use of `s.t`, neither directly nor indirectly. Moreover, the only way to return early from the `clock_update` function is by exiting the program entirely. Moving the assignment to after the `switch` thus cannot cause bugs due to using an outdated value in `clock_combine`, any of the cases and anything happening after `clock_update` returns.

All that is left to do to verify that the suggested correction fixes the problem without introducing other bugs is to ensure that moving the assignment doesn't unintentionally change the behavior of the `local_clock` function. There are three distinct cases how `s.t` is used inside `local_clock`:

- Its value is used in the (currently problematic) calculation of `mu`
- Its value is updated through calls of `rstclock`
- Its value is used in the condition of the `if` statement `if (c.t - s.t < WATCH) return (IGNORE);` in the `FREQ` case on pg 101

The change of behavior of the computation of `mu` is the intended effect of this erratum, so that is fine.
The calls to `rstclock` all set the value of `s.t` to the value of `p->t` (assuming erratum 8214 [0] is correct) , so having the assignment `s.t = p->t;` at the end of `clock_update` fortunately doesn't conflict with the value set by `rstclock`.

Since I haven't fully grasped how the clock discipline is supposed to work I'm unsure about what to do with the `c.t - s.t < WATCH` condition, though. It might be the case that the condition should've used the old value of `s.t` just like the computation of `mu`, in which case there is nothing to do. If, however, the current behavior is correct, the condition has to be replaced by `c.t - p->t < WATCH`.


[0] https://www.rfc-editor.org/errata/eid8214

Report New Errata