summaryrefslogtreecommitdiff
path: root/core/src
diff options
context:
space:
mode:
authorTom Sepez <tsepez@chromium.org>2015-05-15 08:44:31 -0700
committerTom Sepez <tsepez@chromium.org>2015-05-15 08:44:31 -0700
commit7f3b99a6a78e524613337f42a99b5634c0ad05f8 (patch)
treef13654bc0408c72a056b502d3106fd8e28c616e9 /core/src
parentb60617f5557a037e64876f7495af80573a35cb4f (diff)
downloadpdfium-7f3b99a6a78e524613337f42a99b5634c0ad05f8.tar.xz
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
Diffstat (limited to 'core/src')
-rw-r--r--core/src/fxcrt/fx_basic_bstring.cpp11
-rw-r--r--core/src/fxcrt/fx_basic_bstring_unittest.cpp27
-rw-r--r--core/src/fxcrt/fx_basic_wstring.cpp11
-rw-r--r--core/src/fxcrt/fx_basic_wstring_unittest.cpp27
4 files changed, 66 insertions, 10 deletions
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) {