Skip to content

Commit 7c1e77c

Browse files
denizariyandenizariyan
authored
[CODE HEALTH] Prevent std::out_of_range escaping noexcept substr callers (#4562)
1 parent 0f78631 commit 7c1e77c

7 files changed

Lines changed: 108 additions & 18 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ Increment the:
1515

1616
## [Unreleased]
1717

18+
* [CODE HEALTH] Prevent `std::out_of_range` escaping `noexcept` substr callers.
19+
[#4562](https://github.com/open-telemetry/opentelemetry-cpp/pull/4562)
20+
1821
* [METRICS SDK] Enforce a runtime minimum scale of `-11` for
1922
`Base2ExponentialHistogramAggregation`, so a recording that spans the full
2023
double range no longer downscales without end.

‎api/include/opentelemetry/baggage/baggage.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,8 +138,8 @@ class OPENTELEMETRY_EXPORT Baggage
138138
auto metadata_separator = value.find(kMetadataSeparator);
139139
if (metadata_separator != std::string::npos)
140140
{
141-
metadata = value.substr(metadata_separator);
142-
value = value.substr(0, metadata_separator);
141+
metadata = common::StringUtil::Substr(value, metadata_separator);
142+
value = common::StringUtil::Substr(value, 0, metadata_separator);
143143
}
144144

145145
bool err = false;

‎api/include/opentelemetry/common/kv_properties.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,7 @@ class KeyValueStringTokenizer
3636
: str_(str), opts_(opts)
3737
{}
3838

39-
static nostd::string_view GetDefaultKeyOrValue()
40-
{
41-
static std::string default_str = "";
42-
return default_str;
43-
}
39+
static nostd::string_view GetDefaultKeyOrValue() { return ""; }
4440

4541
// Returns next key value in the string header
4642
// @param valid_kv : if the found kv pair is valid or not
@@ -91,8 +87,8 @@ class KeyValueStringTokenizer
9187
}
9288
else
9389
{
94-
key = list_member.substr(0, key_end_pos);
95-
value = list_member.substr(key_end_pos + 1);
90+
key = StringUtil::Substr(list_member, 0, key_end_pos);
91+
value = StringUtil::Substr(list_member, key_end_pos + 1);
9692
}
9793

9894
index_ = end + 2;

‎api/include/opentelemetry/common/string_util.h‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@ namespace common
2121
class StringUtil
2222
{
2323
public:
24-
static nostd::string_view Trim(nostd::string_view str, size_t left, size_t right) noexcept
24+
static nostd::string_view Trim(nostd::string_view str OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND,
25+
size_t left,
26+
size_t right) noexcept
2527
{
2628
if (right >= str.size())
2729
{
@@ -53,7 +55,26 @@ class StringUtil
5355
#endif
5456
}
5557

56-
static nostd::string_view Trim(nostd::string_view str) noexcept
58+
static nostd::string_view Substr(nostd::string_view str OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND,
59+
size_t pos,
60+
size_t count = nostd::string_view::npos) noexcept
61+
{
62+
#if OPENTELEMETRY_HAVE_EXCEPTIONS
63+
try
64+
#endif
65+
{
66+
return str.substr(pos, count);
67+
}
68+
#if OPENTELEMETRY_HAVE_EXCEPTIONS
69+
catch (const std::out_of_range &)
70+
{
71+
return nostd::string_view();
72+
}
73+
#endif
74+
}
75+
76+
static nostd::string_view Trim(
77+
nostd::string_view str OPENTELEMETRY_ATTRIBUTE_LIFETIME_BOUND) noexcept
5778
{
5879
if (str.empty())
5980
{

‎api/include/opentelemetry/trace/propagation/detail/string.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
#pragma once
55

6+
#include "opentelemetry/common/string_util.h"
67
#include "opentelemetry/nostd/string_view.h"
78
#include "opentelemetry/version.h"
89

@@ -39,7 +40,7 @@ inline size_t SplitString(nostd::string_view s,
3940
continue;
4041
}
4142

42-
results[filled++] = s.substr(token_start, i - token_start);
43+
results[filled++] = common::StringUtil::Substr(s, token_start, i - token_start);
4344

4445
if (filled == count)
4546
{
@@ -51,7 +52,7 @@ inline size_t SplitString(nostd::string_view s,
5152

5253
if (filled < count)
5354
{
54-
results[filled++] = s.substr(token_start);
55+
results[filled++] = common::StringUtil::Substr(s, token_start);
5556
}
5657

5758
return filled;

‎api/test/common/string_util_test.cc‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33

44
#include <gtest/gtest.h>
55
#include <cstring>
6+
#include <ostream>
67
#include <string>
78

9+
#include <opentelemetry/common/macros.h>
810
#include <opentelemetry/common/string_util.h>
911

1012
// ------------------------- StringUtil class tests ---------------------------------
@@ -56,3 +58,69 @@ TEST(StringUtilTest, TrimStringOutOfRange)
5658
{
5759
EXPECT_EQ(StringUtil::Trim("x", 2, 1), "");
5860
}
61+
62+
struct SubstrTestData
63+
{
64+
const char *input;
65+
size_t pos;
66+
size_t count;
67+
68+
friend void PrintTo(const SubstrTestData &data, std::ostream *os)
69+
{
70+
*os << "(\"" << data.input << "\"," << data.pos << "," << data.count << ")";
71+
}
72+
};
73+
74+
const SubstrTestData substr_in_range_test_cases[] = {
75+
{"k1=v1", 0, std::string::npos},
76+
{"k1=v1", 3, std::string::npos},
77+
{"k1=v1", 0, 2},
78+
{"k1=v1", 3, 100},
79+
{"k1=v1", 2, 0},
80+
{"k1=v1", 5, std::string::npos},
81+
{"x", 0, std::string::npos},
82+
{"x", 1, std::string::npos},
83+
{"", 0, std::string::npos},
84+
{"", 0, 0},
85+
};
86+
87+
using SubstrTestFixture = ::testing::TestWithParam<SubstrTestData>;
88+
89+
TEST_P(SubstrTestFixture, MatchesStdSubstr)
90+
{
91+
const SubstrTestData test_param = GetParam();
92+
const std::string input(test_param.input);
93+
94+
EXPECT_EQ(StringUtil::Substr(input, test_param.pos, test_param.count),
95+
input.substr(test_param.pos, test_param.count));
96+
}
97+
98+
INSTANTIATE_TEST_SUITE_P(SubstrTestCases,
99+
SubstrTestFixture,
100+
::testing::ValuesIn(substr_in_range_test_cases));
101+
102+
const SubstrTestData substr_out_of_range_test_cases[] = {
103+
{"k1=v1", 6, std::string::npos},
104+
{"k1=v1", 100, 2},
105+
{"k1=v1", std::string::npos, std::string::npos},
106+
{"x", 2, std::string::npos},
107+
{"", 1, std::string::npos},
108+
};
109+
110+
using SubstrOutOfRangeTestFixture = ::testing::TestWithParam<SubstrTestData>;
111+
112+
TEST_P(SubstrOutOfRangeTestFixture, HandlesPosPastTheEnd)
113+
{
114+
const SubstrTestData test_param = GetParam();
115+
116+
#if OPENTELEMETRY_HAVE_EXCEPTIONS
117+
EXPECT_EQ(StringUtil::Substr(test_param.input, test_param.pos, test_param.count), "");
118+
#else
119+
// Without exceptions the underlying calls std::terminate(), so helper cannot recover.
120+
EXPECT_DEATH({ StringUtil::Substr(test_param.input, test_param.pos, test_param.count); }, "");
121+
#endif
122+
}
123+
124+
INSTANTIATE_TEST_SUITE_P(SubstrOutOfRangeTestCases,
125+
SubstrOutOfRangeTestFixture,
126+
::testing::ValuesIn(substr_out_of_range_test_cases));

‎ext/src/http/client/curl/http_operation_curl.cc‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
# include <cctype>
3232
#endif
3333

34+
#include "opentelemetry/common/string_util.h"
3435
#include "opentelemetry/ext/http/client/curl/http_client_curl.h"
3536
#include "opentelemetry/ext/http/client/curl/http_operation_curl.h"
3637
#include "opentelemetry/ext/http/client/curl/http_time_util.h"
@@ -810,19 +811,19 @@ int HttpOperation::CurlLoggerCallback(const CURL * /* handle */,
810811

811812
if (!text_to_log.empty() && text_to_log[size - 1] == '\n')
812813
{
813-
text_to_log = text_to_log.substr(0, size - 1);
814+
text_to_log = common::StringUtil::Substr(text_to_log, 0, size - 1);
814815
}
815816

816817
if (type == CURLINFO_TEXT)
817818
{
818819
static const auto kTlsInfo = nostd::string_view("SSL connection using");
819820
static const auto kFailureMsg = nostd::string_view("Recv failure:");
820821

821-
if (text_to_log.substr(0, kTlsInfo.size()) == kTlsInfo)
822+
if (common::StringUtil::Substr(text_to_log, 0, kTlsInfo.size()) == kTlsInfo)
822823
{
823824
OTEL_INTERNAL_LOG_INFO(text_to_log);
824825
}
825-
else if (text_to_log.substr(0, kFailureMsg.size()) == kFailureMsg)
826+
else if (common::StringUtil::Substr(text_to_log, 0, kFailureMsg.size()) == kFailureMsg)
826827
{
827828
OTEL_INTERNAL_LOG_ERROR(text_to_log);
828829
}
@@ -848,8 +849,8 @@ int HttpOperation::CurlLoggerCallback(const CURL * /* handle */,
848849

849850
if (pos != nostd::string_view::npos)
850851
{
851-
OTEL_INTERNAL_LOG_DEBUG(kHeaderSent << text_to_log.substr(0, pos - 1));
852-
text_to_log = text_to_log.substr(pos + 1);
852+
OTEL_INTERNAL_LOG_DEBUG(kHeaderSent << common::StringUtil::Substr(text_to_log, 0, pos - 1));
853+
text_to_log = common::StringUtil::Substr(text_to_log, pos + 1);
853854
}
854855
}
855856
}

0 commit comments

Comments
 (0)