ake string_util::WriteInto() DCHECK() that the supplied |length_with_…

…null| > 1, meaning that the without-'\0' string is non-empty. This replaces the conditional code added recently that makes this case return NULL. It's easier to understand if it's simply an error to call WriteInto() in this case at all.

Add DCHECK()s or conditionals as appropriate to callers in order to ensure this assertion holds.

BUG=none
TEST=none
Review URL: http://codereview.chromium.org/8418034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@112005 0039d316-1c4b-4281-b951-d872f2087c98

base/base_paths_mac.mm

@@ -18,24 +18,18 @@
 
 namespace {
 
-bool GetNSExecutablePath(FilePath* path) WARN_UNUSED_RESULT;
-
-bool GetNSExecutablePath(FilePath* path) {
+void GetNSExecutablePath(FilePath* path) {
   DCHECK(path);
   // Executable path can have relative references ("..") depending on
   // how the app was launched.
   uint32_t executable_length = 0;
   _NSGetExecutablePath(NULL, &executable_length);
-  DCHECK_GE(executable_length, 1u);
+  DCHECK_GT(executable_length, 1u);
   std::string executable_path;
-  char* executable_path_c = WriteInto(&executable_path, executable_length);
-  int rv = _NSGetExecutablePath(executable_path_c, &executable_length);
+  int rv = _NSGetExecutablePath(WriteInto(&executable_path, executable_length),
+                                &executable_length);
   DCHECK_EQ(rv, 0);
-  DCHECK(!executable_path.empty());
-  if ((rv != 0) || (executable_path.empty()))
-    return false;
   *path = FilePath(executable_path);
-  return true;
 }
 
 // Returns true if the module for |address| is found. |path| will contain
@@ -58,7 +52,8 @@ bool GetModulePathForAddress(FilePath* path, const void* address) {
 bool PathProviderMac(int key, FilePath* result) {
   switch (key) {
     case base::FILE_EXE:
-      return GetNSExecutablePath(result);
+      GetNSExecutablePath(result);
+      return true;
     case base::FILE_MODULE:
       return GetModulePathForAddress(result,
           reinterpret_cast<const void*>(&base::PathProviderMac));

base/rand_util.cc

@@ -71,6 +71,7 @@ void RandBytes(void* output, size_t output_length) {
 }
 
 std::string RandBytesAsString(size_t length) {
+  DCHECK_GT(length, 0u);
   std::string result;
   RandBytes(WriteInto(&result, length + 1), length);
   return result;

base/rand_util.h

@@ -38,9 +38,9 @@ BASE_EXPORT double BitsToOpenEndedUnitInterval(uint64 bits);
 BASE_EXPORT void RandBytes(void* output, size_t output_length);
 
 // Fills a string of length |length| with with cryptographically strong random
-// data and returns it.
+// data and returns it.  |length| should be nonzero.
 //
-// Not that this is a variation of |RandBytes| with a different return type.
+// Note that this is a variation of |RandBytes| with a different return type.
 BASE_EXPORT std::string RandBytesAsString(size_t length);
 
 }  // namespace base

base/rand_util_unittest.cc

@@ -41,8 +41,8 @@ TEST(RandUtilTest, RandBytes) {
 }
 
 TEST(RandUtilTest, RandBytesAsString) {
-  std::string random_string = base::RandBytesAsString(0);
-  EXPECT_EQ(0U, random_string.size());
+  std::string random_string = base::RandBytesAsString(1);
+  EXPECT_EQ(1U, random_string.size());
   random_string = base::RandBytesAsString(145);
   EXPECT_EQ(145U, random_string.size());
   char accumulator = 0;

base/string_util.h

@@ -457,40 +457,32 @@ BASE_EXPORT void ReplaceSubstringsAfterOffset(
     const std::string& find_this,
     const std::string& replace_with);
 
-// Reserves enough memory in |str| to accommodate |length_with_null|
-// characters, sets the size of |str| to |length_with_null - 1| characters,
-// and returns a pointer to the underlying contiguous array of characters.
+// Reserves enough memory in |str| to accommodate |length_with_null| characters,
+// sets the size of |str| to |length_with_null - 1| characters, and returns a
+// pointer to the underlying contiguous array of characters.  This is typically
+// used when calling a function that writes results into a character array, but
+// the caller wants the data to be managed by a string-like object.  It is
+// convenient in that is can be used inline in the call, and fast in that it
+// avoids copying the results of the call from a char* into a string.
 //
-// This is typically used when calling a function that writes results into a
-// character array, but the caller wants the data to be managed by a
-// string-like object.
+// |length_with_null| must be at least 2, since otherwise the underlying string
+// would have size 0, and trying to access &((*str)[0]) in that case can result
+// in a number of problems.
 //
-// |length_with_null| must be >= 1. In the |length_with_null| == 1 case,
-// NULL is returned rather than a pointer to the array, since there is no way
-// to provide access to the underlying character array of a 0-length
-// string-like object without breaking guarantees provided by the C++
-// standards.
-//
-// Internally, this takes linear time because the underlying array needs to
-// be 0-filled for all |length_with_null - 1| * sizeof(character) bytes.
+// Internally, this takes linear time because the resize() call 0-fills the
+// underlying array for potentially all
+// (|length_with_null - 1| * sizeof(string_type::value_type)) bytes.  Ideally we
+// could avoid this aspect of the resize() call, as we expect the caller to
+// immediately write over this memory, but there is no other way to set the size
+// of the string, and not doing that will mean people who access |str| rather
+// than str.c_str() will get back a string of whatever size |str| had on entry
+// to this function (probably 0).
 template <class string_type>
 inline typename string_type::value_type* WriteInto(string_type* str,
                                                    size_t length_with_null) {
-  DCHECK_NE(0u, length_with_null);
+  DCHECK_GT(length_with_null, 1u);
   str->reserve(length_with_null);
   str->resize(length_with_null - 1);
-
-  // If |length_with_null| is 1, calling (*str)[0] is invalid since the
-  // size() is 0. In some implementations this triggers an assertion.
-  //
-  // Trying to access the underlying byte array by casting away const
-  // when calling str->data() or str->c_str() is also incorrect.
-  // Some implementations of basic_string use a copy-on-write approach and
-  // this could end up mutating the data that is shared across multiple string
-  // objects.
-  if (length_with_null <= 1)
-    return NULL;
-
   return &((*str)[0]);
 }
 

base/string_util_unittest.cc

@@ -1113,34 +1113,27 @@ TEST(StringUtilTest, ContainsOnlyChars) {
   EXPECT_FALSE(ContainsOnlyChars("123a", "4321"));
 }
 
-TEST(StringUtilTest, WriteInto) {
+class WriteIntoTest : public testing::Test {
+ protected:
+  static void WritesCorrectly(size_t num_chars) {
+    std::string buffer;
+    char kOriginal[] = "supercali";
+    strncpy(WriteInto(&buffer, num_chars + 1), kOriginal, num_chars);
+    // Using std::string(buffer.c_str()) instead of |buffer| truncates the
+    // string at the first \0.
+    EXPECT_EQ(std::string(kOriginal,
+                          std::min(num_chars, arraysize(kOriginal) - 1)),
+              std::string(buffer.c_str()));
+    EXPECT_EQ(num_chars, buffer.size());
+  }
+};
+
+TEST_F(WriteIntoTest, WriteInto) {
   // Validate that WriteInto reserves enough space and
   // sizes a string correctly.
-  std::string buffer;
-  const char kOriginal[] = "supercali";
-  strncpy(WriteInto(&buffer, 1), kOriginal, 0);
-  EXPECT_STREQ("", buffer.c_str());
-  EXPECT_EQ(0u, buffer.size());
-  strncpy(WriteInto(&buffer, 2), kOriginal, 1);
-  EXPECT_STREQ("s", buffer.c_str());
-  EXPECT_EQ(1u, buffer.size());
-  strncpy(WriteInto(&buffer, 3), kOriginal, 2);
-  EXPECT_STREQ("su", buffer.c_str());
-  EXPECT_EQ(2u, buffer.size());
-  strncpy(WriteInto(&buffer, 5001), kOriginal, 5000);
-  EXPECT_STREQ("supercali", buffer.c_str());
-  EXPECT_EQ(5000u, buffer.size());
-  strncpy(WriteInto(&buffer, 3), kOriginal, 2);
-  EXPECT_STREQ("su", buffer.c_str());
-  EXPECT_EQ(2u, buffer.size());
-  strncpy(WriteInto(&buffer, 1), kOriginal, 0);
-  EXPECT_STREQ("", buffer.c_str());
-  EXPECT_EQ(0u, buffer.size());
-
-  // Validate that WriteInto returns NULL only when
-  // |length_with_null| == 1.
-  EXPECT_TRUE(WriteInto(&buffer, 1) == NULL);
-  EXPECT_TRUE(WriteInto(&buffer, 2) != NULL);
+  WritesCorrectly(1);
+  WritesCorrectly(2);
+  WritesCorrectly(5000);
 
   // Validate that WriteInto doesn't modify other strings
   // when using a Copy-on-Write implementation.
@@ -1149,9 +1142,9 @@ TEST(StringUtilTest, WriteInto) {
   const std::string live = kLive;
   std::string dead = live;
   strncpy(WriteInto(&dead, 5), kDead, 4);
-  EXPECT_STREQ(kDead, dead.c_str());
+  EXPECT_EQ(kDead, dead);
   EXPECT_EQ(4u, dead.size());
-  EXPECT_STREQ(kLive, live.c_str());
+  EXPECT_EQ(kLive, live);
   EXPECT_EQ(4u, live.size());
 }
 

chrome/browser/automation/testing_automation_provider_win.cc

@@ -86,9 +86,10 @@ void TestingAutomationProvider::SetWindowVisible(int handle,
 
 void TestingAutomationProvider::GetWindowTitle(int handle, string16* text) {
   gfx::NativeWindow window = window_tracker_->GetResource(handle);
-  std::wstring result;
   int length = ::GetWindowTextLength(window) + 1;
-  ::GetWindowText(window, WriteInto(&result, length), length);
-  text->assign(WideToUTF16(result));
+  if (length > 1)
+    ::GetWindowText(window, WriteInto(text, length), length);
+  else
+    text->clear();
 }
 

chrome/browser/browser_focus_uitest.cc

@@ -90,7 +90,8 @@ bool ChromeInForeground() {
   std::wstring caption;
   std::wstring filename;
   int len = ::GetWindowTextLength(window) + 1;
-  ::GetWindowText(window, WriteInto(&caption, len), len);
+  if (len > 1)
+    ::GetWindowText(window, WriteInto(&caption, len), len);
   bool chrome_window_in_foreground =
       EndsWith(caption, L" - Google Chrome", true) ||
       EndsWith(caption, L" - Chromium", true);
@@ -102,8 +103,8 @@ bool ChromeInForeground() {
     if (base::OpenProcessHandleWithAccess(process_id,
                                           PROCESS_QUERY_LIMITED_INFORMATION,
                                           &process)) {
-      len = MAX_PATH;
-      if (!GetProcessImageFileName(process, WriteInto(&filename, len), len)) {
+      if (!GetProcessImageFileName(process, WriteInto(&filename, MAX_PATH),
+                                   MAX_PATH)) {
         int error = GetLastError();
         filename = std::wstring(L"Unable to read filename for process id '" +
                                 base::IntToString16(process_id) +

chrome/browser/mac/install_from_dmg.mm

@@ -141,6 +141,10 @@ bool MediaResidesOnDiskImage(io_service_t media, std::string* image_path) {
     CFDataRef image_path_data = static_cast<CFDataRef>(
         image_path_cftyperef.get());
     CFIndex length = CFDataGetLength(image_path_data);
+    if (length <= 0) {
+      LOG(ERROR) << "image_path_data is unexpectedly empty";
+      return true;
+    }
     char* image_path_c = WriteInto(image_path, length + 1);
     CFDataGetBytes(image_path_data,
                    CFRangeMake(0, length),

chrome/browser/ui/views/omnibox/omnibox_view_win.cc

@@ -645,11 +645,9 @@ void OmniboxViewWin::OpenMatch(const AutocompleteMatch& match,
 
 string16 OmniboxViewWin::GetText() const {
   const int len = GetTextLength() + 1;
-  if (len <= 1)
-    return string16();
-
   string16 str;
-  GetWindowText(WriteInto(&str, len), len);
+  if (len > 1)
+    GetWindowText(WriteInto(&str, len), len);
   return str;
 }
 
@@ -2128,15 +2126,11 @@ void OmniboxViewWin::GetSelection(CHARRANGE& sel) const {
 }
 
 string16 OmniboxViewWin::GetSelectedText() const {
-  // Figure out the length of the selection.
   CHARRANGE sel;
   GetSel(sel);
-  if (sel.cpMin == sel.cpMax)  // GetSelText() crashes on NULL input.
-    return string16();
-
-  // Grab the selected text.
   string16 str;
-  GetSelText(WriteInto(&str, sel.cpMax - sel.cpMin + 1));
+  if (sel.cpMin != sel.cpMax)
+    GetSelText(WriteInto(&str, sel.cpMax - sel.cpMin + 1));
   return str;
 }
 

chrome/common/metrics_log_manager.cc

@@ -105,7 +105,7 @@ void MetricsLogManager::LoadPersistedUnsentLogs() {
 void MetricsLogManager::CompressStagedLog() {
   int text_size = staged_log_->GetEncodedLogSize();
   std::string staged_log_text;
-  // Leave room for the NULL terminator.
+  DCHECK_GT(text_size, 0);
   staged_log_->GetEncodedLog(WriteInto(&staged_log_text, text_size + 1),
                              text_size);
 

chrome/common/time_format.cc

@@ -296,9 +296,9 @@ static string16 FormatTimeImpl(const TimeDelta& delta, FormatType format_type) {
   // With the fallback added, this should never fail.
   DCHECK(U_SUCCESS(error));
   int capacity = time_string.length() + 1;
+  DCHECK_GT(capacity, 1);
   string16 result;
-  time_string.extract(static_cast<UChar*>(
-                      WriteInto(&result, capacity)),
+  time_string.extract(static_cast<UChar*>(WriteInto(&result, capacity)),
                       capacity, error);
   DCHECK(U_SUCCESS(error));
   return result;

chrome_frame/chrome_tab.cc

@@ -467,8 +467,9 @@ class SecurityDescBackup {
     DWORD reg_type = REG_NONE;
     if (backup_key.ReadValue(NULL, NULL, &len, &reg_type) != ERROR_SUCCESS)
       return false;
+    DCHECK_EQ(0u, len % sizeof(wchar_t));
 
-    if (reg_type != REG_SZ)
+    if ((len == 0) || (reg_type != REG_SZ))
       return false;
 
     size_t wchar_count = 1 + len / sizeof(wchar_t);

chrome_frame/test/win_event_receiver.cc

@@ -169,8 +169,8 @@ void WindowWatchdog::RemoveObserver(WindowObserver* observer) {
 std::string WindowWatchdog::GetWindowCaption(HWND hwnd) {
   std::string caption;
   int len = ::GetWindowTextLength(hwnd) + 1;
-  ::GetWindowTextA(hwnd, WriteInto(&caption, len), len);
-
+  if (len > 1)
+    ::GetWindowTextA(hwnd, WriteInto(&caption, len), len);
   return caption;
 }
 

chrome_frame/utils.cc

@@ -1328,6 +1328,7 @@ std::string Bscf2Str(DWORD flags) {
 // Reads data from a stream into a string.
 HRESULT ReadStream(IStream* stream, size_t size, std::string* data) {
   DCHECK(stream);
+  DCHECK_GT(size, 0u);
   DCHECK(data);
 
   DWORD read = 0;

crypto/encryptor.h

@@ -61,13 +61,15 @@ class CRYPTO_EXPORT Encryptor {
   // Initializes the encryptor using |key| and |iv|. Returns false if either the
   // key or the initialization vector cannot be used.
   //
-  // When |mode| is CTR then |iv| should be empty.
+  // If |mode| is CBC, |iv| must not be empty; if it is CTR, then |iv| must be
+  // empty.
   bool Init(SymmetricKey* key, Mode mode, const base::StringPiece& iv);
 
-  // Encrypts |plaintext| into |ciphertext|.
+  // Encrypts |plaintext| into |ciphertext|.  |plaintext| may only be empty if
+  // the mode is CBC.
   bool Encrypt(const base::StringPiece& plaintext, std::string* ciphertext);
 
-  // Decrypts |ciphertext| into |plaintext|.
+  // Decrypts |ciphertext| into |plaintext|.  |ciphertext| must not be empty.
   bool Decrypt(const base::StringPiece& ciphertext, std::string* plaintext);
 
   // Sets the counter value when in CTR mode. Currently only 128-bits

crypto/encryptor_mac.cc

@@ -49,17 +49,19 @@ bool Encryptor::Crypt(int /*CCOperation*/ op,
   // length is never larger than the input length plus the block size."
 
   size_t output_size = input.size() + iv_.size();
+  CHECK_GT(output_size, 0u);
+  CHECK_GT(output_size + 1, input.size());
   CCCryptorStatus err = CCCrypt(op,
                                 kCCAlgorithmAES128,
                                 kCCOptionPKCS7Padding,
                                 raw_key.Data, raw_key.Length,
                                 iv_.data(),
                                 input.data(), input.size(),
-                                WriteInto(output, output_size+1),
+                                WriteInto(output, output_size + 1),
                                 output_size,
                                 &output_size);
   if (err) {
-    output->resize(0);
+    output->clear();
     LOG(ERROR) << "CCCrypt returned " << err;
     return false;
   }
@@ -69,11 +71,13 @@ bool Encryptor::Crypt(int /*CCOperation*/ op,
 
 bool Encryptor::Encrypt(const base::StringPiece& plaintext,
                         std::string* ciphertext) {
+  CHECK(!plaintext.empty() || (mode_ == CBC));
   return Crypt(kCCEncrypt, plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
                         std::string* plaintext) {
+  CHECK(!ciphertext.empty());
   return Crypt(kCCDecrypt, ciphertext, plaintext);
 }