masaori335 commented on PR #13409:
URL: https://github.com/apache/trafficserver/pull/13409#issuecomment-5100594795
I'm just forwarding message, but P0 and P1 seems blocker. P4 is pretty
interesting, this fixed a leak silently, which I introduced, thank you :)
----
# Review: `Add live HostDB cache clearing` (aa9fabc9c6)
The design is sound: `probe()` returns a `Ptr<HostDBRecord>` and
`ResolveInfo::record`
holds it, so in-flight transactions genuinely survive a clear. The
status-handler
rewrite also silently fixes a real refcount leak. But the new `clear()`
exposes a
pre-existing lock-discipline hole in a way that is now operator-triggerable.
Findings are ranked P0 (blocking, memory-unsafe) through P4 (nit).
---
## P0 — `clear()` races with unlocked `erase()`: data race / use-after-free
`RefCountCache::clear()` now takes the partition's exclusive lock
(`src/iocore/hostdb/P_RefCountCache.h:513`), which correctly excludes
`probe()`
(shared lock, `src/iocore/hostdb/HostDB.cc:482`) and `put()` (unique lock,
`src/iocore/hostdb/HostDB.cc:1000`).
It does **not** exclude the two `erase()` call sites that take no partition
lock at all:
- `src/iocore/hostdb/HostDB.cc:887` — `dnsEvent()` NXDOMAIN path,
`hostDB.refcountcache->erase(old_r->key)`. This is a common path.
- `src/iocore/hostdb/HostDB.cc:405` — `reply_to_cont()`, reached unlocked
from
`dnsEvent` (`:1030`) and `probeEvent` (`:1109`).
`erase()` mutates the intrusive hash map, mutates the `PriorityQueue
expiry_queue`,
and frees the `RefCountCacheHashEntry` back to the ClassAllocator.
Concurrently the
RPC thread walks and destroys that same map under a lock the ET_NET thread
never
takes. Result: corrupted bucket/expiry lists, double-free, or UAF.
Before this commit `clear()` was only ever called from unit tests, so the
unlocked
`erase()` sites raced only against each other and against `put()` on the same
partition — a much narrower window. This commit turns it into "operator runs
`traffic_ctl hostdb clear` during an NXDOMAIN storm."
**This is not a one-line fix.** `getby()` holds a *shared* lock at
`src/iocore/hostdb/HostDB.cc:580` and calls `reply_to_cont()` at `:600`,
which can
call `erase()`. `ts::shared_mutex` wraps a non-recursive, writer-preferring
`pthread_rwlock_t`, so naively adding a `unique_lock` inside `erase()`
self-deadlocks
that path. Either hoist the erase out of `reply_to_cont()`, or restructure
so `getby()`
releases the shared lock before calling it.
**Strongly recommended:** annotate the partition state so the compiler
enumerates the
violations instead of relying on review —
```cpp
hash_type item_map TS_GUARDED_BY(lock);
PriorityQueue<RefCountCacheHashEntry *> expiry_queue TS_GUARDED_BY(lock);
void erase(uint64_t key, ink_time_t expiry_time = -1) TS_REQUIRES(lock);
```
Note this will only flag on macOS/libc++ — on Fedora CI the std lock types
are not
capabilities, so `-Wthread-safety` will stay quiet even with `AS_ERROR=ON`.
## P1 — Null deref if `clear` is called during startup
`initialize_jsonrpc_server()` runs at
`src/traffic_server/traffic_server.cc:2143`;
`hostDBProcessor.start()` at `:2365`. Between them:
`eventProcessor.start()`, thread
spawn, `Log::init()`. `hostDB.refcountcache` is `nullptr` until
`HostDBCache::start()`
(`src/iocore/hostdb/HostDB.cc:289`). A `traffic_ctl hostdb clear` in that
window
segfaults traffic_server.
`get_hostdb_status` has the same pre-existing hole (its `try`/`catch` does
not catch
SIGSEGV), so one shared guard fixes both:
```cpp
if (hostDB.refcountcache == nullptr) {
return {errata: "HostDB is not initialized"};
}
```
## P2 — The headline behavior is untested
`tests/gold_tests/dns/hostdb_clear.test.py` covers only the happy path.
Nothing
verifies that an in-flight transaction survives a clear — the one behavior
the commit
message and both doc changes promise, and the one most likely to silently
regress.
## P3 — Emptiness assertion is a weak negative
The test asserts `ExcludesExpression(hostname)` on `traffic_ctl hostdb
status` output.
That passes for the wrong reasons too (RPC error, empty response, changed
formatting).
Assert on the metric instead: `proxy.process.hostdb.cache.current_items ==
0`.
## P3 — No audit log for a destructive operation
Wiping the entire DNS cache emits nothing to `diags.log`. Add a
`Note("HostDB cleared via JSONRPC")` in `clear_hostdb()`
(`src/mgmt/rpc/handlers/hostdb/HostDB.cc:212`).
## P3 — `RefCountCachePartition::copy()` is now dead code
The status handler was its only caller. Delete the declaration
(`src/iocore/hostdb/P_RefCountCache.h:176`) and definition (`:346-355`).
## P3 — Prefer `ts::write_guard` over `std::unique_lock`
`include/tsutil/TsSharedMutex.h:188` explicitly directs new code to the
capability-carrying guards. The surrounding `std::unique_lock` uses in
`HostDB.cc` are
legacy, not convention. Relevant to the P0 annotation work above.
## P3 — Writer-lock stall on ET_NET
Freeing up to `max_count / partitions` entries per partition under an
exclusive lock
blocks ET_NET threads in `probe()`, and Linux's writer-preferring rwlock
queues new
readers behind it. Bounded per partition, so likely fine — but consider
scheduling the
per-partition clears on ET_TASK rather than running them inline on the RPC
thread.
## P4 — Leak fix is real but unmentioned
The old status handler called `partition.copy()`, which `alloc()`s a
`RefCountCacheHashEntry` per record and `make_ptr()`s the item (refcount++),
then
dropped the raw pointers on the floor. Every `traffic_ctl hostdb status`
leaked one
hash entry and one permanent refcount per record — meaning those records
could never
be freed. The new `Ptr`-based collection fixes it. Worth a line in the
commit message
so it is not lost.
## P4 — Docs imply host-file entries are cleared
Host-file entries are correctly left alone (they are config, not cache), but
"Remove
all DNS resolution records from HostDB" reads like they would be affected. A
clarifying clause in both `doc/appendices/command-line/traffic_ctl.en.rst`
and
`doc/developer-guide/jsonrpc/jsonrpc-api.en.rst` would help.
## P4 — Argument-count rejection untested
`add_command("clear", ..., 0, ...)` declares arity 0, but no test exercises
`traffic_ctl hostdb clear extra_arg`.
## P4 — Inconsistent `StillRunningAfter` in the last test run
The final test run sets only `tr.StillRunningAfter = ts`, dropping `dns` and
`server`
unlike every preceding run. Harmless, but inconsistent.
---
**Merge gate:** P0 and P1 should block. P2 is worth doing before merge,
since the
in-flight guarantee is the feature's main safety claim. P3/P4 are cleanups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
