Copilot commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3833724934
##########
src/tsutil/unit_tests/test_Metrics.cc:
##########
@@ -640,3 +636,85 @@ 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].
+ for (Metrics::IdType id : {Metrics::IdType{0x0000FFFF},
Metrics::IdType{0x00000400}, Metrics::IdType{0x0001FFFF}}) {
+ REQUIRE(h.valid(id) == false);
+ REQUIRE(h.lookup(id) == bad);
+ REQUIRE(h.name(id) == h.name(Metrics::IdType{0}));
+ }
+}
+
+TEST_CASE("Metrics id lookup is safe against concurrent creation",
"[libtsapi][Metrics]")
+{
+ // The id based read paths take no lock, so resolving an id races a
concurrent create. Run both
+ // sides at once, across enough metrics to cross several blob boundaries.
Under the tsan preset a
+ // non-atomic allocation counter reports a data race here; relaxing the
memory orders does not,
+ // since atomics are race free at any ordering.
+ constexpr int N_READERS = 4;
+ constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64;
+ auto &h = Metrics::hidden_instance();
+ std::atomic<bool> stop{false};
+ std::atomic<int> mismatches{0};
+
+ std::vector<std::thread> readers;
+
+ for (int t = 0; t < N_READERS; ++t) {
+ readers.emplace_back([&]() {
+ while (!stop.load(std::memory_order_relaxed)) {
+ for (Metrics::IdType id = 0; id < N_CREATE; ++id) {
+ if (!h.valid(id)) {
+ continue;
+ }
+
+ // valid() said this id names an allocated slot, so lookup() must
agree and hand back a
+ // real metric rather than clamping to the reserved bad_id slot. A
publication ordering
+ // mistake shows up here as a name that is still empty.
+ std::string_view name;
+ Metrics::MetricType type;
+ auto *m = h.lookup(id, &name, &type);
+
+ if (m == nullptr || (id != 0 && name.empty())) {
+ mismatches.fetch_add(1, std::memory_order_relaxed);
+ }
+ }
Review Comment:
The concurrent-creation test’s reader loop iterates `id` as a linear integer
(`0..N_CREATE`), but `Metrics::IdType` encodes `(blob<<16 | offset |
type<<29)`. As written, this primarily exercises blob 0 and then a range of
*invalid* offsets, and it never probes ids in later blobs (e.g. `0x00010000`,
`0x00020000`), so it doesn’t actually validate the publication/ordering
behavior across blob boundaries as intended.
--
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]
