commit 861a552af4aa7edb24c600e25a7bf388a1cdb364
author Tom Sepez <tsepez@chromium.org> Fri May 15 09:09:22 2015 -0700
committer Tom Sepez <tsepez@chromium.org> Fri May 15 09:09:22 2015 -0700
tree 156c726317ac50e8876f86fc01bf8a5d3a968785
parent be6c8f29b92208f2806b8f70f0320b46500ddd94

Merge to XFA: Fix potential UAF in ConcatInPlace.

Original Review URL: https://codereview.chromium.org/1130763007
TBR=thestig@chromium.org

Review URL: https://codereview.chromium.org/1123333004

core/src/fxcrt/fx_basic_bstring.cpp

diff --git a/core/src/fxcrt/fx_basic_bstring.cpp b/core/src/fxcrt/fx_basic_bstring.cpp
index 87e50e7..781b821 100644
--- a/core/src/fxcrt/fx_basic_bstring.cpp
+++ b/core/src/fxcrt/fx_basic_bstring.cpp
@@ -422,9 +422,7 @@
         return;
     }
     if (m_pData->m_nRefs > 1 || m_pData->m_nDataLength + nSrcLen > m_pData->m_nAllocLength) {
-        StringData* pOldData = m_pData;
         ConcatCopy(m_pData->m_nDataLength, m_pData->m_String, nSrcLen, lpszSrcData);
-        pOldData->Release();
     } else {
         FXSYS_memcpy32(m_pData->m_String + m_pData->m_nDataLength, lpszSrcData, nSrcLen);
         m_pData->m_nDataLength += nSrcLen;
@@ -435,14 +433,17 @@
                                 FX_STRSIZE nSrc2Len, FX_LPCSTR lpszSrc2Data)
 {
     int nNewLen = nSrc1Len + nSrc2Len;
-    if (nNewLen == 0) {
+    if (nNewLen <= 0) {
         return;
     }
+    // Don't release until done copying, might be one of the arguments.
+    StringData* pOldData = m_pData;
     m_pData = StringData::Create(nNewLen);
     if (m_pData) {
-        FXSYS_memcpy32(m_pData->m_String, lpszSrc1Data, nSrc1Len);
-        FXSYS_memcpy32(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len);
+        memcpy(m_pData->m_String, lpszSrc1Data, nSrc1Len);
+        memcpy(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len);
     }
+    pOldData->Release();
 }
 CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst) const
 {

core/src/fxcrt/fx_basic_bstring_unittest.cpp

diff --git a/core/src/fxcrt/fx_basic_bstring_unittest.cpp b/core/src/fxcrt/fx_basic_bstring_unittest.cpp
index 1f80207..bcdd33b 100644
--- a/core/src/fxcrt/fx_basic_bstring_unittest.cpp
+++ b/core/src/fxcrt/fx_basic_bstring_unittest.cpp
@@ -288,6 +288,33 @@
   EXPECT_NE(null_string, non_null_string);
 }
 
