From ddb9b7cdd19b63a81c4a094239e85f84acefaa17 Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Fri, 11 Aug 2017 16:20:32 -0400 Subject: Add checks of index operations on string classes Specifically the index parameter passed in to GetAt(), SetAt() and operator[] are now being tested to be in bounds. BUG=chromium:752480, pdfium:828 Change-Id: I9e94d58c98a8eaaaae53cd0e3ffe2123ea17d8c4 Reviewed-on: https://pdfium-review.googlesource.com/10651 Commit-Queue: Ryan Harrison Reviewed-by: Tom Sepez --- core/fpdfapi/font/cpdf_cmapparser.cpp | 4 +- core/fpdfdoc/cpdf_variabletext.cpp | 10 ++--- core/fpdftext/cpdf_textpage.cpp | 12 +++-- core/fxcrt/cfx_bytestring.cpp | 10 ++--- core/fxcrt/cfx_bytestring.h | 11 +++-- core/fxcrt/cfx_bytestring_unittest.cpp | 80 ++++++++++++++++++++++++++------- core/fxcrt/cfx_string_c_template.h | 11 +++-- core/fxcrt/cfx_widestring.cpp | 10 ++--- core/fxcrt/cfx_widestring.h | 11 +++-- core/fxcrt/cfx_widestring_unittest.cpp | 74 +++++++++++++++++++++++++++--- fxbarcode/oned/BC_OnedCode128Writer.cpp | 2 +- xfa/fxfa/fm2js/cxfa_fmexpression.cpp | 2 +- 12 files changed, 172 insertions(+), 65 deletions(-) diff --git a/core/fpdfapi/font/cpdf_cmapparser.cpp b/core/fpdfapi/font/cpdf_cmapparser.cpp index 0f9bca6018..cbf693966e 100644 --- a/core/fpdfapi/font/cpdf_cmapparser.cpp +++ b/core/fpdfapi/font/cpdf_cmapparser.cpp @@ -35,7 +35,7 @@ CIDSet CIDSetFromSizeT(size_t index) { CFX_ByteStringC CMap_GetString(const CFX_ByteStringC& word) { if (word.GetLength() <= 2) return CFX_ByteStringC(); - return CFX_ByteStringC(&word[1], word.GetLength() - 2); + return word.Right(word.GetLength() - 2); } } // namespace @@ -137,6 +137,8 @@ void CPDF_CMapParser::ParseWord(const CFX_ByteStringC& word) { // Static. uint32_t CPDF_CMapParser::CMap_GetCode(const CFX_ByteStringC& word) { + if (word.IsEmpty()) + return 0; pdfium::base::CheckedNumeric num = 0; if (word.GetAt(0) == '<') { for (int i = 1; i < word.GetLength() && std::isxdigit(word.GetAt(i)); ++i) { diff --git a/core/fpdfdoc/cpdf_variabletext.cpp b/core/fpdfdoc/cpdf_variabletext.cpp index 1fc32946fc..74a5e3adfc 100644 --- a/core/fpdfdoc/cpdf_variabletext.cpp +++ b/core/fpdfdoc/cpdf_variabletext.cpp @@ -430,18 +430,16 @@ void CPDF_VariableText::SetText(const CFX_WideString& swText) { switch (word) { case 0x0D: if (m_bMultiLine) { - if (swText.GetAt(i + 1) == 0x0A) - i += 1; - + if (i + 1 < sz && swText.GetAt(i + 1) == 0x0A) + i++; wp.AdvanceSection(); AddSection(wp, secinfo); } break; case 0x0A: if (m_bMultiLine) { - if (swText.GetAt(i + 1) == 0x0D) - i += 1; - + if (i + 1 < sz && swText.GetAt(i + 1) == 0x0D) + i++; wp.AdvanceSection(); AddSection(wp, secinfo); } diff --git a/core/fpdftext/cpdf_textpage.cpp b/core/fpdftext/cpdf_textpage.cpp index 8f0e376dbc..1470ad700d 100644 --- a/core/fpdftext/cpdf_textpage.cpp +++ b/core/fpdftext/cpdf_textpage.cpp @@ -945,9 +945,9 @@ bool CPDF_TextPage::IsRightToLeft(const CPDF_TextObject* pTextObj, if (item.m_CharCode == static_cast(-1)) continue; CFX_WideString wstrItem = pFont->UnicodeFromCharCode(item.m_CharCode); - wchar_t wChar = wstrItem.GetAt(0); - if ((wstrItem.IsEmpty() || wChar == 0) && item.m_CharCode) - wChar = (wchar_t)item.m_CharCode; + wchar_t wChar = !wstrItem.IsEmpty() ? wstrItem[0] : 0; + if (wChar == 0) + wChar = item.m_CharCode; if (wChar) str += wChar; } @@ -1222,9 +1222,11 @@ bool CPDF_TextPage::IsHyphen(wchar_t curChar) { if (strCurText.IsEmpty()) strCurText = m_TextBuf.AsStringC(); FX_STRSIZE nCount = strCurText.GetLength(); + if (nCount < 1) + return false; int nIndex = nCount - 1; wchar_t wcTmp = strCurText.GetAt(nIndex); - while (wcTmp == 0x20 && nIndex <= nCount - 1 && nIndex >= 0) + while (wcTmp == 0x20 && nIndex > 0 && nIndex <= nCount - 1) wcTmp = strCurText.GetAt(--nIndex); if (0x2D == wcTmp || 0xAD == wcTmp) { if (--nIndex > 0) { @@ -1353,6 +1355,8 @@ CPDF_TextPage::GenerateCharacter CPDF_TextPage::ProcessInsertObject( } CFX_WideString PrevStr = m_pPreTextObj->GetFont()->UnicodeFromCharCode(PrevItem.m_CharCode); + if (PrevStr.IsEmpty()) + return GenerateCharacter::None; wchar_t preChar = PrevStr.GetAt(PrevStr.GetLength() - 1); CFX_Matrix matrix = pObj->GetTextMatrix(); matrix.Concat(formMatrix); diff --git a/core/fxcrt/cfx_bytestring.cpp b/core/fxcrt/cfx_bytestring.cpp index c29d24affd..0aba3be12a 100644 --- a/core/fxcrt/cfx_bytestring.cpp +++ b/core/fxcrt/cfx_bytestring.cpp @@ -694,14 +694,10 @@ FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld, return nCount; } -void CFX_ByteString::SetAt(FX_STRSIZE nIndex, char ch) { - if (!m_pData) { - return; - } - ASSERT(nIndex >= 0); - ASSERT(nIndex < m_pData->m_nDataLength); +void CFX_ByteString::SetAt(FX_STRSIZE index, char c) { + ASSERT(index >= 0 && index < GetLength()); ReallocBeforeWrite(m_pData->m_nDataLength); - m_pData->m_String[nIndex] = ch; + m_pData->m_String[index] = c; } CFX_WideString CFX_ByteString::UTF8Decode() const { diff --git a/core/fxcrt/cfx_bytestring.h b/core/fxcrt/cfx_bytestring.h index cf688b4c88..df31751273 100644 --- a/core/fxcrt/cfx_bytestring.h +++ b/core/fxcrt/cfx_bytestring.h @@ -107,15 +107,14 @@ class CFX_ByteString { const CFX_ByteString& operator+=(const CFX_ByteString& str); const CFX_ByteString& operator+=(const CFX_ByteStringC& bstrc); - uint8_t GetAt(FX_STRSIZE nIndex) const { - return m_pData ? m_pData->m_String[nIndex] : 0; + uint8_t GetAt(FX_STRSIZE index) const { + ASSERT(index >= 0 && index < GetLength()); + return m_pData->m_String[index]; } - uint8_t operator[](FX_STRSIZE nIndex) const { - return m_pData ? m_pData->m_String[nIndex] : 0; - } + uint8_t operator[](FX_STRSIZE index) const { return GetAt(index); } - void SetAt(FX_STRSIZE nIndex, char ch); + void SetAt(FX_STRSIZE index, char c); FX_STRSIZE Insert(FX_STRSIZE index, char ch); FX_STRSIZE InsertAtFront(char ch) { return Insert(0, ch); } FX_STRSIZE InsertAtBack(char ch) { return Insert(GetLength(), ch); } diff --git a/core/fxcrt/cfx_bytestring_unittest.cpp b/core/fxcrt/cfx_bytestring_unittest.cpp index b79a765e20..680a37e342 100644 --- a/core/fxcrt/cfx_bytestring_unittest.cpp +++ b/core/fxcrt/cfx_bytestring_unittest.cpp @@ -11,13 +11,52 @@ #include "testing/gtest/include/gtest/gtest.h" #include "third_party/base/stl_util.h" +TEST(fxcrt, ByteStringGetAt) { + CFX_ByteString short_string("a"); + CFX_ByteString longer_string("abc"); + CFX_ByteString embedded_nul_string("ab\0c", 4); + +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); +#endif + EXPECT_EQ('a', short_string.GetAt(0)); +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); +#endif + EXPECT_EQ('c', longer_string.GetAt(2)); + EXPECT_EQ('b', embedded_nul_string.GetAt(1)); + EXPECT_EQ('\0', embedded_nul_string.GetAt(2)); + EXPECT_EQ('c', embedded_nul_string.GetAt(3)); +} + TEST(fxcrt, ByteStringOperatorSubscript) { - // CFX_ByteString includes the NUL terminator for non-empty strings. CFX_ByteString abc("abc"); +#ifndef NDEBUG + EXPECT_DEATH({ abc[-1]; }, ".*"); +#endif EXPECT_EQ('a', abc[0]); EXPECT_EQ('b', abc[1]); EXPECT_EQ('c', abc[2]); - EXPECT_EQ(0, abc[3]); +#ifndef NDEBUG + EXPECT_DEATH({ abc[3]; }, ".*"); +#endif +} + +TEST(fxcrt, ByteStringSetAt) { + // CFX_ByteString includes the NUL terminator for non-empty strings. + CFX_ByteString abc("abc"); +#ifndef NDEBUG + EXPECT_DEATH({ abc.SetAt(-1, 'd'); }, ".*"); +#endif + abc.SetAt(0, 'd'); + EXPECT_EQ("dbc", abc); + abc.SetAt(1, 'e'); + EXPECT_EQ("dec", abc); + abc.SetAt(2, 'f'); + EXPECT_EQ("def", abc); +#ifndef NDEBUG + EXPECT_DEATH({ abc.SetAt(3, 'g'); }, ".*"); +#endif } TEST(fxcrt, ByteStringOperatorLT) { @@ -905,24 +944,35 @@ TEST(fxcrt, ByteStringCMid) { } TEST(fxcrt, ByteStringCGetAt) { - CFX_ByteString short_string("a"); - CFX_ByteString longer_string("abc"); - CFX_ByteString embedded_nul_string("ab\0c", 4); - - EXPECT_EQ('a', short_string.GetAt(0)); - EXPECT_EQ('c', longer_string.GetAt(2)); - EXPECT_EQ('b', embedded_nul_string.GetAt(1)); - EXPECT_EQ('\0', embedded_nul_string.GetAt(2)); - EXPECT_EQ('c', embedded_nul_string.GetAt(3)); + CFX_ByteStringC short_string("a"); + CFX_ByteStringC longer_string("abc"); + CFX_ByteStringC embedded_nul_string("ab\0c", 4); + +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); +#endif + EXPECT_EQ('a', static_cast(short_string.GetAt(0))); +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); +#endif + EXPECT_EQ('c', static_cast(longer_string.GetAt(2))); + EXPECT_EQ('b', static_cast(embedded_nul_string.GetAt(1))); + EXPECT_EQ('\0', static_cast(embedded_nul_string.GetAt(2))); + EXPECT_EQ('c', static_cast(embedded_nul_string.GetAt(3))); } TEST(fxcrt, ByteStringCOperatorSubscript) { // CFX_ByteStringC includes the NUL terminator for non-empty strings. CFX_ByteStringC abc("abc"); - EXPECT_EQ('a', abc[0]); - EXPECT_EQ('b', abc[1]); - EXPECT_EQ('c', abc[2]); - EXPECT_EQ(0, abc[3]); +#ifndef NDEBUG + EXPECT_DEATH({ abc[-1]; }, ".*"); +#endif + EXPECT_EQ('a', static_cast(abc[0])); + EXPECT_EQ('b', static_cast(abc[1])); + EXPECT_EQ('c', static_cast(abc[2])); +#ifndef NDEBUG + EXPECT_DEATH({ abc[3]; }, ".*"); +#endif } TEST(fxcrt, ByteStringCOperatorLT) { diff --git a/core/fxcrt/cfx_string_c_template.h b/core/fxcrt/cfx_string_c_template.h index 6a95a05d45..bc0fe1e0a0 100644 --- a/core/fxcrt/cfx_string_c_template.h +++ b/core/fxcrt/cfx_string_c_template.h @@ -119,8 +119,13 @@ class CFX_StringCTemplate { FX_STRSIZE GetLength() const { return m_Length; } bool IsEmpty() const { return m_Length == 0; } - UnsignedType GetAt(FX_STRSIZE index) const { return m_Ptr.Get()[index]; } + UnsignedType GetAt(FX_STRSIZE index) const { + ASSERT(index >= 0 && index < GetLength()); + return m_Ptr.Get()[index]; + } + CharType CharAt(FX_STRSIZE index) const { + ASSERT(index >= 0 && index < GetLength()); return static_cast(m_Ptr.Get()[index]); } @@ -159,9 +164,7 @@ class CFX_StringCTemplate { return CFX_StringCTemplate(m_Ptr.Get() + m_Length - count, count); } - const UnsignedType& operator[](size_t index) const { - return m_Ptr.Get()[index]; - } + UnsignedType operator[](FX_STRSIZE index) const { return GetAt(index); } bool operator<(const CFX_StringCTemplate& that) const { int result = FXSYS_cmp(reinterpret_cast(m_Ptr.Get()), diff --git a/core/fxcrt/cfx_widestring.cpp b/core/fxcrt/cfx_widestring.cpp index a1fa7ec096..c2e1e4877d 100644 --- a/core/fxcrt/cfx_widestring.cpp +++ b/core/fxcrt/cfx_widestring.cpp @@ -841,14 +841,10 @@ FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld, return nCount; } -void CFX_WideString::SetAt(FX_STRSIZE nIndex, wchar_t ch) { - if (!m_pData) { - return; - } - ASSERT(nIndex >= 0); - ASSERT(nIndex < m_pData->m_nDataLength); +void CFX_WideString::SetAt(FX_STRSIZE index, wchar_t c) { + ASSERT(index >= 0 && index < GetLength()); ReallocBeforeWrite(m_pData->m_nDataLength); - m_pData->m_String[nIndex] = ch; + m_pData->m_String[index] = c; } // static diff --git a/core/fxcrt/cfx_widestring.h b/core/fxcrt/cfx_widestring.h index ccb1e752f7..b49e898f32 100644 --- a/core/fxcrt/cfx_widestring.h +++ b/core/fxcrt/cfx_widestring.h @@ -101,15 +101,14 @@ class CFX_WideString { bool operator<(const CFX_WideString& str) const; - wchar_t GetAt(FX_STRSIZE nIndex) const { - return m_pData ? m_pData->m_String[nIndex] : 0; + wchar_t GetAt(FX_STRSIZE index) const { + ASSERT(index >= 0 && index < GetLength()); + return m_pData->m_String[index]; } - wchar_t operator[](FX_STRSIZE nIndex) const { - return m_pData ? m_pData->m_String[nIndex] : 0; - } + wchar_t operator[](FX_STRSIZE index) const { return GetAt(index); } - void SetAt(FX_STRSIZE nIndex, wchar_t ch); + void SetAt(FX_STRSIZE index, wchar_t c); int Compare(const wchar_t* str) const; int Compare(const CFX_WideString& str) const; diff --git a/core/fxcrt/cfx_widestring_unittest.cpp b/core/fxcrt/cfx_widestring_unittest.cpp index 6e7b63c5fa..a23763218d 100644 --- a/core/fxcrt/cfx_widestring_unittest.cpp +++ b/core/fxcrt/cfx_widestring_unittest.cpp @@ -10,13 +10,51 @@ #include "testing/gtest/include/gtest/gtest.h" +TEST(fxcrt, WideStringGetAt) { + CFX_WideString short_string(L"a"); + CFX_WideString longer_string(L"abc"); + CFX_WideString embedded_nul_string(L"ab\0c", 4); + +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); +#endif + EXPECT_EQ(L'a', short_string.GetAt(0)); +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); +#endif + EXPECT_EQ(L'c', longer_string.GetAt(2)); + EXPECT_EQ(L'b', embedded_nul_string.GetAt(1)); + EXPECT_EQ(L'\0', embedded_nul_string.GetAt(2)); + EXPECT_EQ(L'c', embedded_nul_string.GetAt(3)); +} + TEST(fxcrt, WideStringOperatorSubscript) { - // CFX_WideString includes the NUL terminator for non-empty strings. CFX_WideString abc(L"abc"); +#ifndef NDEBUG + EXPECT_DEATH({ abc[-1]; }, ".*"); +#endif EXPECT_EQ(L'a', abc[0]); EXPECT_EQ(L'b', abc[1]); EXPECT_EQ(L'c', abc[2]); - EXPECT_EQ(L'\0', abc[3]); +#ifndef NDEBUG + EXPECT_DEATH({ abc[4]; }, ".*"); +#endif +} + +TEST(fxcrt, WideStringSetAt) { + CFX_WideString abc(L"abc"); +#ifndef NDEBUG + EXPECT_DEATH({ abc.SetAt(-1, L'd'); }, ".*"); +#endif + abc.SetAt(0, L'd'); + EXPECT_EQ(L"dbc", abc); + abc.SetAt(1, L'e'); + EXPECT_EQ(L"dec", abc); + abc.SetAt(2, L'f'); + EXPECT_EQ(L"def", abc); +#ifndef NDEBUG + EXPECT_DEATH({ abc.SetAt(3, L'g'); }, ".*"); +#endif } TEST(fxcrt, WideStringOperatorLT) { @@ -742,13 +780,35 @@ TEST(fxcrt, WideStringCFromVector) { EXPECT_EQ(nullptr, cleared_string.raw_str()); } +TEST(fxcrt, WideStringCGetAt) { + CFX_WideStringC short_string(L"a"); + CFX_WideStringC longer_string(L"abc"); + CFX_WideStringC embedded_nul_string(L"ab\0c", 4); + +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(-1); }, ".*"); +#endif + EXPECT_EQ(L'a', static_cast(short_string.GetAt(0))); +#ifndef NDEBUG + EXPECT_DEATH({ short_string.GetAt(1); }, ".*"); +#endif + EXPECT_EQ(L'c', static_cast(longer_string.GetAt(2))); + EXPECT_EQ(L'b', static_cast(embedded_nul_string.GetAt(1))); + EXPECT_EQ(L'\0', static_cast(embedded_nul_string.GetAt(2))); + EXPECT_EQ(L'c', static_cast(embedded_nul_string.GetAt(3))); +} + TEST(fxcrt, WideStringCOperatorSubscript) { - // CFX_WideStringC includes the NUL terminator for non-empty strings. CFX_WideStringC abc(L"abc"); - EXPECT_EQ(L'a', abc.CharAt(0)); - EXPECT_EQ(L'b', abc.CharAt(1)); - EXPECT_EQ(L'c', abc.CharAt(2)); - EXPECT_EQ(L'\0', abc.CharAt(3)); +#ifndef NDEBUG + EXPECT_DEATH({ abc[-1]; }, ".*"); +#endif + EXPECT_EQ(L'a', static_cast(abc[0])); + EXPECT_EQ(L'b', static_cast(abc[1])); + EXPECT_EQ(L'c', static_cast(abc[2])); +#ifndef NDEBUG + EXPECT_DEATH({ abc[4]; }, ".*"); +#endif } TEST(fxcrt, WideStringCOperatorLT) { diff --git a/fxbarcode/oned/BC_OnedCode128Writer.cpp b/fxbarcode/oned/BC_OnedCode128Writer.cpp index 644933d386..95a95b69e2 100644 --- a/fxbarcode/oned/BC_OnedCode128Writer.cpp +++ b/fxbarcode/oned/BC_OnedCode128Writer.cpp @@ -195,7 +195,7 @@ int32_t CBC_OnedCode128Writer::Encode128C(const CFX_ByteString& contents, if (std::isdigit(ch)) { patternIndex = FXSYS_atoi(contents.Mid(position, 2).c_str()); ++position; - if (std::isdigit(contents[position])) + if (position < contents.GetLength() && std::isdigit(contents[position])) ++position; } else { patternIndex = static_cast(ch); diff --git a/xfa/fxfa/fm2js/cxfa_fmexpression.cpp b/xfa/fxfa/fm2js/cxfa_fmexpression.cpp index a74239ff82..c6b5814d6d 100644 --- a/xfa/fxfa/fm2js/cxfa_fmexpression.cpp +++ b/xfa/fxfa/fm2js/cxfa_fmexpression.cpp @@ -60,7 +60,7 @@ bool CXFA_FMFunctionDefinition::ToJavaScript(CFX_WideTextBuf& javascript) { javascript << L"(\n"; } javascript << L"function "; - if (m_wsName.GetAt(0) == L'!') { + if (!m_wsName.IsEmpty() && m_wsName[0] == L'!') { CFX_WideString tempName = EXCLAMATION_IN_IDENTIFIER + m_wsName.Right(m_wsName.GetLength() - 1); javascript << tempName; -- cgit v1.2.3