From db14532fb2637b34f0926b6c3a931132854f53bb Mon Sep 17 00:00:00 2001 From: Ryan Harrison Date: Wed, 2 Aug 2017 14:44:17 -0400 Subject: Rewrite how string Insert() methods handle out of bound indices The existing behaviour was to clamp the provided index to the valid bounds. This would lead to programming errors being hidden, since inserting would still do something even if the index calculation was wrong. The behaviour of these methods has been changed to instead early return when this occurs, returning the old length value. The caller can check if the call to Insert actually did anything by comparing the returned value to the length before calling insert. All of the existing calls to Insert have been tested by running all of the tests with asserts in the Insert method to check the index is in bounds. Additionally the call sites have been manually inspected. The majority of them are of the form Insert(0, foo) and the rest tend to be in a loop advancing from 0 to length. Convenience methods InsertAtFront/InsertAtBack have been added to handle calling Insert when the intent is for the character to be added to the beginning or end of the string. Existing call sites to Insert that do this have been converted. This work was originally being performed to check if there would be any issues in these methods with making FX_STRSIZE unsigned. BUG=pdfium:828 Change-Id: I60cee5ad45338aa8ed46569de7bcc78a76db18f7 Reviewed-on: https://pdfium-review.googlesource.com/9870 Commit-Queue: Ryan Harrison Reviewed-by: Tom Sepez --- core/fpdfapi/font/cpdf_tounicodemap.cpp | 9 ++-- core/fxcrt/cfx_bytestring.cpp | 24 +++++----- core/fxcrt/cfx_bytestring.h | 2 + core/fxcrt/cfx_bytestring_unittest.cpp | 70 ++++++++++++++++++++++------- core/fxcrt/cfx_widestring.cpp | 24 +++++----- core/fxcrt/cfx_widestring.h | 2 + core/fxcrt/cfx_widestring_unittest.cpp | 80 ++++++++++++++++++++++----------- fxbarcode/BC_Utils.cpp | 2 +- xfa/fgas/crt/cfgas_formatstring.cpp | 26 +++++------ xfa/fxfa/parser/cxfa_widgetdata.cpp | 2 +- 10 files changed, 155 insertions(+), 86 deletions(-) diff --git a/core/fpdfapi/font/cpdf_tounicodemap.cpp b/core/fpdfapi/font/cpdf_tounicodemap.cpp index 8989f0be37..30bf0317ee 100644 --- a/core/fpdfapi/font/cpdf_tounicodemap.cpp +++ b/core/fpdfapi/font/cpdf_tounicodemap.cpp @@ -77,15 +77,14 @@ static CFX_WideString StringDataAdd(CFX_WideString str) { for (int i = len - 1; i >= 0; --i) { wchar_t ch = str[i] + value; if (ch < str[i]) { - ret.Insert(0, 0); + ret.InsertAtFront(0); } else { - ret.Insert(0, ch); + ret.InsertAtFront(ch); value = 0; } } - if (value) { - ret.Insert(0, value); - } + if (value) + ret.InsertAtFront(value); return ret; } diff --git a/core/fxcrt/cfx_bytestring.cpp b/core/fxcrt/cfx_bytestring.cpp index 5dcaf613a0..e031c87d2d 100644 --- a/core/fxcrt/cfx_bytestring.cpp +++ b/core/fxcrt/cfx_bytestring.cpp @@ -518,18 +518,18 @@ void CFX_ByteString::Format(const char* pFormat, ...) { va_end(argList); } -FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE nIndex, char ch) { - FX_STRSIZE nNewLength = m_pData ? m_pData->m_nDataLength : 0; - nIndex = std::max(nIndex, 0); - nIndex = std::min(nIndex, nNewLength); - nNewLength++; - - ReallocBeforeWrite(nNewLength); - memmove(m_pData->m_String + nIndex + 1, m_pData->m_String + nIndex, - nNewLength - nIndex); - m_pData->m_String[nIndex] = ch; - m_pData->m_nDataLength = nNewLength; - return nNewLength; +FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE index, char ch) { + const FX_STRSIZE cur_length = m_pData ? m_pData->m_nDataLength : 0; + if (index != pdfium::clamp(index, 0, cur_length)) + return cur_length; + + const FX_STRSIZE new_length = cur_length + 1; + ReallocBeforeWrite(new_length); + memmove(m_pData->m_String + index + 1, m_pData->m_String + index, + new_length - index); + m_pData->m_String[index] = ch; + m_pData->m_nDataLength = new_length; + return new_length; } CFX_ByteString CFX_ByteString::Right(FX_STRSIZE nCount) const { diff --git a/core/fxcrt/cfx_bytestring.h b/core/fxcrt/cfx_bytestring.h index 8bd9f39fc0..df1b8309fc 100644 --- a/core/fxcrt/cfx_bytestring.h +++ b/core/fxcrt/cfx_bytestring.h @@ -118,6 +118,8 @@ class CFX_ByteString { void SetAt(FX_STRSIZE nIndex, char ch); 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); } FX_STRSIZE Delete(FX_STRSIZE index, FX_STRSIZE count = 1); void Format(const char* lpszFormat, ...); diff --git a/core/fxcrt/cfx_bytestring_unittest.cpp b/core/fxcrt/cfx_bytestring_unittest.cpp index 4379519046..89c8ac6060 100644 --- a/core/fxcrt/cfx_bytestring_unittest.cpp +++ b/core/fxcrt/cfx_bytestring_unittest.cpp @@ -387,32 +387,68 @@ TEST(fxcrt, ByteStringReplace) { TEST(fxcrt, ByteStringInsert) { CFX_ByteString fred("FRED"); - fred.Insert(-1, 'X'); - EXPECT_EQ("XFRED", fred); - fred.Insert(0, 'S'); - EXPECT_EQ("SXFRED", fred); - fred.Insert(2, 'T'); - EXPECT_EQ("SXTFRED", fred); - fred.Insert(5, 'U'); - EXPECT_EQ("SXTFRUED", fred); - fred.Insert(8, 'V'); - EXPECT_EQ("SXTFRUEDV", fred); - fred.Insert(12, 'P'); - EXPECT_EQ("SXTFRUEDVP", fred); + EXPECT_EQ(4, fred.Insert(-1, 'X')); + EXPECT_EQ("FRED", fred); + EXPECT_EQ(5, fred.Insert(0, 'S')); + EXPECT_EQ("SFRED", fred); + EXPECT_EQ(6, fred.Insert(1, 'T')); + EXPECT_EQ("STFRED", fred); + EXPECT_EQ(7, fred.Insert(4, 'U')); + EXPECT_EQ("STFRUED", fred); + EXPECT_EQ(8, fred.Insert(7, 'V')); + EXPECT_EQ("STFRUEDV", fred); + EXPECT_EQ(8, fred.Insert(12, 'P')); + EXPECT_EQ("STFRUEDV", fred); { CFX_ByteString empty; - empty.Insert(-1, 'X'); - EXPECT_EQ("X", empty); + EXPECT_EQ(0, empty.Insert(-1, 'X')); + EXPECT_NE("X", empty); } { CFX_ByteString empty; - empty.Insert(0, 'X'); + EXPECT_EQ(1, empty.Insert(0, 'X')); EXPECT_EQ("X", empty); } { CFX_ByteString empty; - empty.Insert(5, 'X'); - EXPECT_EQ("X", empty); + EXPECT_EQ(0, empty.Insert(5, 'X')); + EXPECT_NE("X", empty); + } +} + +TEST(fxcrt, ByteStringInsertAtFrontAndInsertAtBack) { + { + CFX_ByteString empty; + EXPECT_EQ(1, empty.InsertAtFront('D')); + EXPECT_EQ("D", empty); + EXPECT_EQ(2, empty.InsertAtFront('E')); + EXPECT_EQ("ED", empty); + EXPECT_EQ(3, empty.InsertAtFront('R')); + EXPECT_EQ("RED", empty); + EXPECT_EQ(4, empty.InsertAtFront('F')); + EXPECT_EQ("FRED", empty); + } + { + CFX_ByteString empty; + EXPECT_EQ(1, empty.InsertAtBack('F')); + EXPECT_EQ("F", empty); + EXPECT_EQ(2, empty.InsertAtBack('R')); + EXPECT_EQ("FR", empty); + EXPECT_EQ(3, empty.InsertAtBack('E')); + EXPECT_EQ("FRE", empty); + EXPECT_EQ(4, empty.InsertAtBack('D')); + EXPECT_EQ("FRED", empty); + } + { + CFX_ByteString empty; + EXPECT_EQ(1, empty.InsertAtBack('E')); + EXPECT_EQ("E", empty); + EXPECT_EQ(2, empty.InsertAtFront('R')); + EXPECT_EQ("RE", empty); + EXPECT_EQ(3, empty.InsertAtBack('D')); + EXPECT_EQ("RED", empty); + EXPECT_EQ(4, empty.InsertAtFront('F')); + EXPECT_EQ("FRED", empty); } } diff --git a/core/fxcrt/cfx_widestring.cpp b/core/fxcrt/cfx_widestring.cpp index b83752369b..6c079b354e 100644 --- a/core/fxcrt/cfx_widestring.cpp +++ b/core/fxcrt/cfx_widestring.cpp @@ -680,18 +680,18 @@ void CFX_WideString::Format(const wchar_t* pFormat, ...) { va_end(argList); } -FX_STRSIZE CFX_WideString::Insert(FX_STRSIZE nIndex, wchar_t ch) { - FX_STRSIZE nNewLength = m_pData ? m_pData->m_nDataLength : 0; - nIndex = std::max(nIndex, 0); - nIndex = std::min(nIndex, nNewLength); - nNewLength++; - - ReallocBeforeWrite(nNewLength); - wmemmove(m_pData->m_String + nIndex + 1, m_pData->m_String + nIndex, - nNewLength - nIndex); - m_pData->m_String[nIndex] = ch; - m_pData->m_nDataLength = nNewLength; - return nNewLength; +FX_STRSIZE CFX_WideString::Insert(FX_STRSIZE index, wchar_t ch) { + const FX_STRSIZE cur_length = m_pData ? m_pData->m_nDataLength : 0; + if (index != pdfium::clamp(index, 0, cur_length)) + return cur_length; + + const FX_STRSIZE new_length = cur_length + 1; + ReallocBeforeWrite(new_length); + wmemmove(m_pData->m_String + index + 1, m_pData->m_String + index, + new_length - index); + m_pData->m_String[index] = ch; + m_pData->m_nDataLength = new_length; + return new_length; } CFX_WideString CFX_WideString::Right(FX_STRSIZE nCount) const { diff --git a/core/fxcrt/cfx_widestring.h b/core/fxcrt/cfx_widestring.h index 02045c5c09..ccb1e752f7 100644 --- a/core/fxcrt/cfx_widestring.h +++ b/core/fxcrt/cfx_widestring.h @@ -120,6 +120,8 @@ class CFX_WideString { CFX_WideString Right(FX_STRSIZE count) const; FX_STRSIZE Insert(FX_STRSIZE index, wchar_t ch); + FX_STRSIZE InsertAtFront(wchar_t ch) { return Insert(0, ch); } + FX_STRSIZE InsertAtBack(wchar_t ch) { return Insert(GetLength(), ch); } FX_STRSIZE Delete(FX_STRSIZE index, FX_STRSIZE count = 1); void Format(const wchar_t* lpszFormat, ...); diff --git a/core/fxcrt/cfx_widestring_unittest.cpp b/core/fxcrt/cfx_widestring_unittest.cpp index b743a17fd1..a763f8abbd 100644 --- a/core/fxcrt/cfx_widestring_unittest.cpp +++ b/core/fxcrt/cfx_widestring_unittest.cpp @@ -347,38 +347,68 @@ TEST(fxcrt, WideStringReplace) { TEST(fxcrt, WideStringInsert) { CFX_WideString fred(L"FRED"); - fred.Insert(-1, 'X'); - EXPECT_EQ(L"XFRED", fred); - - fred.Insert(0, 'S'); - EXPECT_EQ(L"SXFRED", fred); - - fred.Insert(2, 'T'); - EXPECT_EQ(L"SXTFRED", fred); - - fred.Insert(5, 'U'); - EXPECT_EQ(L"SXTFRUED", fred); - - fred.Insert(8, 'V'); - EXPECT_EQ(L"SXTFRUEDV", fred); - - fred.Insert(12, 'P'); - EXPECT_EQ(L"SXTFRUEDVP", fred); - + EXPECT_EQ(4, fred.Insert(-1, 'X')); + EXPECT_EQ(L"FRED", fred); + EXPECT_EQ(5, fred.Insert(0, 'S')); + EXPECT_EQ(L"SFRED", fred); + EXPECT_EQ(6, fred.Insert(1, 'T')); + EXPECT_EQ(L"STFRED", fred); + EXPECT_EQ(7, fred.Insert(4, 'U')); + EXPECT_EQ(L"STFRUED", fred); + EXPECT_EQ(8, fred.Insert(7, 'V')); + EXPECT_EQ(L"STFRUEDV", fred); + EXPECT_EQ(8, fred.Insert(12, 'P')); + EXPECT_EQ(L"STFRUEDV", fred); { CFX_WideString empty; - empty.Insert(-1, 'X'); - EXPECT_EQ(L"X", empty); + EXPECT_EQ(0, empty.Insert(-1, 'X')); + EXPECT_NE(L"X", empty); } { CFX_WideString empty; - empty.Insert(0, 'X'); + EXPECT_EQ(1, empty.Insert(0, 'X')); EXPECT_EQ(L"X", empty); } { CFX_WideString empty; - empty.Insert(5, 'X'); - EXPECT_EQ(L"X", empty); + EXPECT_EQ(0, empty.Insert(5, 'X')); + EXPECT_NE(L"X", empty); + } +} + +TEST(fxcrt, WideStringInsertAtFrontAndInsertAtBack) { + { + CFX_WideString empty; + EXPECT_EQ(1, empty.InsertAtFront('D')); + EXPECT_EQ(L"D", empty); + EXPECT_EQ(2, empty.InsertAtFront('E')); + EXPECT_EQ(L"ED", empty); + EXPECT_EQ(3, empty.InsertAtFront('R')); + EXPECT_EQ(L"RED", empty); + EXPECT_EQ(4, empty.InsertAtFront('F')); + EXPECT_EQ(L"FRED", empty); + } + { + CFX_WideString empty; + EXPECT_EQ(1, empty.InsertAtBack('F')); + EXPECT_EQ(L"F", empty); + EXPECT_EQ(2, empty.InsertAtBack('R')); + EXPECT_EQ(L"FR", empty); + EXPECT_EQ(3, empty.InsertAtBack('E')); + EXPECT_EQ(L"FRE", empty); + EXPECT_EQ(4, empty.InsertAtBack('D')); + EXPECT_EQ(L"FRED", empty); + } + { + CFX_WideString empty; + EXPECT_EQ(1, empty.InsertAtBack('E')); + EXPECT_EQ(L"E", empty); + EXPECT_EQ(2, empty.InsertAtFront('R')); + EXPECT_EQ(L"RE", empty); + EXPECT_EQ(3, empty.InsertAtBack('D')); + EXPECT_EQ(L"RED", empty); + EXPECT_EQ(4, empty.InsertAtFront('F')); + EXPECT_EQ(L"FRED", empty); } } @@ -720,8 +750,8 @@ TEST(fxcrt, WideStringCOperatorSubscript) { TEST(fxcrt, WideStringCOperatorLT) { CFX_WideStringC empty; CFX_WideStringC a(L"a"); - CFX_WideStringC abc(L"\x0110qq"); // Comes before despite endianness. - CFX_WideStringC def(L"\x1001qq"); // Comes after despite endianness. + CFX_WideStringC abc(L"\x0110qq"); // Comes InsertAtFront despite endianness. + CFX_WideStringC def(L"\x1001qq"); // Comes InsertAtBack despite endianness. EXPECT_FALSE(empty < empty); EXPECT_FALSE(a < a); diff --git a/fxbarcode/BC_Utils.cpp b/fxbarcode/BC_Utils.cpp index 3c75406425..6a6ed3e0a4 100644 --- a/fxbarcode/BC_Utils.cpp +++ b/fxbarcode/BC_Utils.cpp @@ -19,7 +19,7 @@ bool BC_FX_ByteString_Replace(CFX_ByteString& dst, } dst.Delete(first, last - first); for (int32_t i = 0; i < count; i++) { - dst.Insert(0, c); + dst.InsertAtFront(c); } return true; } diff --git a/xfa/fgas/crt/cfgas_formatstring.cpp b/xfa/fgas/crt/cfgas_formatstring.cpp index d940247385..b20fa082c3 100644 --- a/xfa/fgas/crt/cfgas_formatstring.cpp +++ b/xfa/fgas/crt/cfgas_formatstring.cpp @@ -1208,7 +1208,7 @@ bool CFGAS_FormatString::ParseNum(const CFX_WideString& wsSrcNum, if (!FXSYS_isDecimalDigit(str[cc])) return false; - wsValue->Insert(0, str[cc]); + wsValue->InsertAtFront(str[cc]); cc--; ccf--; break; @@ -1216,7 +1216,7 @@ bool CFGAS_FormatString::ParseNum(const CFX_WideString& wsSrcNum, case 'Z': if (strf[ccf] == 'z' || str[cc] != ' ') { if (FXSYS_isDecimalDigit(str[cc])) { - wsValue->Insert(0, str[cc]); + wsValue->InsertAtFront(str[cc]); cc--; } } else { @@ -1545,7 +1545,7 @@ bool CFGAS_FormatString::ParseNum(const CFX_WideString& wsSrcNum, *wsValue = decimal; } if (bNeg) - wsValue->Insert(0, L'-'); + wsValue->InsertAtFront(L'-'); return true; } @@ -1878,7 +1878,7 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, CFX_WideString wsSrcNum(wsInputNum); wsSrcNum.TrimLeft('0'); if (wsSrcNum.IsEmpty() || wsSrcNum[0] == '.') - wsSrcNum.Insert(0, '0'); + wsSrcNum.InsertAtFront('0'); CFX_Decimal decimal = CFX_Decimal(wsSrcNum.AsStringC()); if (dwNumStyle & FX_NUMSTYLE_Percent) { @@ -1963,10 +1963,10 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, if (!FXSYS_isDecimalDigit(str[cc])) return false; - wsOutput->Insert(0, str[cc]); + wsOutput->InsertAtFront(str[cc]); cc--; } else { - wsOutput->Insert(0, L'0'); + wsOutput->InsertAtFront(L'0'); } ccf--; break; @@ -1975,7 +1975,7 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, if (!FXSYS_isDecimalDigit(str[cc])) return false; if (str[0] != '0') - wsOutput->Insert(0, str[cc]); + wsOutput->InsertAtFront(str[cc]); cc--; } @@ -1986,10 +1986,10 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, if (!FXSYS_isDecimalDigit(str[cc])) return false; - wsOutput->Insert(0, str[0] == '0' ? L' ' : str[cc]); + wsOutput->InsertAtFront(str[0] == '0' ? L' ' : str[cc]); cc--; } else { - wsOutput->Insert(0, L' '); + wsOutput->InsertAtFront(L' '); } ccf--; break; @@ -1999,7 +1999,7 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, pLocale->GetNumbericSymbol(FX_LOCALENUMSYMBOL_Minus) + *wsOutput; bAddNeg = true; } else { - wsOutput->Insert(0, L' '); + wsOutput->InsertAtFront(L' '); } ccf--; break; @@ -2070,12 +2070,12 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, ccf--; break; case '(': - wsOutput->Insert(0, bNeg ? L'(' : L' '); + wsOutput->InsertAtFront(bNeg ? L'(' : L' '); bAddNeg = true; ccf--; break; case ')': - wsOutput->Insert(0, bNeg ? L')' : L' '); + wsOutput->InsertAtFront(bNeg ? L')' : L' '); ccf--; break; case '\'': @@ -2083,7 +2083,7 @@ bool CFGAS_FormatString::FormatStrNum(const CFX_WideStringC& wsInputNum, ccf--; break; default: - wsOutput->Insert(0, strf[ccf]); + wsOutput->InsertAtFront(strf[ccf]); ccf--; } } diff --git a/xfa/fxfa/parser/cxfa_widgetdata.cpp b/xfa/fxfa/parser/cxfa_widgetdata.cpp index 34e3aeaf0f..d68a907e18 100644 --- a/xfa/fxfa/parser/cxfa_widgetdata.cpp +++ b/xfa/fxfa/parser/cxfa_widgetdata.cpp @@ -1750,7 +1750,7 @@ void CXFA_WidgetData::NormalizeNumStr(const CFX_WideString& wsValue, wsOutput.TrimRight(L"."); } if (wsOutput.IsEmpty() || wsOutput[0] == '.') - wsOutput.Insert(0, '0'); + wsOutput.InsertAtFront('0'); } void CXFA_WidgetData::FormatNumStr(const CFX_WideString& wsValue, -- cgit v1.2.3