Harden SimpleStringBuilder safety checks Replace RTC_DCHECK with RTC_CHECK in SimpleStringBuilder to ensure safety constraints are enforced in all build configurations, including release builds. This change prevents undefined behavior or silent truncation by crashing when a buffer overflow is detected, rather than only performing these checks in debug modes. The modifications include: * Updating SimpleStringBuilder to use RTC_CHECK for consistency and bounds verification. * Simplifying RtpExtension::ToString and VideoReceiveStream2 by migrating from SimpleStringBuilder to StringBuilder. * Simplify+clarify the StringBuilder::Release implementation. * Update unit tests to reflect that overflow now consistently results in a crash. Bug: chromium:486536241 Change-Id: Ia067508e2b9409154ad4afdbe4c0971a1dd32e1e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/459441 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Per Kjellander <perkj@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47251}
diff --git a/api/rtp_parameters.cc b/api/rtp_parameters.cc index 3a36bfe..b6b2f89 100644 --- a/api/rtp_parameters.cc +++ b/api/rtp_parameters.cc
@@ -18,7 +18,6 @@ #include "absl/strings/ascii.h" #include "absl/strings/string_view.h" -#include "api/array_view.h" #include "api/rtc_error.h" #include "api/rtp_transceiver_direction.h" #include "media/base/media_constants.h" @@ -206,15 +205,14 @@ RtpParameters::~RtpParameters() = default; std::string RtpExtension::ToString() const { - char buf[256]; - SimpleStringBuilder sb(buf); + StringBuilder sb; sb << "{uri: " << uri; sb << ", id: " << id; if (encrypt) { sb << ", encrypt"; } - sb << '}'; - return sb.str(); + sb << "}"; + return sb.Release(); } bool RtpExtension::IsSupportedForAudio(absl::string_view uri) {
diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index b98dcd6..29888f0 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn
@@ -2184,6 +2184,7 @@ "network:received_packet", "synchronization:mutex", "task_utils:repeating_task", + "//testing/gtest", "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/base:core_headers", "//third_party/abseil-cpp/absl/memory",
diff --git a/rtc_base/strings/string_builder.cc b/rtc_base/strings/string_builder.cc index f52445a..af8e7e8 100644 --- a/rtc_base/strings/string_builder.cc +++ b/rtc_base/strings/string_builder.cc
@@ -24,7 +24,7 @@ SimpleStringBuilder::SimpleStringBuilder(std::span<char> buffer) : buffer_(buffer) { buffer_[0] = '\0'; - RTC_DCHECK(IsConsistent()); + RTC_CHECK(IsConsistent()); } SimpleStringBuilder& SimpleStringBuilder::operator<<(char ch) { @@ -32,13 +32,13 @@ } SimpleStringBuilder& SimpleStringBuilder::operator<<(absl::string_view str) { - RTC_DCHECK_LT(size_ + str.length(), buffer_.size()) + RTC_CHECK_LT(size_ + str.length(), buffer_.size()) << "Buffer size was insufficient"; const size_t chars_added = SafeMin(str.length(), buffer_.size() - size_ - 1); memcpy(&buffer_[size_], str.data(), chars_added); size_ += chars_added; buffer_[size_] = '\0'; - RTC_DCHECK(IsConsistent()); + RTC_CHECK(IsConsistent()); return *this; } @@ -98,7 +98,7 @@ if (len >= 0) { const size_t chars_added = SafeMin(len, buffer_.size() - 1 - size_); size_ += chars_added; - RTC_DCHECK_EQ(len, chars_added) << "Buffer size was insufficient"; + RTC_CHECK_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. @@ -106,7 +106,7 @@ buffer_[size_] = '\0'; } va_end(args); - RTC_DCHECK(IsConsistent()); + RTC_CHECK(IsConsistent()); return *this; }
diff --git a/rtc_base/strings/string_builder.h b/rtc_base/strings/string_builder.h index 41efe9a..4a3df25 100644 --- a/rtc_base/strings/string_builder.h +++ b/rtc_base/strings/string_builder.h
@@ -155,11 +155,8 @@ size_t size() const { return str_.size(); } - std::string Release() { - std::string ret = std::move(str_); - str_.clear(); - return ret; - } + // Moves out the internal std::string. + std::string Release() { return std::move(str_); } // Allows appending a printf style formatted string. StringBuilder& AppendFormat(const char* fmt, ...)
diff --git a/rtc_base/strings/string_builder_unittest.cc b/rtc_base/strings/string_builder_unittest.cc index cb45313..4764ac4 100644 --- a/rtc_base/strings/string_builder_unittest.cc +++ b/rtc_base/strings/string_builder_unittest.cc
@@ -14,7 +14,6 @@ #include <string> #include "absl/strings/string_view.h" -#include "rtc_base/checks.h" #include "test/gmock.h" #include "test/gtest.h" @@ -81,18 +80,13 @@ // 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 +#if GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) TEST(SimpleStringBuilderDeathTest, 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(SimpleStringBuilderDeathTest, BufferOverrunStdString) { @@ -100,41 +94,21 @@ 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(SimpleStringBuilderDeathTest, 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(SimpleStringBuilderDeathTest, 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(SimpleStringBuilderDeathTest, BufferOverrunConstCharPAlreadyFull) { @@ -142,12 +116,7 @@ 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(SimpleStringBuilderDeathTest, BufferOverrunIntAlreadyFull) { @@ -155,12 +124,7 @@ 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
diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 93ceb84..8e3f6a8 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc
@@ -573,8 +573,7 @@ // dumped video, since it's developers-only feature for debugging. absl::c_replace(decoded_output_file, ';', '/'); if (!decoded_output_file.empty()) { - char filename_buffer[256]; - SimpleStringBuilder ssb(filename_buffer); + StringBuilder ssb; ssb << decoded_output_file << "/webrtc_receive_stream_" << remote_ssrc() << "-" << env_.clock().TimeInMicroseconds() << ".ivf"; video_decoder =