Copilot commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3915986469
##########
include/tsutil/Metrics.h:
##########
Review Comment:
`entry` comes from `_splitID(id)` as an `int16_t` (low 16 bits). If those
low bits are >= 0x8000, `entry` becomes negative and will pass `entry >=
MAX_SIZE`, allowing `allocated()` to return true and callers to index
`std::array` with a negative offset (UB / OOB). Fix by rejecting negative
entries (e.g., `entry < 0`), or better, make `_splitID` return an unsigned
offset type (`uint16_t`/`size_t`) and validate it against `MAX_SIZE` before any
indexing.
##########
src/tsutil/Metrics.cc:
##########
@@ -73,6 +73,14 @@ Metrics::Storage::create(std::string_view name, const
MetricType type)
auto it = _lookups.find(name);
if (it != _lookups.end()) {
+ // Re-creating a name is how a tombstoned metric is resurrected: same
slot, same atomic, and
+ // whatever value it accumulated while it was hidden.
+ auto [blob_ix, offset] = _splitID(it->second);
+
+ if (Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); blob !=
nullptr) {
+ std::get<2>(*blob)[offset].fetch_and(static_cast<uint8_t>(~TOMBSTONE),
MEMORY_ORDER);
+ }
+
Review Comment:
This reimplements the flag-clearing logic inline and also silently does
nothing if `blob` is null, which would leave a tombstoned metric unrecoverably
hidden even though `create()` claims to resurrect it. Consider centralizing
resurrection via `tombstone(it->second, false)` (or an internal helper) so the
behavior is consistent with `tombstone()` API semantics and failure modes are
handled uniformly (e.g., by asserting `allocated(it->second)` or guaranteeing
the flag store exists for looked-up IDs).
--
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]
