From 5c85b31cdc5ec588371311d9d5404fca3e4f2aaf Mon Sep 17 00:00:00 2001 From: Matthew Johnson Date: Tue, 16 Jul 2024 19:04:53 -0400 Subject: [PATCH] replace invalid characters in ids (#106) It is easy enough to break the spectatord line protocol, if any of the control characters (`:,=`) are inserted in unexpected places. Since metric names and tags are often programmatically generated, we want to only construct id strings which are valid. --- .gitignore | 1 + BUILD.bazel | 14 +++++++ spectator/age_gauge_test.cc | 8 ++++ spectator/counter_test.cc | 14 +++++-- spectator/dist_summary_test.cc | 8 ++++ spectator/gauge_test.cc | 8 ++++ spectator/id.h | 3 +- spectator/id_test.cc | 6 +++ spectator/max_gauge_test.cc | 8 ++++ spectator/monotonic_counter_test.cc | 9 +++++ spectator/monotonic_counter_uint_test.cc | 34 +++++++++++++++++ spectator/perc_dist_summary_test.cc | 16 ++++++-- spectator/perc_timer_test.cc | 14 +++++-- spectator/stateless_meters.h | 35 ++++++++++++++++- spectator/timer_test.cc | 15 ++++++-- tools/gen_valid_chars.cc | 48 ++++++++++++++++++++++++ 16 files changed, 222 insertions(+), 19 deletions(-) create mode 100644 spectator/monotonic_counter_uint_test.cc create mode 100644 tools/gen_valid_chars.cc diff --git a/.gitignore b/.gitignore index 54b6549..96ef948 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ .vscode/ bazel-* build-* +spectator/valid_chars.inc diff --git a/BUILD.bazel b/BUILD.bazel index 1955d40..1b2d258 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -25,6 +25,20 @@ cc_library( ], ) +cc_binary( + name = "gen_valid_chars", + srcs = ["tools/gen_valid_chars.cc"], + visibility = ["//visibility:public"], +) + +genrule( + name = "generate_valid_chars_inc", + srcs = [":gen_valid_chars"], # Dependency on the gen_valid_chars executable + outs = ["spectator/valid_chars.inc"], + cmd = "$(location :gen_valid_chars) > $(OUTS)", + visibility = ["//visibility:public"], +) + cc_test( name = "spectator_test", srcs = glob([ diff --git a/spectator/age_gauge_test.cc b/spectator/age_gauge_test.cc index cab82af..844e1f9 100644 --- a/spectator/age_gauge_test.cc +++ b/spectator/age_gauge_test.cc @@ -25,4 +25,12 @@ TEST(AgeGauge, Set) { EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(AgeGauge, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + AgeGauge g{id, &publisher}; + EXPECT_EQ("A:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/counter_test.cc b/spectator/counter_test.cc index 2727d54..91267ce 100644 --- a/spectator/counter_test.cc +++ b/spectator/counter_test.cc @@ -13,8 +13,8 @@ TEST(Counter, Activity) { TestPublisher publisher; auto id = std::make_shared("ctr.name", Tags{}); auto id2 = std::make_shared("c2", Tags{{"key", "val"}}); - Counter c{id, &publisher}; - Counter c2{id2, &publisher}; + Counter c{id, &publisher}; + Counter c2{id2, &publisher}; c.Increment(); c2.Add(1.2); c.Add(0.1); @@ -25,10 +25,18 @@ TEST(Counter, Activity) { TEST(Counter, Id) { TestPublisher publisher; - Counter c{std::make_shared("foo", Tags{{"key", "val"}}), + Counter c{std::make_shared("foo", Tags{{"key", "val"}}), &publisher}; auto id = std::make_shared("foo", Tags{{"key", "val"}}); EXPECT_EQ(*(c.MeterId()), *id); } +TEST(Counter, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Counter c{id, &publisher}; + EXPECT_EQ("c:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} } // namespace diff --git a/spectator/dist_summary_test.cc b/spectator/dist_summary_test.cc index 7202b1b..ed8eec5 100644 --- a/spectator/dist_summary_test.cc +++ b/spectator/dist_summary_test.cc @@ -22,4 +22,12 @@ TEST(DistributionSummary, Record) { EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(DistributionSummary, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + DistributionSummary d{id, &publisher}; + EXPECT_EQ("d:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix()); +} } // namespace diff --git a/spectator/gauge_test.cc b/spectator/gauge_test.cc index 014910e..d92e11a 100644 --- a/spectator/gauge_test.cc +++ b/spectator/gauge_test.cc @@ -24,4 +24,12 @@ TEST(Gauge, Set) { EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(Gauge, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Gauge g{id, &publisher}; + EXPECT_EQ("g:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/id.h b/spectator/id.h index 611d2a1..1f351af 100644 --- a/spectator/id.h +++ b/spectator/id.h @@ -17,8 +17,7 @@ class Tags { public: Tags() = default; - Tags( - std::initializer_list> vs) { + Tags(std::initializer_list> vs) { for (auto& pair : vs) { add(pair.first, pair.second); } diff --git a/spectator/id_test.cc b/spectator/id_test.cc index ed2aa1c..35a53ec 100644 --- a/spectator/id_test.cc +++ b/spectator/id_test.cc @@ -8,9 +8,15 @@ using spectator::Tags; TEST(Id, Create) { Id id{"foo", Tags{}}; EXPECT_EQ(id.Name(), "foo"); + EXPECT_EQ(id.GetTags().size(), 0); Id id_tags{"name", Tags{{"k", "v"}, {"k1", "v1"}}}; + EXPECT_EQ(id_tags.Name(), "name"); EXPECT_EQ(id_tags.GetTags().size(), 2); + + std::shared_ptr id_of{Id::of("name", Tags{{"k", "v"}, {"k1", "v1"}})}; + EXPECT_EQ(id_of->Name(), "name"); + EXPECT_EQ(id_of->GetTags().size(), 2); } TEST(Id, Tags) { diff --git a/spectator/max_gauge_test.cc b/spectator/max_gauge_test.cc index f769b4d..10c5bac 100644 --- a/spectator/max_gauge_test.cc +++ b/spectator/max_gauge_test.cc @@ -24,4 +24,12 @@ TEST(MaxGauge, Set) { EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(MaxGauge, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MaxGauge g{id, &publisher}; + EXPECT_EQ("m:test______^____-_~______________.___foo,tag1___=value1___:", g.GetPrefix()); +} } // namespace diff --git a/spectator/monotonic_counter_test.cc b/spectator/monotonic_counter_test.cc index 18dac4b..688d5d7 100644 --- a/spectator/monotonic_counter_test.cc +++ b/spectator/monotonic_counter_test.cc @@ -23,4 +23,13 @@ TEST(MonotonicCounter, Set) { "C:ctr:43"}; EXPECT_EQ(publisher.SentMessages(), expected); } + +TEST(MonotonicCounter, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MonotonicCounter c{id, &publisher}; + EXPECT_EQ("C:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} } // namespace diff --git a/spectator/monotonic_counter_uint_test.cc b/spectator/monotonic_counter_uint_test.cc new file mode 100644 index 0000000..d3f55f2 --- /dev/null +++ b/spectator/monotonic_counter_uint_test.cc @@ -0,0 +1,34 @@ +#include "stateless_meters.h" +#include "test_publisher.h" +#include + +namespace { + +using spectator::Id; +using spectator::MonotonicCounterUint; +using spectator::Tags; +using spectator::TestPublisher; + +TEST(MonotonicCounterUint, Set) { + TestPublisher publisher; + auto id = std::make_shared("ctr", Tags{}); + auto id2 = std::make_shared("ctr2", Tags{{"key", "val"}}); + MonotonicCounterUint c{id, &publisher}; + MonotonicCounterUint c2{id2, &publisher}; + + c.Set(42); + c2.Set(2); + c.Set(-1); + std::vector expected = {"U:ctr:42", "U:ctr2,key=val:2", "U:ctr:18446744073709551615"}; + EXPECT_EQ(publisher.SentMessages(), expected); +} + +TEST(MonotonicCounterUint, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + MonotonicCounterUint c{id, &publisher}; + EXPECT_EQ("U:test______^____-_~______________.___foo,tag1___=value1___:", c.GetPrefix()); +} +} // namespace \ No newline at end of file diff --git a/spectator/perc_dist_summary_test.cc b/spectator/perc_dist_summary_test.cc index 5f9df29..86b730e 100644 --- a/spectator/perc_dist_summary_test.cc +++ b/spectator/perc_dist_summary_test.cc @@ -11,13 +11,21 @@ using spectator::TestPublisher; TEST(PercDistSum, Record) { TestPublisher publisher; auto id = std::make_shared("pds", Tags{}); - PercentileDistributionSummary c{id, &publisher, 0, 1000}; - c.Record(50); - c.Record(5000); - c.Record(-5000); + PercentileDistributionSummary d{id, &publisher, 0, 1000}; + d.Record(50); + d.Record(5000); + d.Record(-5000); std::vector expected = {"D:pds:50", "D:pds:1000", "D:pds:0"}; EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(PercDistSum, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + PercentileDistributionSummary d{id, &publisher, 0, 1000}; + EXPECT_EQ("D:test______^____-_~______________.___foo,tag1___=value1___:", d.GetPrefix()); +} } // namespace diff --git a/spectator/perc_timer_test.cc b/spectator/perc_timer_test.cc index 9efa7de..6301349 100644 --- a/spectator/perc_timer_test.cc +++ b/spectator/perc_timer_test.cc @@ -11,14 +11,20 @@ using spectator::TestPublisher; TEST(PercentileTimer, Record) { TestPublisher publisher; auto id = std::make_shared("pt", Tags{}); - PercentileTimer c{id, &publisher, absl::ZeroDuration(), - absl::Seconds(5)}; + PercentileTimer c{id, &publisher, absl::ZeroDuration(), absl::Seconds(5)}; c.Record(absl::Milliseconds(42)); c.Record(std::chrono::microseconds(500)); c.Record(absl::Seconds(10)); - std::vector expected = {"T:pt:0.042", "T:pt:0.0005", - "T:pt:5"}; + std::vector expected = {"T:pt:0.042", "T:pt:0.0005", "T:pt:5"}; EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(PercentileTimer, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("test`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + PercentileTimer t{id, &publisher, absl::ZeroDuration(), absl::Seconds(5)}; + EXPECT_EQ("T:test______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix()); +} } // namespace diff --git a/spectator/stateless_meters.h b/spectator/stateless_meters.h index 7c4b815..7cf3525 100644 --- a/spectator/stateless_meters.h +++ b/spectator/stateless_meters.h @@ -6,14 +6,39 @@ namespace spectator { namespace detail { + +#include "valid_chars.inc" + inline std::string as_string(std::string_view v) { return {v.data(), v.size()}; } + +inline bool contains_non_atlas_char(const std::string& input) { + return std::any_of(input.begin(), input.end(), [](char c) { return !kAtlasChars[c]; }); +} + +inline std::string replace_invalid_characters(const std::string& input) { + if (contains_non_atlas_char(input)) { + std::string result{input}; + for (char &c : result) { + if (!kAtlasChars[c]) { + c = '_'; + } + } + return result; + } else { + return input; + } +} + inline std::string create_prefix(const Id& id, std::string_view type_name) { - std::string res = as_string(type_name) + ":" + id.Name(); + std::string res = as_string(type_name) + ":" + replace_invalid_characters(id.Name()); for (const auto& tags : id.GetTags()) { - absl::StrAppend(&res, ",", tags.first, "=", tags.second); + auto first = replace_invalid_characters(tags.first); + auto second = replace_invalid_characters(tags.second); + absl::StrAppend(&res, ",", first, "=", second); } + absl::StrAppend(&res, ":"); return res; } @@ -38,6 +63,12 @@ class StatelessMeter { assert(publisher_ != nullptr); } virtual ~StatelessMeter() = default; + std::string GetPrefix() { + if (value_prefix_.empty()) { + value_prefix_ = detail::create_prefix(*id_, Type()); + } + return value_prefix_; + } [[nodiscard]] IdPtr MeterId() const noexcept { return id_; } [[nodiscard]] virtual std::string_view Type() = 0; diff --git a/spectator/timer_test.cc b/spectator/timer_test.cc index ed5f92e..0f380e5 100644 --- a/spectator/timer_test.cc +++ b/spectator/timer_test.cc @@ -12,14 +12,21 @@ TEST(Timer, Record) { TestPublisher publisher; auto id = std::make_shared("t.name", Tags{}); auto id2 = std::make_shared("t2", Tags{{"key", "val"}}); - Timer t{id, &publisher}; - Timer t2{id2, &publisher}; + Timer t{id, &publisher}; + Timer t2{id2, &publisher}; t.Record(std::chrono::milliseconds(1)); t2.Record(absl::Seconds(0.1)); t2.Record(absl::Microseconds(500)); - std::vector expected = { - "t:t.name:0.001", "t:t2,key=val:0.1", "t:t2,key=val:0.0005"}; + std::vector expected = {"t:t.name:0.001", "t:t2,key=val:0.1", "t:t2,key=val:0.0005"}; EXPECT_EQ(publisher.SentMessages(), expected); } +TEST(Timer, InvalidTags) { + TestPublisher publisher; + // test with a single tag, because tags order is not guaranteed in a flat_hash_map + auto id = std::make_shared("timer`!@#$%^&*()-=~_+[]{}\\|;:'\",<.>/?foo", + Tags{{"tag1,:=", "value1,:="}}); + Timer t{id, &publisher}; + EXPECT_EQ("t:timer______^____-_~______________.___foo,tag1___=value1___:", t.GetPrefix()); +} } // namespace diff --git a/tools/gen_valid_chars.cc b/tools/gen_valid_chars.cc new file mode 100644 index 0000000..ab046f0 --- /dev/null +++ b/tools/gen_valid_chars.cc @@ -0,0 +1,48 @@ +// generate the atlas valid charsets + +#include +#include + +void dump_array(std::ostream& os, const std::string& name, const std::array& chars) { + os << "static constexpr std::array " << name << " = {{"; + + os << chars[0]; + for (auto i = 1u; i < chars.size(); ++i) { + os << ", " << chars[i]; + } + + os << "}};\n"; +} + +int main(int argc, char* argv[]) { + std::ofstream of; + if (argc > 1) { + of.open(argv[1]); + } else { + of.open("/dev/stdout"); + } + + // default false + std::array charsAllowed{}; + for (int i = 0; i < 256; ++i) { + charsAllowed[i] = false; + } + + // configure allowed characters + charsAllowed['.'] = true; + charsAllowed['-'] = true; + + for (auto ch = '0'; ch <= '9'; ++ch) { + charsAllowed[ch] = true; + } + for (auto ch = 'a'; ch <= 'z'; ++ch) { + charsAllowed[ch] = true; + } + for (auto ch = 'A'; ch <= 'Z'; ++ch) { + charsAllowed[ch] = true; + } + charsAllowed['~'] = true; + charsAllowed['^'] = true; + + dump_array(of, "kAtlasChars", charsAllowed); +}