cmcfarlen commented on PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#issuecomment-5512120280
Thanks — all three findings were real, including the two that were filed as
suppressed comments. Fixed in 9ed3ee871d.
**`tombstone()` / `tombstoned()` accepting ids that name no allocated
slot.** The bound check was `blob_ix == _cur_blob && offset > _cur_off`, which
only constrains the *current* blob. For any blob below it the offset is the low
16 bits of the id and so can reach 65535 against a 1024-entry array, and
`tombstone()` writes through it. Both now go through a new
`Storage::allocated()`:
```cpp
bool
allocated(IdType id) const
{
auto [blob, entry] = _splitID(id);
if (id < 0 || entry >= MAX_SIZE || !_blobs[blob]) {
return false;
}
return blob < _cur_blob || (blob == _cur_blob && entry < _cur_off);
}
```
This is deliberately stricter than the existing `valid()` in the second way
you noted as well: `_cur_off` is the *next* free slot, so `valid()` accepts one
slot that does not exist yet. That mattered more than I expected. A new test
marked the free slot, and an unrelated metric created in a later test section
landed in it and came out invisible, because `create()` only clears the flag
when it finds the name already present, not when it allocates a fresh slot. So
the "harmless" case was a real way to lose a metric.
I left `valid()` alone rather than tightening it here — other callers depend
on its current semantics and that is a separate change.
**The positional iterator constructor.** You are right that it let a caller
rest an iterator on a tombstoned slot, which reintroduces the non-terminating
range-walk. Rather than only skipping, the three constructors are now private
with `friend class Metrics`, so `begin()`, `end()` and `find()` are the only
ways to obtain one. `find()` already resolves a tombstoned name to `end()`. The
positional constructor also skips now, so the invariant holds for any future
in-class use.
New tests covering each case: an offset past the end of a full blob, the
next free slot, and a blob index that was never allocated.
--
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]
