If we can't read a unicode character, write the standard "unknown" (0xFFFD) character. This will prevent security issues where the current behaviour can be used to strip characters out of a string after it has passed some validation.

BUG=30798
TEST=utf_string_conversions_unittest.cc,utf_offset_string_conversions_unittest.cc,zip_unittest.cc

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

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

6 files changed, 30 insertions, 40 deletions

diff --git a/base/utf_offset_string_conversions.cc b/base/utf_offset_string_conversions.cc
index 69b572e..4c47ef8 100644
--- a/base/utf_offset_string_conversions.cc
+++ b/base/utf_offset_string_conversions.cc
@@ -36,8 +36,7 @@ bool ConvertUnicode(const SRC_CHAR* src,
     if (ReadUnicodeCharacter(src, src_len32, &i, &code_point)) {
       chars_written = WriteUnicodeCharacter(code_point, output);
     } else {
-      // TODO(jungshik): consider adding 'Replacement character' (U+FFFD)
-      // in place of an invalid codepoint.
+      chars_written = WriteUnicodeCharacter(0xFFFD, output);
       success = false;
     }
     if ((output_offset != std::wstring::npos) &&
diff --git a/base/utf_offset_string_conversions_unittest.cc b/base/utf_offset_string_conversions_unittest.cc
index 00d87d3..4f13ab3 100644
--- a/base/utf_offset_string_conversions_unittest.cc
+++ b/base/utf_offset_string_conversions_unittest.cc
@@ -43,7 +43,7 @@ TEST(UTFOffsetStringConversionsTest, AdjustOffset) {
     {"", 0, std::wstring::npos},
     {"\xe4\xbd\xa0\xe5\xa5\xbd", 1, std::wstring::npos},
     {"\xe4\xbd\xa0\xe5\xa5\xbd", 3, 1},
-    {"\xed\xb0\x80z", 3, 0},
+    {"\xed\xb0\x80z", 3, 1},
     {"A\xF0\x90\x8C\x80z", 1, 1},
     {"A\xF0\x90\x8C\x80z", 2, std::wstring::npos},
 #if defined(WCHAR_T_IS_UTF16)
diff --git a/base/utf_string_conversions.cc b/base/utf_string_conversions.cc
index 7376933..d517e1b 100644
--- a/base/utf_string_conversions.cc
+++ b/base/utf_string_conversions.cc
@@ -32,8 +32,7 @@ bool ConvertUnicode(const SRC_CHAR* src,
     if (ReadUnicodeCharacter(src, src_len32, &i, &code_point)) {
       WriteUnicodeCharacter(code_point, output);
     } else {
-      // TODO(jungshik): consider adding 'Replacement character' (U+FFFD)
-      // in place of an invalid codepoint.
+      WriteUnicodeCharacter(0xFFFD, output);
       success = false;
     }
   }
diff --git a/base/utf_string_conversions_unittest.cc b/base/utf_string_conversions_unittest.cc
index 19189971..6ba0b5b 100644
--- a/base/utf_string_conversions_unittest.cc
+++ b/base/utf_string_conversions_unittest.cc
@@ -94,13 +94,13 @@ TEST(UTFStringConversionsTest, ConvertUTF8ToWide) {
     // Non-character is passed through.
     {"\xef\xbf\xbfHello", L"\xffffHello", true},
     // Truncated UTF-8 sequence.
-    {"\xe4\xa0\xe5\xa5\xbd", L"\x597d", false},
+    {"\xe4\xa0\xe5\xa5\xbd", L"\xfffd\x597d", false},
     // Truncated off the end.
-    {"\xe5\xa5\xbd\xe4\xa0", L"\x597d", false},
+    {"\xe5\xa5\xbd\xe4\xa0", L"\x597d\xfffd", false},
     // Non-shortest-form UTF-8.
-    {"\xf0\x84\xbd\xa0\xe5\xa5\xbd", L"\x597d", false},
+    {"\xf0\x84\xbd\xa0\xe5\xa5\xbd", L"\xfffd\x597d", false},
     // This UTF-8 character decodes to a UTF-16 surrogate, which is illegal.
-    {"\xed\xb0\x80", L"", false},
+    {"\xed\xb0\x80", L"\xfffd", false},
     // Non-BMP characters. The second is a non-character regarded as valid.
     // The result will either be in UTF-16 or UTF-32.
 #if defined(WCHAR_T_IS_UTF16)
@@ -152,9 +152,9 @@ TEST(UTFStringConversionsTest, ConvertUTF16ToUTF8) {
     {L"\xffffHello", "\xEF\xBF\xBFHello", true},
     {L"\xdbff\xdffeHello", "\xF4\x8F\xBF\xBEHello", true},
     // The first character is a truncated UTF-16 character.
-    {L"\xd800\x597d", "\xe5\xa5\xbd", false},
+    {L"\xd800\x597d", "\xef\xbf\xbd\xe5\xa5\xbd", false},
     // Truncated at the end.
-    {L"\x597d\xd800", "\xe5\xa5\xbd", false},
+    {L"\x597d\xd800", "\xe5\xa5\xbd\xef\xbf\xbd", false},
   };
 
   for (int i = 0; i < arraysize(convert_cases); i++) {
@@ -184,10 +184,10 @@ TEST(UTFStringConversionsTest, ConvertUTF32ToUTF8) {
     {L"\xffffHello", "\xEF\xBF\xBFHello", true},
     {L"\x10fffeHello", "\xF4\x8F\xBF\xBEHello", true},
     // Invalid Unicode code points.
-    {L"\xfffffffHello", "Hello", false},
+    {L"\xfffffffHello", "\xEF\xBF\xBDHello", false},
     // The first character is a truncated UTF-16 character.
-    {L"\xd800\x597d", "\xe5\xa5\xbd", false},
-    {L"\xdc01Hello", "Hello", false},
+    {L"\xd800\x597d", "\xef\xbf\xbd\xe5\xa5\xbd", false},
+    {L"\xdc01Hello", "\xef\xbf\xbdHello", false},
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(convert_cases); i++) {
diff --git a/chrome/common/zip_unittest.cc b/chrome/common/zip_unittest.cc
index 57eabf4..afb50c0 100644
--- a/chrome/common/zip_unittest.cc
+++ b/chrome/common/zip_unittest.cc
@@ -41,23 +41,16 @@ class ZipTest : public PlatformTest {
   }
 
   void TestUnzipFile(const FilePath::StringType& filename,
-                     bool expect_hidden_files, bool need_success) {
+                     bool expect_hidden_files) {
     FilePath test_dir;
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir));
     test_dir = test_dir.AppendASCII("zip");
-    TestUnzipFile(test_dir.Append(filename), expect_hidden_files,
-                  need_success);
+    TestUnzipFile(test_dir.Append(filename), expect_hidden_files);
   }
 
-  void TestUnzipFile(const FilePath& path, bool expect_hidden_files,
-                     bool need_success) {
+  void TestUnzipFile(const FilePath& path, bool expect_hidden_files) {
     ASSERT_TRUE(file_util::PathExists(path)) << "no file " << path.value();
-    if (need_success) {
-      ASSERT_TRUE(Unzip(path, test_dir_));
-    } else {
-      ASSERT_FALSE(Unzip(path, test_dir_));
-      return;
-    }
+    ASSERT_TRUE(Unzip(path, test_dir_));
 
     file_util::FileEnumerator files(test_dir_, true,
         static_cast<file_util::FileEnumerator::FILE_TYPE>(
@@ -95,15 +88,18 @@ class ZipTest : public PlatformTest {
 };
 
 TEST_F(ZipTest, Unzip) {
-  TestUnzipFile(FILE_PATH_LITERAL("test.zip"), true, true);
+  TestUnzipFile(FILE_PATH_LITERAL("test.zip"), true);
 }
 
 TEST_F(ZipTest, UnzipUncompressed) {
-  TestUnzipFile(FILE_PATH_LITERAL("test_nocompress.zip"), true, true);
+  TestUnzipFile(FILE_PATH_LITERAL("test_nocompress.zip"), true);
 }
 
 TEST_F(ZipTest, UnzipEvil) {
-  TestUnzipFile(FILE_PATH_LITERAL("evil.zip"), true, false);
+  FilePath path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &path));
+  path = path.AppendASCII("zip").AppendASCII("evil.zip");
+  ASSERT_FALSE(Unzip(path, test_dir_));
   FilePath evil_file = test_dir_;
   evil_file = evil_file.AppendASCII(
       "../levilevilevilevilevilevilevilevilevilevilevilevil");
@@ -111,15 +107,11 @@ TEST_F(ZipTest, UnzipEvil) {
 }
 
 TEST_F(ZipTest, UnzipEvil2) {
-  ScopedTempDir dest_dir;
-  ASSERT_TRUE(dest_dir.CreateUniqueTempDir());
-
-  FilePath test_dir;
-  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir));
-  test_dir = test_dir.AppendASCII("zip");
-  TestUnzipFile(FILE_PATH_LITERAL("evil_via_invalid_utf8.zip"), true, false);
-
-  FilePath evil_file = dest_dir.path();
+  FilePath path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &path));
+  path = path.AppendASCII("zip").AppendASCII("evil_via_invalid_utf8.zip");
+  ASSERT_TRUE(Unzip(path, test_dir_));
+  FilePath evil_file = test_dir_;
   evil_file = evil_file.AppendASCII("../evil.txt");
   ASSERT_FALSE(file_util::PathExists(evil_file));
 }
@@ -134,7 +126,7 @@ TEST_F(ZipTest, Zip) {
   FilePath zip_file = temp_dir.path().AppendASCII("out.zip");
 
   EXPECT_TRUE(Zip(src_dir, zip_file, true));
-  TestUnzipFile(zip_file, true, true);
+  TestUnzipFile(zip_file, true);
 }
 
 TEST_F(ZipTest, ZipIgnoreHidden) {
@@ -147,7 +139,7 @@ TEST_F(ZipTest, ZipIgnoreHidden) {
   FilePath zip_file = temp_dir.path().AppendASCII("out.zip");
 
   EXPECT_TRUE(Zip(src_dir, zip_file, false));
-  TestUnzipFile(zip_file, false, true);
+  TestUnzipFile(zip_file, false);
 }
 
 }  // namespace
diff --git a/net/base/file_stream_posix.cc b/net/base/file_stream_posix.cc
index f039996..eda0927 100644
--- a/net/base/file_stream_posix.cc
+++ b/net/base/file_stream_posix.cc
@@ -329,7 +329,7 @@ int FileStream::Open(const FilePath& path, int open_flags) {
   }
 
   open_flags_ = open_flags;
-  file_ = base::CreatePlatformFile(path.ToWStringHack(), open_flags_, NULL);
+  file_ = base::CreatePlatformFile(path, open_flags_, NULL);
   if (file_ == base::kInvalidPlatformFileValue) {
     LOG(WARNING) << "Failed to open file: " << errno
           << " (" << path.ToWStringHack() << ")";