ztimer/periodic: remove timer on init if already running

[benpicco]

Contribution description

If ztimer_periodic_init() is called on a periodic timer that is already running, the init function will completely overwrite the ztimer_t structure that then is already part of a linked list.

This causes corruption of the clock's timer list and leads to interesting results (e.g. if the timer was the first element in the list, it will now be added again causing ->next to point to itself which creates a circular list and an infinite loop when iterating the clock's timers.)

Testing procedure

Issues/PRs references

[riot-ci]

Murdock results

✔️ PASSED

eadb374 ztimer/periodic: remove timer on init if already running

Success	Failures	Total	Runtime
6936	0	6936	11m:53s

Artifacts

• Documentation preview

[fabian18]

I think it could be that, when the timer is passed uninitialized, like in the test, it could contain a garbage next pointer.
The if the list is emty, the _is_set() function could return a false positive.

[benpicco]

Ugh, I somehow expected the ztimer_is_set() function to iterate all timers on the clock and compare their address with the passed timer, not rely on the values inside the timer struct at all - but that's indeed not the case.

Maybe we need something like

bool ztimer_is_set_safe(const ztimer_clock_t *clock, const ztimer_t *timer)
{
    const ztimer_base_t *list = &clock->list;
    const ztimer_base_t *entry = &timer->base;

    while (list->next) {
        ztimer_base_t *list_entry = list->next;
        if (list_entry == entry) {
            return true;
        }
        list = list->next;
    }

    return false;
}

[kfessel]

maybe a timer should at least be stopped before being reinitialized e.g.: if the clock is changed in the reinitialize this would not work. even with this change and the ztimer_safe_is_set, if we would make that work for reinit (using the .clock) it might not work for init any more.

I am not sure why you want to reinit but probably to change the period -> maybe we need a set period function.

[kaspar030]

The if the list is emty, the _is_set() function could return a false positive.

True. Maybe the documentation needs updating - that's more like a "is maybe set".

ztimer_remove() only iterates if there's something set for the ztimer_t's next, which is an optimization (as if there's no "next", the timer cannot be set due to the timer list being a circular list).

ztimer_remove() is safe w.r.t. having garbage in the ztimer_t struct, so is_set(t) -> ztimer_remove(t) is fine even for uninitialized timers.

[benpicco]

ztimer_remove() is safe w.r.t. having garbage in the ztimer_t struct, so is_set(t) -> ztimer_remove(t) is fine even for uninitialized timers.

So I can drop the ztimer_is_set(clock, &timer->timer) check and just call ztimer_remove(clock, &timer->timer) unconditionally?

[kaspar030]

So I can drop the ztimer_is_set(clock, &timer->timer) check and just call ztimer_remove(clock, &timer->timer) unconditionally?

yes, that will do the right thing, more efficiently.

[benpicco]

Say no more!

[kaspar030]

ACK.

[kaspar030]

bors merge

[benpicco]

bors merge

[bors]

Already running a review

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• Murdock
• static-tests
• tools-build-success

[kfessel]

this line should be
ztimer_remove(timer->clock, &timer->timer)

since clock may not be the clock the timer is set on it can not be safely removed there (there is just a good chance that the same clock is used for the reinitialize)

but doing that would also call the remove function with a not initialized timer->clock, which may also not be safe