From 30f2ff16bf30ccaa66bb7e144873fc29b4bcbdc2 Mon Sep 17 00:00:00 2001 From: Chris Palmer Date: Tue, 8 Jul 2014 13:27:00 -0700 Subject: Fix integer overflow in fx_basic_[bw]string.cpp. BUG=382601 R=jun_fang@foxitsoftware.com Review URL: https://codereview.chromium.org/336003004 --- core/include/fxcrt/fx_string.h | 4 +-- core/src/fxcrt/fx_basic_bstring.cpp | 54 ++++++++++++++++++++----------------- core/src/fxcrt/fx_basic_wstring.cpp | 35 ++++++++++++++++-------- 3 files changed, 56 insertions(+), 37 deletions(-) diff --git a/core/include/fxcrt/fx_string.h b/core/include/fxcrt/fx_string.h index 08fb83cfd7..7e02101def 100644 --- a/core/include/fxcrt/fx_string.h +++ b/core/include/fxcrt/fx_string.h @@ -340,7 +340,7 @@ public: protected: struct CFX_StringData* m_pData; - void AllocCopy(CFX_ByteString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex, FX_STRSIZE nExtraLen) const; + void AllocCopy(CFX_ByteString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex) const; void AssignCopy(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData); void ConcatCopy(FX_STRSIZE nSrc1Len, FX_LPCSTR lpszSrc1Data, FX_STRSIZE nSrc2Len, FX_LPCSTR lpszSrc2Data); void ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData); @@ -756,7 +756,7 @@ protected: void ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCWSTR lpszSrcData); void ConcatCopy(FX_STRSIZE nSrc1Len, FX_LPCWSTR lpszSrc1Data, FX_STRSIZE nSrc2Len, FX_LPCWSTR lpszSrc2Data); void AssignCopy(FX_STRSIZE nSrcLen, FX_LPCWSTR lpszSrcData); - void AllocCopy(CFX_WideString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex, FX_STRSIZE nExtraLen) const; + void AllocCopy(CFX_WideString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex) const; }; 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 ef52447771..435f5a5e5c 100644 --- a/core/src/fxcrt/fx_basic_bstring.cpp +++ b/core/src/fxcrt/fx_basic_bstring.cpp @@ -5,6 +5,8 @@ // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com #include "../../include/fxcrt/fx_basic.h" +#include "../../../third_party/numerics/safe_math.h" + static int _Buffer_itoa(char* buf, int i, FX_DWORD flags) { if (i == 0) { @@ -45,10 +47,14 @@ CFX_ByteString CFX_ByteString::FormatInteger(int i, FX_DWORD flags) } static CFX_StringData* FX_AllocString(int nLen) { - if (nLen == 0) { + // |nLen| is currently declared as in |int|. TODO(palmer): It should be + // a |size_t|, or at least unsigned. + if (nLen == 0 || nLen < 0) { return NULL; } - CFX_StringData* pData = (CFX_StringData*)FX_Alloc(FX_BYTE, sizeof(long) * 3 + (nLen + 1) * sizeof(char)); + base::CheckedNumeric nSize = nLen; + nSize += sizeof(long) * 3 + 1; + CFX_StringData* pData = (CFX_StringData*)FX_Alloc(FX_BYTE, nSize.ValueOrDie()); if (!pData) { return NULL; } @@ -86,7 +92,7 @@ CFX_ByteString::CFX_ByteString(FX_LPCSTR lpsz, FX_STRSIZE nLen) if (nLen) { m_pData = FX_AllocString(nLen); if (m_pData) { - FXSYS_memcpy32(m_pData->m_String, lpsz, nLen * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, lpsz, nLen); } } else { m_pData = NULL; @@ -97,7 +103,7 @@ CFX_ByteString::CFX_ByteString(FX_LPCBYTE lpsz, FX_STRSIZE nLen) if (nLen > 0) { m_pData = FX_AllocString(nLen); if (m_pData) { - FXSYS_memcpy32(m_pData->m_String, lpsz, nLen * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, lpsz, nLen); } } else { m_pData = NULL; @@ -195,7 +201,7 @@ void CFX_ByteString::Load(FX_LPCBYTE buf, FX_STRSIZE len) if (len) { m_pData = FX_AllocString(len); if (m_pData) { - FXSYS_memcpy32(m_pData->m_String, buf, len * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, buf, len); } } else { m_pData = NULL; @@ -293,7 +299,7 @@ bool CFX_ByteString::EqualNoCase(FX_BSTR str) const void CFX_ByteString::AssignCopy(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData) { AllocBeforeWrite(nSrcLen); - FXSYS_memcpy32(m_pData->m_String, lpszSrcData, nSrcLen * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, lpszSrcData, nSrcLen); m_pData->m_nDataLength = nSrcLen; m_pData->m_String[nSrcLen] = 0; } @@ -307,7 +313,7 @@ void CFX_ByteString::CopyBeforeWrite() FX_STRSIZE nDataLength = pData->m_nDataLength; m_pData = FX_AllocString(nDataLength); if (m_pData != NULL) { - FXSYS_memcpy32(m_pData->m_String, pData->m_String, (nDataLength + 1) * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, pData->m_String, nDataLength + 1); } } void CFX_ByteString::AllocBeforeWrite(FX_STRSIZE nLen) @@ -375,7 +381,7 @@ FX_LPSTR CFX_ByteString::GetBuffer(FX_STRSIZE nMinBufLength) if (!m_pData) { return NULL; } - FXSYS_memcpy32(m_pData->m_String, pOldData->m_String, (nOldLen + 1)*sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, pOldData->m_String, (nOldLen + 1)); m_pData->m_nDataLength = nOldLen; pOldData->m_nRefs --; if (pOldData->m_nRefs <= 0) { @@ -401,7 +407,7 @@ FX_STRSIZE CFX_ByteString::Delete(FX_STRSIZE nIndex, FX_STRSIZE nCount) CopyBeforeWrite(); int nBytesToCopy = nOldLength - mLength + 1; FXSYS_memmove32(m_pData->m_String + nIndex, - m_pData->m_String + mLength, nBytesToCopy * sizeof(char)); + m_pData->m_String + mLength, nBytesToCopy); m_pData->m_nDataLength = nOldLength - nCount; } return m_pData->m_nDataLength; @@ -416,7 +422,7 @@ void CFX_ByteString::ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData) if (!m_pData) { return; } - FXSYS_memcpy32(m_pData->m_String, lpszSrcData, nSrcLen * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, lpszSrcData, nSrcLen); return; } if (m_pData->m_nRefs > 1 || m_pData->m_nDataLength + nSrcLen > m_pData->m_nAllocLength) { @@ -424,7 +430,7 @@ void CFX_ByteString::ConcatInPlace(FX_STRSIZE nSrcLen, FX_LPCSTR lpszSrcData) ConcatCopy(m_pData->m_nDataLength, m_pData->m_String, nSrcLen, lpszSrcData); FX_ReleaseString(pOldData); } else { - FXSYS_memcpy32(m_pData->m_String + m_pData->m_nDataLength, lpszSrcData, nSrcLen * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String + m_pData->m_nDataLength, lpszSrcData, nSrcLen); m_pData->m_nDataLength += nSrcLen; m_pData->m_String[m_pData->m_nDataLength] = 0; } @@ -438,8 +444,8 @@ void CFX_ByteString::ConcatCopy(FX_STRSIZE nSrc1Len, FX_LPCSTR lpszSrc1Data, } m_pData = FX_AllocString(nNewLen); if (m_pData) { - FXSYS_memcpy32(m_pData->m_String, lpszSrc1Data, nSrc1Len * sizeof(char)); - FXSYS_memcpy32(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len * sizeof(char)); + FXSYS_memcpy32(m_pData->m_String, lpszSrc1Data, nSrc1Len); + FXSYS_memcpy32(m_pData->m_String + nSrc1Len, lpszSrc2Data, nSrc2Len); } } CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst) const @@ -467,20 +473,20 @@ CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst, FX_STRSIZE nCount) const return *this; } CFX_ByteString dest; - AllocCopy(dest, nCount, nFirst, 0); + AllocCopy(dest, nCount, nFirst); return dest; } -void CFX_ByteString::AllocCopy(CFX_ByteString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex, - FX_STRSIZE nExtraLen) const +void CFX_ByteString::AllocCopy(CFX_ByteString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex) const { - FX_STRSIZE nNewLen = nCopyLen + nExtraLen; - if (nNewLen == 0) { + // |FX_STRSIZE| is currently typedef'd as in |int|. TODO(palmer): It + // should be a |size_t|, or at least unsigned. + if (nCopyLen == 0 || nCopyLen < 0) { return; } ASSERT(dest.m_pData == NULL); - dest.m_pData = FX_AllocString(nNewLen); + dest.m_pData = FX_AllocString(nCopyLen); if (dest.m_pData) { - FXSYS_memcpy32(dest.m_pData->m_String, m_pData->m_String + nCopyIndex, nCopyLen * sizeof(char)); + FXSYS_memcpy32(dest.m_pData->m_String, m_pData->m_String + nCopyIndex, nCopyLen); } } #define FORCE_ANSI 0x10000 @@ -724,14 +730,14 @@ FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE nIndex, FX_CHAR ch) return 0; } if(pOldData != NULL) { - FXSYS_memmove32(m_pData->m_String, pstr, (pOldData->m_nDataLength + 1)*sizeof(char)); + FXSYS_memmove32(m_pData->m_String, pstr, (pOldData->m_nDataLength + 1)); FX_ReleaseString(pOldData); } else { m_pData->m_String[0] = 0; } } FXSYS_memmove32(m_pData->m_String + nIndex + 1, - m_pData->m_String + nIndex, (nNewLength - nIndex)*sizeof(char)); + m_pData->m_String + nIndex, (nNewLength - nIndex)); m_pData->m_String[nIndex] = ch; m_pData->m_nDataLength = nNewLength; return nNewLength; @@ -748,7 +754,7 @@ CFX_ByteString CFX_ByteString::Right(FX_STRSIZE nCount) const return *this; } CFX_ByteString dest; - AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount, 0); + AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount); return dest; } CFX_ByteString CFX_ByteString::Left(FX_STRSIZE nCount) const @@ -763,7 +769,7 @@ CFX_ByteString CFX_ByteString::Left(FX_STRSIZE nCount) const return *this; } CFX_ByteString dest; - AllocCopy(dest, nCount, 0, 0); + AllocCopy(dest, nCount, 0); return dest; } FX_STRSIZE CFX_ByteString::Find(FX_CHAR ch, FX_STRSIZE nStart) const diff --git a/core/src/fxcrt/fx_basic_wstring.cpp b/core/src/fxcrt/fx_basic_wstring.cpp index 7af3303ca2..68a65d5b6b 100644 --- a/core/src/fxcrt/fx_basic_wstring.cpp +++ b/core/src/fxcrt/fx_basic_wstring.cpp @@ -5,15 +5,26 @@ // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com #include "../../include/fxcrt/fx_basic.h" +#include "../../../third_party/numerics/safe_math.h" + static CFX_StringDataW* FX_AllocStringW(int nLen) { - if (nLen == 0) { + if (nLen == 0 || nLen < 0) { return NULL; } - CFX_StringDataW* pData = (CFX_StringDataW*)FX_Alloc(FX_BYTE, sizeof(long) * 3 + (nLen + 1) * sizeof(FX_WCHAR)); + + base::CheckedNumeric iSize = static_cast(sizeof(FX_WCHAR)); + iSize *= nLen + 1; + iSize += sizeof(long) * 3; + CFX_StringDataW* pData = (CFX_StringDataW*)FX_Alloc(FX_BYTE, iSize.ValueOrDie()); if (!pData) { return NULL; } + + // TODO(palmer): |nLen| should really be declared as |size_t|, but for + // now I just want to fix the overflow without changing any interfaces. + // Declaring |nLen| as |size_t| will also simplify the above code + // somewhat. pData->m_nAllocLength = nLen; pData->m_nDataLength = nLen; pData->m_nRefs = 1; @@ -421,17 +432,19 @@ CFX_WideString CFX_WideString::FromUTF16LE(const unsigned short* wstr, FX_STRSIZ result.ReleaseBuffer(wlen); return result; } -void CFX_WideString::AllocCopy(CFX_WideString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex, - FX_STRSIZE nExtraLen) const +void CFX_WideString::AllocCopy(CFX_WideString& dest, FX_STRSIZE nCopyLen, FX_STRSIZE nCopyIndex) const { - FX_STRSIZE nNewLen = nCopyLen + nExtraLen; - if (nNewLen == 0) { + // |FX_STRSIZE| is currently typedef'd as in |int|. TODO(palmer): It + // should be a |size_t|, or at least unsigned. + if (nCopyLen == 0 || nCopyLen < 0) { return; } + base::CheckedNumeric iSize = static_cast(sizeof(FX_WCHAR)); + iSize *= nCopyLen; ASSERT(dest.m_pData == NULL); - dest.m_pData = FX_AllocStringW(nNewLen); + dest.m_pData = FX_AllocStringW(nCopyLen); if (dest.m_pData) { - FXSYS_memcpy32(dest.m_pData->m_String, m_pData->m_String + nCopyIndex, nCopyLen * sizeof(FX_WCHAR)); + FXSYS_memcpy32(dest.m_pData->m_String, m_pData->m_String + nCopyIndex, iSize.ValueOrDie()); } } CFX_WideString CFX_WideString::Left(FX_STRSIZE nCount) const @@ -446,7 +459,7 @@ CFX_WideString CFX_WideString::Left(FX_STRSIZE nCount) const return *this; } CFX_WideString dest; - AllocCopy(dest, nCount, 0, 0); + AllocCopy(dest, nCount, 0); return dest; } CFX_WideString CFX_WideString::Mid(FX_STRSIZE nFirst) const @@ -474,7 +487,7 @@ CFX_WideString CFX_WideString::Mid(FX_STRSIZE nFirst, FX_STRSIZE nCount) const return *this; } CFX_WideString dest; - AllocCopy(dest, nCount, nFirst, 0); + AllocCopy(dest, nCount, nFirst); return dest; } CFX_WideString CFX_WideString::Right(FX_STRSIZE nCount) const @@ -489,7 +502,7 @@ CFX_WideString CFX_WideString::Right(FX_STRSIZE nCount) const return *this; } CFX_WideString dest; - AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount, 0); + AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount); return dest; } int CFX_WideString::CompareNoCase(FX_LPCWSTR lpsz) const -- cgit v1.2.3