Make SimpleStringBuilder into a non-template
So that future CLs can de-inline its methods.
We do this by asking the caller to allocate the buffer instead of
having it as a data member.
Bug: webrtc:8982
Change-Id: I246b0973e54510fdd880c3b6875336c31334d008
Reviewed-on: https://webrtc-review.googlesource.com/60000
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22355}diff --git a/rtc_base/strings/string_builder.cc b/rtc_base/strings/string_builder.cc
new file mode 100644
index 0000000..499fb9a
--- /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<char> 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 15951e0..b450a99 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 <cstdio>
+#include <cstring>
#include <string>
+#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 <size_t buffer_size>
+// 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<char> buffer);
SimpleStringBuilder(const SimpleStringBuilder&) = delete;
SimpleStringBuilder& operator=(const SimpleStringBuilder&) = delete;
@@ -80,48 +82,60 @@
// 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<char> 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 0403ab8..8d6312f 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, 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