moonchen commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3918995640
##########
src/tsutil/Metrics.cc:
##########
@@ -58,12 +58,17 @@ Metrics::Storage::addBlob() // The mutex must be held
before calling this!
{
auto blob = std::make_unique<Metrics::NamesAndAtomics>();
+ auto const cur_blob = _cur_blob.load(std::memory_order_relaxed);
+
debug_assert(blob);
- // The write below is to _blobs[_cur_blob + 1], so the last usable blob
index is MAX_BLOBS - 1.
- release_assert(_cur_blob < MAX_BLOBS - 1);
+ // The write below is to _blobs[cur_blob + 1], so the last usable blob index
is MAX_BLOBS - 1.
+ release_assert(cur_blob < MAX_BLOBS - 1);
+
+ _blobs[cur_blob + 1] = std::move(blob);
+ _cur_off.store(0, std::memory_order_relaxed);
Review Comment:
Could the blob and offset be published as one coherent state, or via
per-blob published bounds?
A reader can load the old `_cur_blob`, then observe `_cur_off == 0` after
the reset above but before the `_cur_blob` store. `_is_allocated()` will
consequently reject every valid ID in the just-completed blob.
This is more serious than a dropped increment: with the default
`ENABLE_FAST_SDK=OFF`, `TSStatInt*` passes that result to `sdk_assert`, which
calls the non-returning `_TSReleaseAssert`. If the initial sanity check
succeeds but the subsequent lookup lands in this window, lookup instead clamps
the operation to `bad_id`.
##########
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].
+ 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, over 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.
+ //
+ // Readers take ids from what the writer has registered rather than counting
integers: an id packs
+ // the blob index above the offset, so consecutive integers only ever name
the first blob. The
+ // store is relaxed, so a reader can pick up an id whose slot is not
published yet, which is the
+ // case of interest.
+ 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::atomic<int> resolved{0};
+
+ std::vector<std::atomic<Metrics::IdType>> created(N_CREATE);
+
+ for (auto &c : created) {
+ c.store(Metrics::NOT_FOUND, std::memory_order_relaxed);
+ }
+
+ std::vector<std::thread> readers;
+
+ for (int t = 0; t < N_READERS; ++t) {
+ readers.emplace_back([&]() {
+ int n = 0;
+
+ while (!stop.load(std::memory_order_relaxed)) {
+ for (int i = 0; i < N_CREATE; ++i) {
+ auto const id = created[i].load(std::memory_order_relaxed);
+
+ if (id == Metrics::NOT_FOUND || !h.valid(id)) {
+ continue;
+ }
+
+ // valid() accepted the id, so lookup() must hand back the metric
with its name rather
+ // than clamping to the reserved bad_id slot.
+ std::string_view name;
+ Metrics::MetricType type;
+ auto *m = h.lookup(id, &name, &type);
+
+ if (m == nullptr || name.empty()) {
Review Comment:
This check does not enforce the preceding comment. An invalid lookup returns
the `bad_id` pointer, whose name is non-empty, so neither condition detects
that lookup clamped.
Since the expected name is known from `i`, please compare the returned name
with `"pub.order." + std::to_string(i)` or otherwise explicitly reject the
`bad_id` result.
##########
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].
+ 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, over 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.
+ //
+ // Readers take ids from what the writer has registered rather than counting
integers: an id packs
+ // the blob index above the offset, so consecutive integers only ever name
the first blob. The
+ // store is relaxed, so a reader can pick up an id whose slot is not
published yet, which is the
+ // case of interest.
+ 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::atomic<int> resolved{0};
+
+ std::vector<std::atomic<Metrics::IdType>> created(N_CREATE);
+
+ for (auto &c : created) {
+ c.store(Metrics::NOT_FOUND, std::memory_order_relaxed);
+ }
+
+ std::vector<std::thread> readers;
+
+ for (int t = 0; t < N_READERS; ++t) {
+ readers.emplace_back([&]() {
+ int n = 0;
+
+ while (!stop.load(std::memory_order_relaxed)) {
+ for (int i = 0; i < N_CREATE; ++i) {
+ auto const id = created[i].load(std::memory_order_relaxed);
+
+ if (id == Metrics::NOT_FOUND || !h.valid(id)) {
+ continue;
+ }
+
+ // valid() accepted the id, so lookup() must hand back the metric
with its name rather
+ // than clamping to the reserved bad_id slot.
+ std::string_view name;
+ Metrics::MetricType type;
+ auto *m = h.lookup(id, &name, &type);
+
+ if (m == nullptr || name.empty()) {
+ mismatches.fetch_add(1, std::memory_order_relaxed);
+ }
+ ++n;
+ }
+ }
+ resolved.fetch_add(n, std::memory_order_relaxed);
+ });
+ }
+
+ for (int i = 0; i < N_CREATE; ++i) {
+ auto const nm = "pub.order." + std::to_string(i);
+
+ REQUIRE(Metrics::Counter::createHiddenPtr(nm) != nullptr);
+ created[i].store(h.lookup(nm), std::memory_order_relaxed);
+ }
+
+ stop.store(true, std::memory_order_relaxed);
Review Comment:
Please add a barrier or ready-count handshake before starting the writer
loop. Nothing currently guarantees that any reader reaches its loop before
`stop` becomes true.
I reproduced the failure by pinning this test to one CPU: on run 4, every
reader observed `stop == true` before doing work and the test failed at
`resolved.load() > 0`.
--
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]
