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 <rharrison@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/font/cpdf_tounicodemap.cpp b/core/fpdfapi/font/cpdf_tounicodemap.cpp
index 8989f0b..30bf031 100644
--- a/core/fpdfapi/font/cpdf_tounicodemap.cpp
+++ b/core/fpdfapi/font/cpdf_tounicodemap.cpp
@@ -77,15 +77,14 @@
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 5dcaf61..e031c87 100644
--- a/core/fxcrt/cfx_bytestring.cpp
+++ b/core/fxcrt/cfx_bytestring.cpp
@@ -518,18 +518,18 @@
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++;
+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;
- 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;
+ 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 8bd9f39..df1b830 100644
--- a/core/fxcrt/cfx_bytestring.h
+++ b/core/fxcrt/cfx_bytestring.h
@@ -118,6 +118,8 @@
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 4379519..89c8ac6 100644
--- a/core/fxcrt/cfx_bytestring_unittest.cpp
+++ b/core/fxcrt/cfx_bytestring_unittest.cpp
@@ -387,32 +387,68 @@
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(0, empty.Insert(-1, 'X'));
+ EXPECT_NE("X", empty);
+ }
+ {
+ CFX_ByteString empty;
+ EXPECT_EQ(1, empty.Insert(0, 'X'));
EXPECT_EQ("X", empty);
}
{
CFX_ByteString empty;
- empty.Insert(0, '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;
- empty.Insert(5, 'X');
- EXPECT_EQ("X", 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 b837523..6c079b3 100644
--- a/core/fxcrt/cfx_widestring.cpp
+++ b/core/fxcrt/cfx_widestring.cpp
@@ -680,18 +680,18 @@
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++;
+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;
- 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;
+ 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 02045c5..ccb1e75 100644
--- a/core/fxcrt/cfx_widestring.h
+++ b/core/fxcrt/cfx_widestring.h
@@ -120,6 +120,8 @@
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 b743a17..a763f8a 100644
--- a/core/fxcrt/cfx_widestring_unittest.cpp
+++ b/core/fxcrt/cfx_widestring_unittest.cpp
@@ -347,38 +347,68 @@
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(0, empty.Insert(-1, 'X'));
+ EXPECT_NE(L"X", empty);
+ }
+ {
+ CFX_WideString empty;
+ EXPECT_EQ(1, empty.Insert(0, 'X'));
EXPECT_EQ(L"X", empty);
}
{
CFX_WideString empty;
- empty.Insert(0, '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;
- empty.Insert(5, 'X');
- EXPECT_EQ(L"X", 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, 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 3c75406..6a6ed3e 100644
--- a/fxbarcode/BC_Utils.cpp
+++ b/fxbarcode/BC_Utils.cpp
@@ -19,7 +19,7 @@
}
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 d940247..b20fa08 100644
--- a/xfa/fgas/crt/cfgas_formatstring.cpp
+++ b/xfa/fgas/crt/cfgas_formatstring.cpp
@@ -1208,7 +1208,7 @@
if (!FXSYS_isDecimalDigit(str[cc]))
return false;
- wsValue->Insert(0, str[cc]);
+ wsValue->InsertAtFront(str[cc]);
cc--;
ccf--;
break;
@@ -1216,7 +1216,7 @@
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 @@
*wsValue = decimal;
}
if (bNeg)
- wsValue->Insert(0, L'-');
+ wsValue->InsertAtFront(L'-');
return true;
}
@@ -1878,7 +1878,7 @@
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 @@
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 @@
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 @@
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 @@
pLocale->GetNumbericSymbol(FX_LOCALENUMSYMBOL_Minus) + *wsOutput;
bAddNeg = true;
} else {
- wsOutput->Insert(0, L' ');
+ wsOutput->InsertAtFront(L' ');
}
ccf--;
break;
@@ -2070,12 +2070,12 @@
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 @@
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 34e3aea..d68a907 100644
--- a/xfa/fxfa/parser/cxfa_widgetdata.cpp
+++ b/xfa/fxfa/parser/cxfa_widgetdata.cpp
@@ -1750,7 +1750,7 @@
wsOutput.TrimRight(L".");
}
if (wsOutput.IsEmpty() || wsOutput[0] == '.')
- wsOutput.Insert(0, '0');
+ wsOutput.InsertAtFront('0');
}
void CXFA_WidgetData::FormatNumStr(const CFX_WideString& wsValue,
