Skip to content

Commit b8fcb57

Browse files
Damien MEHALAdmehala
Damien MEHALA
authored and
dmehala
committed
code review
1 parent 505ca62 commit b8fcb57

File tree

3 files changed

+127
-26
lines changed

3 files changed

+127
-26
lines changed

‎examples/baggage/main.cpp‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,12 @@ std::ostream& operator<<(std::ostream& os, dd::Baggage::Error error) {
2828
case Baggage::Error::MISSING_HEADER:
2929
os << "missing `baggage` header";
3030
break;
31-
case Baggage::Error::MALFORMED_BAGGAGE_HEADER:
31+
case Baggage::Error::MALFORMED_BAGGAGE_HEADER: {
3232
os << "malformed `baggage` header";
33-
break;
33+
if (error.pos) {
34+
os << " at position " << *error.pos;
35+
}
36+
} break;
3437
case Baggage::Error::MAXIMUM_CAPACITY_REACHED:
3538
os << "maximum number of bagge items reached";
3639
break;

‎src/datadog/baggage.cpp‎

Lines changed: 97 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,41 @@ namespace tracing {
55

66
namespace {
77

8+
/// Whitespace in RFC 7230 section 3.2.3 definition
9+
/// BDNF:
10+
/// - OWS = *(SP / HTAB)
11+
/// - SP = SPACE (0x20)
12+
/// - HTAB = Horizontal tab (0x09)
13+
constexpr bool is_whitespace(char c) { return c == 0x20 || c == 0x09; }
14+
15+
constexpr bool is_allowed_key_char(char c) {
16+
// clang-format off
17+
return (c >= 0x30 && c <= 0x39) ///< [0-9]
18+
|| (c >= 0x41 && c <= 0x5A) ///< [a-z]
19+
|| (c >= 0x61 && c <= 0x7A) ///< [A-Z]
20+
|| (c == 0x21) ///< "!"
21+
|| (c >= 0x23 && c <= 0x27) ///< "#" / "$" / "%" / "&" / "'"
22+
|| (c == 0x2A) ///< "*"
23+
|| (c == 0x2B) ///< "+"
24+
|| (c == 0x2D) ///< "-"
25+
|| (c == 0x2E) ///< "."
26+
|| (c == 0x5E) ///< "^"
27+
|| (c == 0x5F) ///< "_"
28+
|| (c == 0x60) ///< "`"
29+
|| (c == 0x7C) ///< "|"
30+
|| (c == 0x7E); ///< "~"
31+
// clang-format on
32+
}
33+
34+
constexpr bool is_allowed_value_char(char c) {
35+
// clang-format off
36+
return (c == 0x21) ///< "!"
37+
|| (c >= 0x23 && c <= 0x2B) ///< "#" / "$" / "%" / "&" / "'" / "(" / /< ")" / "*" / "+" / "," / "-"
38+
|| (c >= 0x2D && c <= 0x5B) ///< "-" / "." / "/" / [0-9] / ";' / "<" / "=" / ">" / "?" / "@" / [A-Z]
39+
|| (c >= 0x5D && c <= 0x7E); ///< "]" / "^" / "_" / "`" / [a-z]
40+
// clang-format on
41+
}
42+
843
Expected<std::unordered_map<std::string, std::string>, Baggage::Error>
944
parse_baggage(StringView input) {
1045
std::unordered_map<std::string, std::string> result;
@@ -13,8 +48,10 @@ parse_baggage(StringView input) {
1348
enum class state : char {
1449
leading_spaces_key,
1550
key,
51+
trailing_spaces_key,
1652
leading_spaces_value,
17-
value
53+
value,
54+
trailing_spaces_value,
1855
} internal_state = state::leading_spaces_key;
1956

2057
size_t beg = 0;
@@ -26,55 +63,97 @@ parse_baggage(StringView input) {
2663
const size_t end = input.size();
2764

2865
for (size_t i = 0; i < end; ++i) {
66+
auto c = input[i];
67+
2968
switch (internal_state) {
3069
case state::leading_spaces_key: {
31-
if (input[i] != ' ') {
70+
if (!is_whitespace(c)) {
3271
beg = i;
3372
tmp_end = i;
3473
internal_state = state::key;
74+
goto key;
3575
}
3676
} break;
3777

3878
case state::key: {
39-
if (input[i] == ',') {
79+
key:
80+
if (c == '=') {
81+
tmp_end = i;
82+
goto consume_key;
83+
} else if (is_whitespace(c)) {
84+
tmp_end = i;
85+
internal_state = state::trailing_spaces_key;
86+
} else if (!is_allowed_key_char(c)) {
4087
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, i};
41-
} else if (input[i] == '=') {
42-
key = StringView{input.data() + beg, tmp_end - beg + 1};
88+
}
89+
} break;
90+
91+
case state::trailing_spaces_key: {
92+
if (c == '=') {
93+
consume_key:
94+
size_t count = tmp_end - beg;
95+
if (count < 1)
96+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, i};
97+
98+
key = StringView{input.data() + beg, count};
4399
internal_state = state::leading_spaces_value;
44-
} else if (input[i] != ' ') {
45-
tmp_end = i;
100+
} else if (!is_whitespace(c)) {
101+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, i};
46102
}
47103
} break;
48104

49105
case state::leading_spaces_value: {
50-
if (input[i] != ' ') {
106+
if (!is_whitespace(c)) {
51107
beg = i;
52108
tmp_end = i;
53109
internal_state = state::value;
110+
goto value;
54111
}
55112
} break;
56113

57114
case state::value: {
58-
if (input[i] == ',') {
59-
value = StringView{input.data() + beg, tmp_end - beg + 1};
115+
value:
116+
if (c == ',') {
117+
tmp_end = i;
118+
goto consume_value;
119+
} else if (is_whitespace(c)) {
120+
tmp_end = i;
121+
internal_state = state::trailing_spaces_value;
122+
} else if (!is_allowed_value_char(c)) {
123+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, i};
124+
}
125+
} break;
126+
127+
case state::trailing_spaces_value: {
128+
if (c == ',') {
129+
consume_value:
130+
size_t count = tmp_end - beg;
131+
if (count < 1)
132+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER,
133+
tmp_end};
134+
135+
value = StringView{input.data() + beg, count};
60136
result.emplace(std::string(key), std::string(value));
61137
beg = i;
62138
tmp_end = i;
63139
internal_state = state::leading_spaces_key;
64-
} else if (input[i] != ' ') {
65-
tmp_end = i;
140+
} else if (!is_whitespace(c)) {
141+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, i};
66142
}
67-
} break;
143+
}
68144
}
69145
}
70146

71-
if (internal_state != state::value) {
72-
return {Baggage::Error::MALFORMED_BAGGAGE_HEADER};
147+
if (internal_state == state::value) {
148+
value = StringView{input.data() + beg, end - beg};
149+
result.emplace(std::string(key), std::string(value));
150+
} else if (internal_state == state::trailing_spaces_value) {
151+
value = StringView{input.data() + beg, tmp_end - beg};
152+
result.emplace(std::string(key), std::string(value));
153+
} else {
154+
return Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER, end};
73155
}
74156

75-
value = StringView{input.data() + beg, tmp_end - beg + 1};
76-
result.emplace(std::string(key), std::string(value));
77-
78157
return result;
79158
}
80159

‎test/test_baggage.cpp‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "catch.hpp"
44
#include "mocks/dict_readers.h"
55
#include "mocks/dict_writers.h"
6+
#include "random.h"
67

78
#define BAGGAGE_TEST(x) TEST_CASE(x, "[baggage]")
89

@@ -33,7 +34,7 @@ BAGGAGE_TEST("extract") {
3334
" ",
3435
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
3536
},
36-
/*{
37+
{
3738
"valid",
3839
"key1=value1,key2=value2",
3940
Baggage({{"key1", "value1"}, {"key2", "value2"}}),
@@ -84,9 +85,14 @@ BAGGAGE_TEST("extract") {
8485
Baggage({{"key1", "value1"}, {"key2", "value2"}}),
8586
},
8687
{
87-
"spaces in key is allowed",
88+
"spaces in key is not allowed",
8889
"key1 foo=value1",
89-
Baggage({{"key1 foo", "value1"}}),
90+
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
91+
},
92+
{
93+
"spaces in value is not allowed",
94+
"key1=value1 value2",
95+
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
9096
},
9197
{
9298
"verify separator",
@@ -112,7 +118,17 @@ BAGGAGE_TEST("extract") {
112118
"malformed baggage 4",
113119
"key1=value1,=",
114120
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
115-
},*/
121+
},
122+
{
123+
"malformed baggage 5",
124+
"key1=value1,key2=",
125+
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
126+
},
127+
{
128+
"malformed baggage 6",
129+
"key1=",
130+
Baggage::Error{Baggage::Error::MALFORMED_BAGGAGE_HEADER},
131+
},
116132
}));
117133

118134
CAPTURE(test_case.name, test_case.input);
@@ -147,11 +163,14 @@ BAGGAGE_TEST("inject") {
147163

148164
SECTION("default limits are respected") {
149165
auto default_opts = Baggage::default_options;
150-
SECTION("max items") {
166+
SECTION("max items reached") {
151167
Baggage bag;
152168
for (size_t i = 0; i < default_opts.max_items; ++i) {
153-
bag.set("a", "a");
169+
bag.set(uuid(), "a");
154170
}
171+
// NOTE(@dmehala): if that fails, the flackiness is comming from UUIDs
172+
// collision.
173+
REQUIRE(bag.size() == default_opts.max_items);
155174
bag.set("a", "a");
156175

157176
MockDictWriter writer;

0 commit comments

Comments
 (0)