WriteInto handles length_with_null values of 1 better.

In debug builds on VS2010, the old code triggered a debug assertion in
xstring, since [0] was being accessed and the size of the array was 0.

A |length_with_null| of 1 is considered a valid use of this function, so
an alternate implementation is used which returns the data() pointer.

BUG=None
TEST=net_unittests --gtest_filter=*Digest* on debug on VS2010 works.


Review URL: http://codereview.chromium.org/8143001

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

6 files changed, 97 insertions, 25 deletions

diff --git a/base/i18n/icu_string_conversions.cc b/base/i18n/icu_string_conversions.cc
index 6b46537..6ee01f6 100644
--- a/base/i18n/icu_string_conversions.cc
+++ b/base/i18n/icu_string_conversions.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -8,6 +8,7 @@
 
 #include "base/basictypes.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "unicode/ucnv.h"
@@ -187,7 +188,8 @@ bool CodepageToUTF16(const std::string& encoded,
   size_t uchar_max_length = encoded.length() + 1;
 
   SetUpErrorHandlerForToUChars(on_error, converter, &status);
-  int actual_size = ucnv_toUChars(converter, WriteInto(utf16, uchar_max_length),
+  scoped_array<char16> buffer(new char16[uchar_max_length]);
+  int actual_size = ucnv_toUChars(converter, buffer.get(),
       static_cast<int>(uchar_max_length), encoded.data(),
       static_cast<int>(encoded.length()), &status);
   ucnv_close(converter);
@@ -196,7 +198,7 @@ bool CodepageToUTF16(const std::string& encoded,
     return false;
   }
 
-  utf16->resize(actual_size);
+  utf16->assign(buffer.get(), actual_size);
   return true;
 }
 
@@ -251,8 +253,9 @@ bool CodepageToWide(const std::string& encoded,
   size_t wchar_max_length = encoded.length() + 1;
 
   SetUpErrorHandlerForToUChars(on_error, converter, &status);
+  scoped_array<wchar_t> buffer(new wchar_t[wchar_max_length]);
   int actual_size = ucnv_toAlgorithmic(utf32_platform_endian(), converter,
-      reinterpret_cast<char*>(WriteInto(wide, wchar_max_length)),
+      reinterpret_cast<char*>(buffer.get()),
       static_cast<int>(wchar_max_length) * sizeof(wchar_t), encoded.data(),
       static_cast<int>(encoded.length()), &status);
   ucnv_close(converter);
@@ -262,7 +265,7 @@ bool CodepageToWide(const std::string& encoded,
   }
 
   // actual_size is # of bytes.
-  wide->resize(actual_size / sizeof(wchar_t));
+  wide->assign(buffer.get(), actual_size / sizeof(wchar_t));
   return true;
 #endif  // defined(WCHAR_T_IS_UTF32)
 }
@@ -279,13 +282,13 @@ bool ConvertToUtf8AndNormalize(const std::string& text,
   UErrorCode status = U_ZERO_ERROR;
   size_t max_length = utf16.length() + 1;
   string16 normalized_utf16;
+  scoped_array<char16> buffer(new char16[max_length]);
   int actual_length = unorm_normalize(
       utf16.c_str(), utf16.length(), UNORM_NFC, 0,
-      WriteInto(&normalized_utf16, max_length),
-      static_cast<int>(max_length), &status);
+      buffer.get(), static_cast<int>(max_length), &status);
   if (!U_SUCCESS(status))
     return false;
-  normalized_utf16.resize(actual_length);
+  normalized_utf16.assign(buffer.get(), actual_length);
 
   return UTF16ToUTF8(normalized_utf16.data(),
                      normalized_utf16.length(), result);
diff --git a/base/i18n/icu_string_conversions_unittest.cc b/base/i18n/icu_string_conversions_unittest.cc
index a3ec35c..af2d709 100644
--- a/base/i18n/icu_string_conversions_unittest.cc
+++ b/base/i18n/icu_string_conversions_unittest.cc
@@ -238,6 +238,13 @@ static const struct {
    L"\x0E2A\x0E27\x0E31\x0E2A\x0E14\x0E35"
    L"\x0E04\x0E23\x0e31\x0E1A",
    NULL},
+  // Empty text
+  {"iscii-dev",
+   "",
+   OnStringConversionError::FAIL,
+   true,
+   L"",
+   NULL},
 };
 
 TEST(ICUStringConversionsTest, ConvertBetweenCodepageAndWide) {
@@ -358,6 +365,7 @@ static const struct {
   // Windows-1258 does have a combining character at xD2 (which is U+0309).
   // The sequence of (U+00E2, U+0309) is also encoded as U+1EA9.
   {"foo\xE2\xD2", "windows-1258", true, "foo\xE1\xBA\xA9"},
+  {"", "iso-8859-1", true, ""},
 };
 TEST(ICUStringConversionsTest, ConvertToUtf8AndNormalize) {
   std::string result;
diff --git a/base/string_util.h b/base/string_util.h
index e956e79..a0e554d 100644
--- a/base/string_util.h
+++ b/base/string_util.h
@@ -442,26 +442,40 @@ BASE_EXPORT void ReplaceSubstringsAfterOffset(
     const std::string& find_this,
     const std::string& replace_with);
 
-// This is mpcomplete's pattern for saving a string copy when dealing with
-// a function that writes results into a wchar_t[] and wanting the result to
-// end up in a std::wstring.  It ensures that the std::wstring's internal
-// buffer has enough room to store the characters to be written into it, and
-// sets its .length() attribute to the right value.
+// 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.
 //
-// The reserve() call allocates the memory required to hold the string
-// plus a terminating null.  This is done because resize() isn't
-// guaranteed to reserve space for the null.  The resize() call is
-// simply the only way to change the string's 'length' member.
+// 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.
 //
-// XXX-performance: the call to wide.resize() takes linear time, since it fills
-// the string's buffer with nulls.  I call it to change the length of the
-// string (needed because writing directly to the buffer doesn't do this).
-// Perhaps there's a constant-time way to change the string's length.
+// |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.
 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);
   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]);
 }
 
