InkAtomicList empty-checks and head peeks bypass atomic loads
- Dominant language
- C++
- Stars
- 2k
- Forks
- 874
- Avg merge
- 6d 15h
- Merged PRs (30d)
- 46
Description
While working on #13555 (see also #13571) we audited all accesses to the lock-free list head (`head_p`). All mutations go through `INK_QUEUE_LD` / `ink_atomic_cas`, but a few read-only "peeks" use plain, non-atomic reads of the pointer field:
* `INK_ATOMICLIST_EMPTY` (`include/tscore/ink_queue.h`), used by `ProtectedQueue::wait` (`src/iocore/eventsystem/ProtectedQueue.cc`) to decide whether to `cond_timedwait`.
* `AtomicSLL::head()` and `AtomicSLL::empty()` (`include/tscore/List.h`).
* `LogObject` constructor/destructor reads of `m_log_buffer` (`src/proxy/logging/LogObject.cc`), though these run in single-threaded contexts.
### Impact
These reads race with concurrent CAS writers, which is formally undefined behavior (and visible to TSAN). In practice the impact is bounded today: the pointer half is a single aligned 8-byte access on all supported targets, so it does not tear, and the one wait-gating use is a *timed* wait, so a stale answer costs at most one timeout period of event latency, not a lost wakeup.
### Suggested fix
Convert the peeks to `__atomic_load_n(&head.s.pointer, __ATOMIC_RELAXED)` (or an inline helper next to `INK_QUEUE_LD`). This is free on every supported platform and removes the formal race without changing behavior.
Low priority; filing so the audit result does not get lost.
Contributor guide
Research direction
Start by reading INK_QUEUE_LD in include/tscore/ink_queue.h, then inspect INK_ATOMICLIST_EMPTY, AtomicSLL::head(), AtomicSLL::empty(), and the m_log_buffer accesses in the listed source files. Update the read-only head peeks to use relaxed atomic loads, and verify that the relevant checks no longer produce the reported race under TSAN without changing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100