diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc index 8170554e96..8a1fbed7dc 100644 --- a/audio/audio_receive_stream.cc +++ b/audio/audio_receive_stream.cc @@ -30,7 +30,8 @@ namespace webrtc { std::string AudioReceiveStream::Config::Rtp::ToString() const { - rtc::SimpleStringBuilder<1024> ss; + char ss_buf[1024]; + rtc::SimpleStringBuilder ss(ss_buf); ss << "{remote_ssrc: " << remote_ssrc; ss << ", local_ssrc: " << local_ssrc; ss << ", transport_cc: " << (transport_cc ? "on" : "off"); @@ -48,7 +49,8 @@ std::string AudioReceiveStream::Config::Rtp::ToString() const { } std::string AudioReceiveStream::Config::ToString() const { - rtc::SimpleStringBuilder<1024> ss; + char ss_buf[1024]; + rtc::SimpleStringBuilder ss(ss_buf); ss << "{rtp: " << rtp.ToString(); ss << ", rtcp_send_transport: " << (rtcp_send_transport ? "(Transport)" : "null"); diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 7e6a23da57..b4f2c1755b 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -142,12 +142,15 @@ rtc_source_set("safe_minmax") { rtc_source_set("stringutils") { sources = [ + "strings/string_builder.cc", "strings/string_builder.h", "stringutils.cc", "stringutils.h", ] deps = [ ":checks", + ":safe_minmax", + "../api:array_view", ] } diff --git a/rtc_base/logging.cc b/rtc_base/logging.cc index 4798f4dbec..ec8cd7ff4a 100644 --- a/rtc_base/logging.cc +++ b/rtc_base/logging.cc @@ -127,7 +127,8 @@ LogMessage::LogMessage(const char* file, print_stream_ << "(" << FilenameFromPath(file) << ":" << line << "): "; if (err_ctx != ERRCTX_NONE) { - SimpleStringBuilder<1024> tmp; + char tmp_buf[1024]; + SimpleStringBuilder tmp(tmp_buf); tmp.AppendFormat("[0x%08X]", err); switch (err_ctx) { case ERRCTX_ERRNO: diff --git a/rtc_base/strings/string_builder.cc b/rtc_base/strings/string_builder.cc new file mode 100644 index 0000000000..499fb9a25b --- /dev/null +++ b/rtc_base/strings/string_builder.cc @@ -0,0 +1,21 @@ +/* + * Copyright 2018 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "rtc_base/strings/string_builder.h" + +namespace rtc { + +SimpleStringBuilder::SimpleStringBuilder(rtc::ArrayView buffer) + : buffer_(buffer) { + buffer_[0] = '\0'; + RTC_DCHECK(IsConsistent()); +} + +} // namespace rtc diff --git a/rtc_base/strings/string_builder.h b/rtc_base/strings/string_builder.h index 15951e0bf7..b450a995b0 100644 --- a/rtc_base/strings/string_builder.h +++ b/rtc_base/strings/string_builder.h @@ -12,22 +12,24 @@ #define RTC_BASE_STRINGS_STRING_BUILDER_H_ #include +#include #include +#include "api/array_view.h" #include "rtc_base/checks.h" +#include "rtc_base/numerics/safe_minmax.h" #include "rtc_base/stringutils.h" namespace rtc { -// This is a minimalistic string builder class meant to cover the most cases -// of when you might otherwise be tempted to use a stringstream (discouraged -// for anything except logging). -// This class allocates a fixed size buffer on the stack and concatenates -// strings and numbers into it, allowing the results to be read via |str()|. -template +// This is a minimalistic string builder class meant to cover the most cases of +// when you might otherwise be tempted to use a stringstream (discouraged for +// anything except logging). It uses a fixed-size buffer provided by the caller +// and concatenates strings and numbers into it, allowing the results to be +// read via |str()|. class SimpleStringBuilder { public: - SimpleStringBuilder() { buffer_[0] = '\0'; } + explicit SimpleStringBuilder(rtc::ArrayView buffer); SimpleStringBuilder(const SimpleStringBuilder&) = delete; SimpleStringBuilder& operator=(const SimpleStringBuilder&) = delete; @@ -80,48 +82,60 @@ class SimpleStringBuilder { // Returns a pointer to the built string. The name |str()| is borrowed for // compatibility reasons as we replace usage of stringstream throughout the // code base. - const char* str() const { return &buffer_[0]; } + const char* str() const { return buffer_.data(); } // Returns the length of the string. The name |size()| is picked for STL // compatibility reasons. size_t size() const { return size_; } // Allows appending a printf style formatted string. - SimpleStringBuilder& AppendFormat(const char* fmt, ...) { +#if defined(__GNUC__) + __attribute__((__format__(__printf__, 2, 3))) +#endif + SimpleStringBuilder& + AppendFormat(const char* fmt, ...) { va_list args; va_start(args, fmt); - int len = std::vsnprintf(&buffer_[size_], buffer_size - size_, fmt, args); - RTC_DCHECK_GE(len, 0); - // Negative values are likely programmer error, but let's not update the - // length if so. - if (len > 0) - AddToLength(len); + const int len = + std::vsnprintf(&buffer_[size_], buffer_.size() - size_, fmt, args); + if (len >= 0) { + const size_t chars_added = rtc::SafeMin(len, buffer_.size() - 1 - size_); + size_ += chars_added; + RTC_DCHECK_EQ(len, chars_added) << "Buffer size was insufficient"; + } else { + // This should never happen, but we're paranoid, so re-write the + // terminator in case vsnprintf() overwrote it. + RTC_NOTREACHED(); + buffer_[size_] = '\0'; + } va_end(args); + RTC_DCHECK(IsConsistent()); return *this; } // An alternate way from operator<<() to append a string. This variant is // slightly more efficient when the length of the string to append, is known. SimpleStringBuilder& Append(const char* str, size_t length = SIZE_UNKNOWN) { - AddToLength( - rtc::strcpyn(&buffer_[size_], buffer_size - size_, str, length)); + const size_t chars_added = + rtc::strcpyn(&buffer_[size_], buffer_.size() - size_, str, length); + size_ += chars_added; + RTC_DCHECK_EQ(chars_added, + length == SIZE_UNKNOWN ? std::strlen(str) : length) + << "Buffer size was insufficient"; + RTC_DCHECK(IsConsistent()); return *this; } private: - void AddToLength(size_t chars_added) { - size_ += chars_added; - RTC_DCHECK_EQ('\0', buffer_[size_]); - RTC_DCHECK_LE(size_, buffer_size - 1) - << "Buffer size limit reached (" << buffer_size << ")"; + bool IsConsistent() const { + return size_ <= buffer_.size() - 1 && buffer_[size_] == '\0'; } - // An always-zero-terminated fixed buffer that we write to. - // Assuming the SimpleStringBuilder instance lives on the stack, this - // buffer will be stack allocated, which is done for performance reasons. + // An always-zero-terminated fixed-size buffer that we write to. The fixed + // size allows the buffer to be stack allocated, which helps performance. // Having a fixed size is furthermore useful to avoid unnecessary resizing // while building it. - char buffer_[buffer_size]; // NOLINT + const rtc::ArrayView buffer_; // Represents the number of characters written to the buffer. // This does not include the terminating '\0'. diff --git a/rtc_base/strings/string_builder_unittest.cc b/rtc_base/strings/string_builder_unittest.cc index 0403ab8529..8d6312f947 100644 --- a/rtc_base/strings/string_builder_unittest.cc +++ b/rtc_base/strings/string_builder_unittest.cc @@ -11,13 +11,15 @@ #include "rtc_base/strings/string_builder.h" #include "rtc_base/checks.h" -#include "rtc_base/gunit.h" #include "rtc_base/stringutils.h" +#include "test/gmock.h" +#include "test/gtest.h" namespace rtc { TEST(SimpleStringBuilder, Limit) { - SimpleStringBuilder<10> sb; + char sb_buf[10]; + SimpleStringBuilder sb(sb_buf); EXPECT_EQ(0u, strlen(sb.str())); // Test that for a SSB with a buffer size of 10, that we can write 9 chars @@ -27,26 +29,115 @@ TEST(SimpleStringBuilder, Limit) { } TEST(SimpleStringBuilder, NumbersAndChars) { - SimpleStringBuilder<100> sb; + char sb_buf[100]; + SimpleStringBuilder sb(sb_buf); sb << 1 << ':' << 2.1 << ":" << 2.2f << ':' << 78187493520ll << ':' << 78187493520ul; EXPECT_EQ(0, strcmp(sb.str(), "1:2.100000:2.200000:78187493520:78187493520")); } TEST(SimpleStringBuilder, Format) { - SimpleStringBuilder<100> sb; + char sb_buf[100]; + SimpleStringBuilder sb(sb_buf); sb << "Here we go - "; - sb.AppendFormat("This is a hex formatted value: 0x%08x", 3735928559); + sb.AppendFormat("This is a hex formatted value: 0x%08llx", 3735928559ULL); EXPECT_EQ(0, strcmp(sb.str(), "Here we go - This is a hex formatted value: 0xdeadbeef")); } TEST(SimpleStringBuilder, StdString) { - SimpleStringBuilder<100> sb; + char sb_buf[100]; + SimpleStringBuilder sb(sb_buf); std::string str = "does this work?"; sb << str; EXPECT_EQ(str, sb.str()); } +// These tests are safe to run if we have death test support or if DCHECKs are +// off. +#if (GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)) || !RTC_DCHECK_IS_ON + +TEST(SimpleStringBuilder, BufferOverrunConstCharP) { + char sb_buf[4]; + SimpleStringBuilder sb(sb_buf); + const char* const msg = "This is just too much"; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << msg, ""); +#else + sb << msg; + EXPECT_THAT(sb.str(), testing::StrEq("Thi")); +#endif +} + +TEST(SimpleStringBuilder, BufferOverrunStdString) { + char sb_buf[4]; + SimpleStringBuilder sb(sb_buf); + sb << 12; + const std::string msg = "Aw, come on!"; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << msg, ""); +#else + sb << msg; + EXPECT_THAT(sb.str(), testing::StrEq("12A")); +#endif +} + +TEST(SimpleStringBuilder, BufferOverrunInt) { + char sb_buf[4]; + SimpleStringBuilder sb(sb_buf); + constexpr int num = -12345; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << num, ""); +#else + sb << num; + // If we run into the end of the buffer, resonable results are either that + // the append has no effect or that it's truncated at the point where the + // buffer ends. + EXPECT_THAT(sb.str(), + testing::AnyOf(testing::StrEq(""), testing::StrEq("-12"))); +#endif +} + +TEST(SimpleStringBuilder, BufferOverrunDouble) { + char sb_buf[5]; + SimpleStringBuilder sb(sb_buf); + constexpr double num = 123.456; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << num, ""); +#else + sb << num; + EXPECT_THAT(sb.str(), + testing::AnyOf(testing::StrEq(""), testing::StrEq("123."))); +#endif +} + +TEST(SimpleStringBuilder, BufferOverrunConstCharPAlreadyFull) { + char sb_buf[4]; + SimpleStringBuilder sb(sb_buf); + sb << 123; + const char* const msg = "This is just too much"; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << msg, ""); +#else + sb << msg; + EXPECT_THAT(sb.str(), testing::StrEq("123")); +#endif +} + +TEST(SimpleStringBuilder, BufferOverrunIntAlreadyFull) { + char sb_buf[4]; + SimpleStringBuilder sb(sb_buf); + sb << "xyz"; + constexpr int num = -12345; +#if RTC_DCHECK_IS_ON + EXPECT_DEATH(sb << num, ""); +#else + sb << num; + EXPECT_THAT(sb.str(), testing::StrEq("xyz")); +#endif +} + +#endif + } // namespace rtc diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index f507a2383c..62a68ed5ef 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -64,7 +64,8 @@ const char* UmaPrefixForContentType(VideoContentType content_type) { } std::string UmaSuffixForContentType(VideoContentType content_type) { - rtc::SimpleStringBuilder<1024> ss; + char ss_buf[1024]; + rtc::SimpleStringBuilder ss(ss_buf); int simulcast_id = videocontenttypehelpers::GetSimulcastId(content_type); if (simulcast_id > 0) { ss << ".S" << simulcast_id - 1; @@ -135,7 +136,8 @@ ReceiveStatisticsProxy::~ReceiveStatisticsProxy() { void ReceiveStatisticsProxy::UpdateHistograms() { RTC_DCHECK_RUN_ON(&decode_thread_); - rtc::SimpleStringBuilder<8 * 1024> log_stream; + char log_stream_buf[8 * 1024]; + rtc::SimpleStringBuilder log_stream(log_stream_buf); int stream_duration_sec = (clock_->TimeInMilliseconds() - start_ms_) / 1000; if (stats_.frame_counts.key_frames > 0 || stats_.frame_counts.delta_frames > 0) { diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc index 36b63313e6..8f36f46ea7 100644 --- a/video/send_statistics_proxy.cc +++ b/video/send_statistics_proxy.cc @@ -268,7 +268,8 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms( RTC_DCHECK(uma_prefix_ == kRealtimePrefix || uma_prefix_ == kScreenPrefix); const int kIndex = uma_prefix_ == kScreenPrefix ? 1 : 0; const int kMinRequiredPeriodicSamples = 6; - rtc::SimpleStringBuilder<8 * 1024> log_stream; + char log_stream_buf[8 * 1024]; + rtc::SimpleStringBuilder log_stream(log_stream_buf); int in_width = input_width_counter_.Avg(kMinRequiredMetricsSamples); int in_height = input_height_counter_.Avg(kMinRequiredMetricsSamples); if (in_width != -1) {