diff --git a/base/string_util_unittest.cc b/base/string_util_unittest.cc
index d449dee..f354a99 100644
--- a/base/string_util_unittest.cc
+++ b/base/string_util_unittest.cc
@@ -1079,4 +1079,46 @@ TEST(StringUtilTest, ContainsOnlyChars) {
   EXPECT_FALSE(ContainsOnlyChars("123a", "4321"));
 }
 
+TEST(StringUtilTest, 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);
+
+  // Validate that WriteInto doesn't modify other strings
+  // when using a Copy-on-Write implementation.
+  const char kLive[] = "live";
+  const char kDead[] = "dead";
+  const std::string live = kLive;
+  std::string dead = live;
+  strncpy(WriteInto(&dead, 5), kDead, 4);
+  EXPECT_STREQ(kDead, dead.c_str());
+  EXPECT_EQ(4u, dead.size());
+  EXPECT_STREQ(kLive, live.c_str());
+  EXPECT_EQ(4u, live.size());
+}
+
 }  // namespace base
diff --git a/net/proxy/proxy_resolver_v8.cc b/net/proxy/proxy_resolver_v8.cc
index 5c3113d..1bd736c 100644
--- a/net/proxy/proxy_resolver_v8.cc
+++ b/net/proxy/proxy_resolver_v8.cc
@@ -138,8 +138,10 @@ const size_t kMaxStringBytesForCopy = 256;
 
 // Converts a V8 String to a UTF8 std::string.
 std::string V8StringToUTF8(v8::Handle<v8::String> s) {
+  int len = s->Length();
   std::string result;
-  s->WriteUtf8(WriteInto(&result, s->Length() + 1));
+  if (len > 0)
+    s->WriteUtf8(WriteInto(&result, len + 1));
   return result;
 }
 
@@ -149,7 +151,8 @@ string16 V8StringToUTF16(v8::Handle<v8::String> s) {
   string16 result;
   // Note that the reinterpret cast is because on Windows string16 is an alias
   // to wstring, and hence has character type wchar_t not uint16_t.
-  s->Write(reinterpret_cast<uint16_t*>(WriteInto(&result, len + 1)), 0, len);
+  if (len > 0)
+    s->Write(reinterpret_cast<uint16_t*>(WriteInto(&result, len + 1)), 0, len);
   return result;
 }
 
diff --git a/views/controls/textfield/native_textfield_win.cc b/views/controls/textfield/native_textfield_win.cc
index 8e29ebc..6001674 100644
--- a/views/controls/textfield/native_textfield_win.cc
+++ b/views/controls/textfield/native_textfield_win.cc
@@ -209,8 +209,10 @@ string16 NativeTextfieldWin::GetSelectedText() const {
   GetSel(start, end);
 
   // Grab the selected text.
-  std::wstring str;
-  GetSelText(WriteInto(&str, end - start + 1));
+  string16 str;
+  long length = end - start;
+  if (length > 0)
+    GetSelText(WriteInto(&str, length + 1));
 
   return str;
 }