timeutil: prevent illegal sharing of Timers

[yuzefovich]

timeutil: prevent illegal sharing of Timers

Currently, whenever Timer.Stop is called, we unconditionally put that timer into timerPool. This works well assuming that the contract of Stop is satisfied - namely that the stopped timer can no longer be used.

However, we have at least one case where this contract is violated (fixed in the following commit), and in such a scenario it is possible for multiple users to access the same timer object which can lead to an undefined behavior (we've seen a deadlock on a test server shutdown).

This commit hardens the timer code to prevent a class of such issues by only putting the timer into the pool on Stop only if the timer originally came from the pool.

Release note: None

stmtdiagnostics: fix incorrect usage of timeutil.Timer

The contract of timeutil.Timer.Stop is such that the timer can no longer be used, and it was violated in Registry.poll. This now fixed by allocating a fresh timer after each Stop call.

Fixes: #119593.

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

pkg/util/timeutil/timer.go line 110 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

On a quick search there are like 20 places where the timers are allocated on the "stack" and don't use the pool. For example, replicaScanner from server/scanner.go - it seems reasonable to me that we embed the timer into the parent struct to avoid accessing timerPool (although, later, on Reset we'll still access the shared timeTimerPool). To me this seems like a reasonable use case since we avoid an extra pointer to track / extra access to timerPool - WDYT?

I see, makes sense. No need to disallow stack allocation here then.

[yuzefovich]

TFTRs!

bors r+

[craig[bot]]

Build succeeded:

• Bazel Essential CI (Cockroach)