Copilot commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3845599631
##########
src/tsutil/Metrics.cc:
##########
@@ -188,34 +201,39 @@ Metrics::Storage::createSpan(size_t size,
Metrics::MetricType type, Metrics::IdT
// On the final blob there is nowhere left to grow, so refuse a span that
would fill or overflow
// it rather than letting addBlob() assert. Same intent as the guard in
create(), and the same
// cost: some slots of the last blob go unused.
- if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
+ if (_cur_blob.load(std::memory_order_relaxed) >= MAX_BLOBS - 1 &&
_cur_off.load(std::memory_order_relaxed) + size >= MAX_SIZE) {
Review Comment:
In `createSpan()`, `_cur_off` / `_cur_blob` are loaded multiple times before
the re-read (and `_cur_off` is loaded twice just for the size-fit checks).
Caching the initial relaxed loads into locals for the two early guard checks
would make the logic easier to follow and avoids extra atomic loads. (Keeping
the existing re-read after `addBlob()` still makes sense.)
##########
include/tsutil/Metrics.h:
##########
@@ -354,15 +359,37 @@ class Metrics
current() const
{
std::lock_guard lock(_mutex);
- return {_cur_blob, _cur_off};
+ return {_cur_blob.load(std::memory_order_relaxed),
_cur_off.load(std::memory_order_relaxed)};
}
+ /** Whether @a id names an allocated slot.
+ *
+ * The gate for every id based accessor, since ids from the @c TSStat* API
are untrusted. An id
+ * qualifies when it is non-negative, its offset is one @c _makeId could
produce, and its slot
+ * has been handed out.
+ */
bool
- valid(IdType id) const
+ _is_allocated(IdType id) const
{
- auto [blob, entry] = _splitID(id);
+ if (id < 0) {
+ return false;
+ }
+
+ auto [blob_ix, offset] = _splitID(id);
- return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob ==
_cur_blob && entry <= _cur_off)));
+ // _cur_blob first: acquiring it also makes visible everything published
under it.
+ auto const cur_blob = _cur_blob.load(std::memory_order_acquire);
+ auto const cur_off = _cur_off.load(std::memory_order_acquire);
+
+ // A non-null blob past cur_blob is allocated but not yet published,
hence <= and < rather
+ // than a test for "not the current blob".
+ return offset < MAX_SIZE && blob_ix <= cur_blob && _blobs[blob_ix] !=
nullptr && (blob_ix < cur_blob || offset < cur_off);
+ }
+
+ bool
+ valid(IdType id) const
+ {
+ return _is_allocated(id);
}
Review Comment:
`_is_allocated()` looks like an internal helper (leading underscore naming)
but is declared in the `public:` section of `Storage`. If this isn’t intended
as part of the reachable API surface, make it `private` (or at least
`protected`) and keep `valid()` as the public gate. If it is intended to be
public, consider renaming it to a non-underscore name consistent with the rest
of the public methods.
##########
src/tsutil/unit_tests/test_Metrics.cc:
##########
@@ -640,3 +637,116 @@ TEST_CASE("Metrics span lands exactly on a blob
boundary", "[libtsapi][Metrics]"
REQUIRE(Metrics::Counter::load(p) == 7);
REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p);
}
+
+TEST_CASE("Metrics malformed id offsets resolve to bad_id",
"[libtsapi][Metrics]")
+{
+ // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a
malformed one must
+ // not index past a blob's arrays. Two blobs are needed for the offset check
to be what rejects
+ // it; with one, the null blob check would.
+ auto &h = Metrics::hidden_instance();
+
+ for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) {
+ REQUIRE(Metrics::Counter::createHiddenPtr("f1.fill." + std::to_string(i))
!= nullptr);
+ }
+
+ auto const *bad = h.lookup(Metrics::IdType{0}); // the reserved bad_id slot
+ REQUIRE(bad != nullptr);
+
+ // blob 0 is allocated, so the null check does not fire; only the MAX_SIZE
test stands between
+ // this and atomics[65535].
Review Comment:
These new test cases mutate the singleton hidden metrics store
(`hidden_instance()`) by allocating >MAX_SIZE metrics. Because Catch2 does not
guarantee test execution order, this can make the overall unit test suite
order-dependent (e.g., later tests may see an unexpectedly large pre-populated
store or approach store limits). Consider adding a test-only reset/isolated
Storage instance for these cases, or restructuring so the tests don't depend on
persistent global state across TEST_CASEs.
##########
src/tsutil/Metrics.cc:
##########
@@ -188,34 +201,39 @@ Metrics::Storage::createSpan(size_t size,
Metrics::MetricType type, Metrics::IdT
// On the final blob there is nowhere left to grow, so refuse a span that
would fill or overflow
// it rather than letting addBlob() assert. Same intent as the guard in
create(), and the same
// cost: some slots of the last blob go unused.
- if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
+ if (_cur_blob.load(std::memory_order_relaxed) >= MAX_BLOBS - 1 &&
_cur_off.load(std::memory_order_relaxed) + size >= MAX_SIZE) {
if (id) {
*id = 0; // Slot 0 is the reserved bad_id.
}
return {};
}
// A span has to be contiguous, so one that does not fit in the current blob
starts a new one.
- if (_cur_off + size > MAX_SIZE) {
+ if (_cur_off.load(std::memory_order_relaxed) + size > MAX_SIZE) {
addBlob();
}
- Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type);
- Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get();
+ // Re-read: addBlob() above may have moved both.
+ auto const cur_blob = _cur_blob.load(std::memory_order_relaxed);
+ auto const cur_off = _cur_off.load(std::memory_order_relaxed);
Review Comment:
In `createSpan()`, `_cur_off` / `_cur_blob` are loaded multiple times before
the re-read (and `_cur_off` is loaded twice just for the size-fit checks).
Caching the initial relaxed loads into locals for the two early guard checks
would make the logic easier to follow and avoids extra atomic loads. (Keeping
the existing re-read after `addBlob()` still makes sense.)
##########
src/tsutil/Metrics.cc:
##########
@@ -188,34 +201,39 @@ Metrics::Storage::createSpan(size_t size,
Metrics::MetricType type, Metrics::IdT
// On the final blob there is nowhere left to grow, so refuse a span that
would fill or overflow
// it rather than letting addBlob() assert. Same intent as the guard in
create(), and the same
// cost: some slots of the last blob go unused.
- if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
+ if (_cur_blob.load(std::memory_order_relaxed) >= MAX_BLOBS - 1 &&
_cur_off.load(std::memory_order_relaxed) + size >= MAX_SIZE) {
if (id) {
*id = 0; // Slot 0 is the reserved bad_id.
}
return {};
}
// A span has to be contiguous, so one that does not fit in the current blob
starts a new one.
- if (_cur_off + size > MAX_SIZE) {
+ if (_cur_off.load(std::memory_order_relaxed) + size > MAX_SIZE) {
Review Comment:
In `createSpan()`, `_cur_off` / `_cur_blob` are loaded multiple times before
the re-read (and `_cur_off` is loaded twice just for the size-fit checks).
Caching the initial relaxed loads into locals for the two early guard checks
would make the logic easier to follow and avoids extra atomic loads. (Keeping
the existing re-read after `addBlob()` still makes sense.)
--
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]