+TEST(fxcrt, ByteStringConcatInPlace) {
+    CFX_ByteString fred;
+    fred.ConcatInPlace(4, "FRED");
+    EXPECT_EQ("FRED", fred);
+
+    fred.ConcatInPlace(2, "DY");
+    EXPECT_EQ("FREDDY", fred);
+
+    fred.Delete(3, 3);
+    EXPECT_EQ("FRE", fred);
+
+    fred.ConcatInPlace(1, "D");
+    EXPECT_EQ("FRED", fred);
+
+    CFX_ByteString copy = fred;
+    fred.ConcatInPlace(2, "DY");
+    EXPECT_EQ("FREDDY", fred);
+    EXPECT_EQ("FRED", copy);
+
+    // Test invalid arguments.
+    copy = fred;
+    fred.ConcatInPlace(-6, "freddy");
+    CFX_ByteString not_aliased("xxxxxx");
+    EXPECT_EQ("FREDDY", fred);
+    EXPECT_EQ("xxxxxx", not_aliased);
+}
+
 TEST(fxcrt, ByteStringCNotNull) {
   CFX_ByteStringC string3("abc");
   CFX_ByteStringC string6("abcdef");

core/src/fxcrt/fx_basic_wstring.cpp

diff --git a/core/src/fxcrt/fx_basic_wstring.cpp b/core/src/fxcrt/fx_basic_wstring.cpp
index da02205..3c54ca9 100644
--- a/core/src/fxcrt/fx_basic_wstring.cpp
+++ b/core/src/fxcrt/fx_basic_wstring.cpp
@@ -237,9 +237,7 @@
         return;
     }
     if (m_pData->m_nRefs > 1 || m_pData->m_nDataLength + nSrcLen > m_pData->m_nAllocLength) {
-        StringData* pOldData = m_pData;
         ConcatCopy(m_pData->m_nDataLength, m_pData->m_String, nSrcLen, lpszSrcData);
-        pOldData->Release();
     } else {
         FXSYS_memcpy32(m_pData->m_String + m_pData->m_nDataLength, lpszSrcData, nSrcLen * sizeof(FX_WCHAR));
         m_pData->m_nDataLength += nSrcLen;
@@ -250,14 +248,17 @@
                                 FX_STRSIZE nSrc2Len, FX_LPCWSTR lpszSrc2Data)
 {
     FX_STRSIZE nNewLen = nSrc1Len + nSrc2Len;
-    if (nNewLen == 0) {
+    if (nNewLen <= 0) {
         return;
     }
+    // Don't release until done copying, might be one of the arguments.
+    StringData* pOldData = m_pData;
     m_pData = StringData::Create(nNewLen);
     if (m_pData) {
-        FXSYS_memcpy32(m_pData->m_String, lpszSrc1Data, nSrc1Len * sizeof(FX_WCHAR));
-        FXSYS_memcpy32(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len * sizeof(FX_WCHAR));
+        wmemcpy(m_pData->m_String, lpszSrc1Data, nSrc1Len);
+        wmemcpy(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len);
     }
+    pOldData->Release();
 }
 void CFX_WideString::CopyBeforeWrite()
 {

core/src/fxcrt/fx_basic_wstring_unittest.cpp

diff --git a/core/src/fxcrt/fx_basic_wstring_unittest.cpp b/core/src/fxcrt/fx_basic_wstring_unittest.cpp
index 21b5ae5..847e5e8 100644
--- a/core/src/fxcrt/fx_basic_wstring_unittest.cpp
+++ b/core/src/fxcrt/fx_basic_wstring_unittest.cpp
@@ -249,6 +249,33 @@
     EXPECT_TRUE(c_string3 != wide_string);
 }
 
+TEST(fxcrt, WideStringConcatInPlace) {
+    CFX_WideString fred;
+    fred.ConcatInPlace(4, L"FRED");
+    EXPECT_EQ(L"FRED", fred);
+
+    fred.ConcatInPlace(2, L"DY");
+    EXPECT_EQ(L"FREDDY", fred);
+
+    fred.Delete(3, 3);
+    EXPECT_EQ(L"FRE", fred);
+
+    fred.ConcatInPlace(1, L"D");
+    EXPECT_EQ(L"FRED", fred);
+
+    CFX_WideString copy = fred;
+    fred.ConcatInPlace(2, L"DY");
+    EXPECT_EQ(L"FREDDY", fred);
+    EXPECT_EQ(L"FRED", copy);
+
+    // Test invalid arguments.
+    copy = fred;
+    fred.ConcatInPlace(-6, L"freddy");
+    CFX_WideString not_aliased(L"xxxxxx");
+    EXPECT_EQ(L"FREDDY", fred);
+    EXPECT_EQ(L"xxxxxx", not_aliased);
+}
+
 #define ByteStringLiteral(str) CFX_ByteString(FX_BSTRC(str))
 
 TEST(fxcrt, WideStringUTF16LE_Encode) {