From 5e177d217195edbcdf1f54f928a942e1696197d8 Mon Sep 17 00:00:00 2001
From: Tommi <[email protected]>
Date: Tue, 24 Mar 2026 15:22:38 +0100
Subject: [PATCH] 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 <[email protected]>
Reviewed-by: Per Kjellander <[email protected]>
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;

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