From 5314f602349ae3683c5c4dacbc3b459ca32caa01 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 4 Oct 2021 17:17:30 +0000 Subject: [PATCH 1/4] disallow trailing hex letters when parsing trace IDs --- src/propagation.cpp | 26 ++++++++++++++++++++++---- test/propagation_test.cpp | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index 471e7d11..f3b6db9f 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "sample.h" @@ -119,6 +120,23 @@ std::unique_ptr>> enforce_tag_pres return nullptr; } +// Interpret the specified `text` as a non-negative integer formatted in the +// specified `base` (e.g. base 10 for decimal, base 16 for hexadecimal), +// possibly surrounded by whitespace, and return the integer. Throw an +// exception derived from `std::logic_error` if an error occurs. +uint64_t parse_uint64(const std::string& text, int base) { + std::size_t end_index; + const uint64_t result = std::stoull(text, &end_index, base); + + // If any of the remaining characters are not whitespace, then `text` + // contains something other than a base-`base` integer. + if (std::any_of(text.begin() + end_index, text.end(), [](unsigned char ch) { return !std::isspace(ch);})) { + throw std::invalid_argument("integer text field has a trailing non-whitespace character"); + } + + return result; +} + } // namespace std::vector getPropagationHeaderNames(const std::set &styles, @@ -416,8 +434,8 @@ ot::expected> SpanContext::deserialize( std::string trace_id_str = j[json_trace_id_key]; std::string parent_id_str = j[json_parent_id_key]; - trace_id = std::stoull(trace_id_str); - parent_id = std::stoull(parent_id_str); + trace_id = parse_uint64(trace_id_str, 10); + parent_id = parse_uint64(parent_id_str, 10); if (j.find(json_sampling_priority_key) != j.end()) { sampling_priority = asSamplingPriority(j[json_sampling_priority_key]); @@ -483,10 +501,10 @@ ot::expected> SpanContext::deserialize( reader.ForeachKey([&](ot::string_view key, ot::string_view value) -> ot::expected { try { if (equals_ignore_case(key, headers_impl.trace_id_header)) { - trace_id = std::stoull(value, nullptr, headers_impl.base); + trace_id = parse_uint64(value, headers_impl.base); trace_id_set = true; } else if (equals_ignore_case(key, headers_impl.span_id_header)) { - parent_id = std::stoull(value, nullptr, headers_impl.base); + parent_id = parse_uint64(value, headers_impl.base); parent_id_set = true; } else if (equals_ignore_case(key, headers_impl.sampling_priority_header)) { sampling_priority = asSamplingPriority(std::stoi(value)); diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index 737deb3b..cb93ca02 100644 --- a/test/propagation_test.cpp +++ b/test/propagation_test.cpp @@ -97,6 +97,33 @@ TEST_CASE("SpanContext") { REQUIRE(*received_context2 == *received_context); } } + + SECTION("even with leading whitespace in integer fields") { + carrier.Set("x-datadog-trace-id", " 123"); + auto sc = SpanContext::deserialize(logger, carrier, propagation_styles); + REQUIRE(sc); + auto received_context = dynamic_cast(sc->get()); + REQUIRE(received_context); + REQUIRE(received_context->id() == 420); + } + + SECTION("even with trailing whitespace in integer fields") { + carrier.Set("x-datadog-trace-id", "123 "); + auto sc = SpanContext::deserialize(logger, carrier, propagation_styles); + REQUIRE(sc); + auto received_context = dynamic_cast(sc->get()); + REQUIRE(received_context); + REQUIRE(received_context->id() == 420); + } + + SECTION("even with whitespace surrounding integer fields") { + carrier.Set("x-datadog-trace-id", " 123 "); + auto sc = SpanContext::deserialize(logger, carrier, propagation_styles); + REQUIRE(sc); + auto received_context = dynamic_cast(sc->get()); + REQUIRE(received_context); + REQUIRE(received_context->id() == 420); + } } SECTION("can access ids") { REQUIRE(context.ToTraceID() == "123"); @@ -196,6 +223,13 @@ TEST_CASE("deserialize fails") { REQUIRE(!err); REQUIRE(err.error() == ot::span_context_corrupted_error); } + + SECTION("when decimal integer IDs start decimal but have hex characters") { + carrier.Set(test_case.x_datadog_trace_id, "123deadbeef"); + auto err = SpanContext::deserialize(logger, carrier, test_case.styles); + REQUIRE(!err); + REQUIRE(err.error() == ot::span_context_corrupted_error); + } } TEST_CASE("SamplingPriority values are clamped apropriately for b3") { From 0dd9956babb8618c6876d08e0f833e563520a543 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 4 Oct 2021 17:18:14 +0000 Subject: [PATCH 2/4] clang-format --- src/propagation.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/propagation.cpp b/src/propagation.cpp index f3b6db9f..7dfa5d0f 100644 --- a/src/propagation.cpp +++ b/src/propagation.cpp @@ -124,13 +124,14 @@ std::unique_ptr>> enforce_tag_pres // specified `base` (e.g. base 10 for decimal, base 16 for hexadecimal), // possibly surrounded by whitespace, and return the integer. Throw an // exception derived from `std::logic_error` if an error occurs. -uint64_t parse_uint64(const std::string& text, int base) { +uint64_t parse_uint64(const std::string &text, int base) { std::size_t end_index; const uint64_t result = std::stoull(text, &end_index, base); // If any of the remaining characters are not whitespace, then `text` // contains something other than a base-`base` integer. - if (std::any_of(text.begin() + end_index, text.end(), [](unsigned char ch) { return !std::isspace(ch);})) { + if (std::any_of(text.begin() + end_index, text.end(), + [](unsigned char ch) { return !std::isspace(ch); })) { throw std::invalid_argument("integer text field has a trailing non-whitespace character"); } From 3e88dae13c1a2fc0b17d9013b109ee667bd627a1 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 4 Oct 2021 17:27:35 +0000 Subject: [PATCH 3/4] don't mention specific error code in new test --- test/propagation_test.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index cb93ca02..82c12066 100644 --- a/test/propagation_test.cpp +++ b/test/propagation_test.cpp @@ -228,7 +228,6 @@ TEST_CASE("deserialize fails") { carrier.Set(test_case.x_datadog_trace_id, "123deadbeef"); auto err = SpanContext::deserialize(logger, carrier, test_case.styles); REQUIRE(!err); - REQUIRE(err.error() == ot::span_context_corrupted_error); } } From dbb8db1e106f947882c2871a06dead4eada782c8 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 4 Oct 2021 17:38:06 +0000 Subject: [PATCH 4/4] wrong ID in unit test --- test/propagation_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/propagation_test.cpp b/test/propagation_test.cpp index 82c12066..f5b35ce6 100644 --- a/test/propagation_test.cpp +++ b/test/propagation_test.cpp @@ -104,7 +104,7 @@ TEST_CASE("SpanContext") { REQUIRE(sc); auto received_context = dynamic_cast(sc->get()); REQUIRE(received_context); - REQUIRE(received_context->id() == 420); + REQUIRE(received_context->traceId() == 123); } SECTION("even with trailing whitespace in integer fields") { @@ -113,7 +113,7 @@ TEST_CASE("SpanContext") { REQUIRE(sc); auto received_context = dynamic_cast(sc->get()); REQUIRE(received_context); - REQUIRE(received_context->id() == 420); + REQUIRE(received_context->traceId() == 123); } SECTION("even with whitespace surrounding integer fields") { @@ -122,7 +122,7 @@ TEST_CASE("SpanContext") { REQUIRE(sc); auto received_context = dynamic_cast(sc->get()); REQUIRE(received_context); - REQUIRE(received_context->id() == 420); + REQUIRE(received_context->traceId() == 123); } } SECTION("can access ids") {