From 7f3b99a6a78e524613337f42a99b5634c0ad05f8 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Fri, 15 May 2015 08:44:31 -0700 Subject: Fix potential UAF in ConcatInPlace. If ConcatCopy somehow gets a zero nNewlen, it returns early, without allocating a new m_Data. ConcatInPlace then frees the old one, leaving m_Data dangling. Also be concerned about the multiplication in the widestring version. So use wmemcpy and let the library cope with it. R=thestig@chromium.org Review URL: https://codereview.chromium.org/1130763007 --- core/include/fxcrt/fx_string.h | 2 ++ core/src/fxcrt/fx_basic_bstring.cpp | 11 ++++++----- core/src/fxcrt/fx_basic_bstring_unittest.cpp | 27 +++++++++++++++++++++++++++ core/src/fxcrt/fx_basic_wstring.cpp | 11 ++++++----- core/src/fxcrt/fx_basic_wstring_unittest.cpp | 27 +++++++++++++++++++++++++++ 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/core/include/fxcrt/fx_string.h b/core/include/fxcrt/fx_string.h index a7b9a23e25..3614cbe0ee 100644 --- a/core/include/fxcrt/fx_string.h +++ b/core/include/fxcrt/fx_string.h @@ -389,6 +389,7 @@ protected: void AllocBeforeWrite(FX_STRSIZE nLen); StringData* m_pData; + friend class fxcrt_ByteStringConcatInPlace_Test; }; inline CFX_ByteStringC::CFX_ByteStringC(const CFX_ByteString& src) { @@ -815,6 +816,7 @@ protected: void AllocCopy(CFX_WideString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex) const; StringData* m_pData; + friend class fxcrt_WideStringConcatInPlace_Test; }; inline CFX_WideStringC::CFX_WideStringC(const CFX_WideString& src) { diff --git a/core/src/fxcrt/fx_basic_bstring.cpp b/core/src/fxcrt/fx_basic_bstring.cpp index 87e50e76cc..781b821f00 100644 --- a/core/src/fxcrt/fx_basic_bstring.cpp +++ b/core/src/fxcrt/fx_basic_bstring.cpp @@ -422,9 +422,7 @@ void CFX_ByteString::ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData) 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 @@ void CFX_ByteString::ConcatCopy(FX_STRSIZE nSrc1Len, FX_LPCSTR lpszSrc1Data, 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 { diff --git a/core/src/fxcrt/fx_basic_bstring_unittest.cpp b/core/src/fxcrt/fx_basic_bstring_unittest.cpp index 1f80207e96..bcdd33b8b8 100644 --- a/core/src/fxcrt/fx_basic_bstring_unittest.cpp +++ b/core/src/fxcrt/fx_basic_bstring_unittest.cpp @@ -288,6 +288,33 @@ TEST(fxcrt, ByteStringCNull) { 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"); diff --git a/core/src/fxcrt/fx_basic_wstring.cpp b/core/src/fxcrt/fx_basic_wstring.cpp index da022053b8..3c54ca983e 100644 --- a/core/src/fxcrt/fx_basic_wstring.cpp +++ b/core/src/fxcrt/fx_basic_wstring.cpp @@ -237,9 +237,7 @@ void CFX_WideString::ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCWSTR lpszSrcData) 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 @@ void CFX_WideString::ConcatCopy(FX_STRSIZE nSrc1Len, FX_LPCWSTR lpszSrc1Data, 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() { diff --git a/core/src/fxcrt/fx_basic_wstring_unittest.cpp b/core/src/fxcrt/fx_basic_wstring_unittest.cpp index 21b5ae54f6..847e5e8076 100644 --- a/core/src/fxcrt/fx_basic_wstring_unittest.cpp +++ b/core/src/fxcrt/fx_basic_wstring_unittest.cpp @@ -249,6 +249,33 @@ TEST(fxcrt, WideStringOperatorNE) { 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) { -- cgit v1.2.3