Add help IsValid* methods to string classeschromium/3198chromium/3197

The various string classes, CFX_ByteString, CFX_ByteStringC,
CFX_WideString, and CFX_WideStringC, have many conditionals that are
effectively determining if a value is a valid index or length. This CL
refactors the logic into one place per class, so it only needs to be
changed once if its behaviour needs to change. It also make the some
of the methods stricter on the inputs they will accept.

BUG=pdfium:828

Change-Id: Iadcdaa34a6d862a2804485770027179c89dc6956
Reviewed-on: https://pdfium-review.googlesource.com/12030
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

7 files changed, 179 insertions, 192 deletions

diff --git a/core/fxcrt/cfx_bytestring.cpp b/core/fxcrt/cfx_bytestring.cpp
index f6074e961b..ac390949ee 100644
--- a/core/fxcrt/cfx_bytestring.cpp
+++ b/core/fxcrt/cfx_bytestring.cpp
@@ -456,24 +456,39 @@ void CFX_ByteString::Concat(const char* pSrcData, FX_STRSIZE nSrcLen) {
   m_pData.Swap(pNewData);
 }
 
-CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE nFirst, FX_STRSIZE nCount) const {
-  ASSERT(nCount >= 0);
+CFX_ByteString CFX_ByteString::Mid(FX_STRSIZE first, FX_STRSIZE count) const {
   if (!m_pData)
     return CFX_ByteString();
 
-  nFirst = pdfium::clamp(nFirst, 0, m_pData->m_nDataLength);
-  nCount = pdfium::clamp(nCount, 0, m_pData->m_nDataLength - nFirst);
-  if (nCount == 0)
+  if (!IsValidIndex(first))
+    return CFX_ByteString();
+
+  if (count == 0 || !IsValidLength(count))
+    return CFX_ByteString();
+
+  if (!IsValidIndex(first + count - 1))
     return CFX_ByteString();
 
-  if (nFirst == 0 && nCount == m_pData->m_nDataLength)
+  if (first == 0 && count == m_pData->m_nDataLength)
     return *this;
 
   CFX_ByteString dest;
-  AllocCopy(dest, nCount, nFirst);
+  AllocCopy(dest, count, first);
   return dest;
 }
 
+CFX_ByteString CFX_ByteString::Left(FX_STRSIZE count) const {
+  if (count == 0 || !IsValidLength(count))
+    return CFX_ByteString();
+  return Mid(0, count);
+}
+
+CFX_ByteString CFX_ByteString::Right(FX_STRSIZE count) const {
+  if (count == 0 || !IsValidLength(count))
+    return CFX_ByteString();
+  return Mid(GetLength() - count, count);
+}
+
 void CFX_ByteString::AllocCopy(CFX_ByteString& dest,
                                FX_STRSIZE nCopyLen,
                                FX_STRSIZE nCopyIndex) const {
@@ -519,78 +534,51 @@ void CFX_ByteString::Format(const char* pFormat, ...) {
 }
 
 void CFX_ByteString::SetAt(FX_STRSIZE index, char c) {
-  ASSERT(index >= 0 && index < GetLength());
+  ASSERT(IsValidIndex(index));
   ReallocBeforeWrite(m_pData->m_nDataLength);
   m_pData->m_String[index] = c;
 }
 
-FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE index, char ch) {
+FX_STRSIZE CFX_ByteString::Insert(FX_STRSIZE location, char ch) {
   const FX_STRSIZE cur_length = m_pData ? m_pData->m_nDataLength : 0;
-  if (index != pdfium::clamp(index, 0, cur_length))
+  if (!IsValidLength(location))
     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;
+  memmove(m_pData->m_String + location + 1, m_pData->m_String + location,
+          new_length - location);
+  m_pData->m_String[location] = ch;
   m_pData->m_nDataLength = new_length;
   return new_length;
 }
 
-CFX_ByteString CFX_ByteString::Right(FX_STRSIZE nCount) const {
-  if (!m_pData)
-    return CFX_ByteString();
-
-  nCount = std::max(nCount, 0);
-  if (nCount >= m_pData->m_nDataLength)
-    return *this;
-
-  CFX_ByteString dest;
-  AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount);
-  return dest;
-}
-
-CFX_ByteString CFX_ByteString::Left(FX_STRSIZE nCount) const {
-  if (!m_pData)
-    return CFX_ByteString();
-
-  nCount = std::max(nCount, 0);
-  if (nCount >= m_pData->m_nDataLength)
-    return *this;
-
-  CFX_ByteString dest;
-  AllocCopy(dest, nCount, 0);
-  return dest;
-}
-
 pdfium::Optional<FX_STRSIZE> CFX_ByteString::Find(char ch,
-                                                  FX_STRSIZE nStart) const {
+                                                  FX_STRSIZE start) const {
   if (!m_pData)
     return pdfium::Optional<FX_STRSIZE>();
 
-  if (nStart < 0 || nStart >= m_pData->m_nDataLength)
+  if (!IsValidIndex(start))
     return pdfium::Optional<FX_STRSIZE>();
 
   const char* pStr = static_cast<const char*>(
-      memchr(m_pData->m_String + nStart, ch, m_pData->m_nDataLength - nStart));
+      memchr(m_pData->m_String + start, ch, m_pData->m_nDataLength - start));
   return pStr ? pdfium::Optional<FX_STRSIZE>(
                     static_cast<FX_STRSIZE>(pStr - m_pData->m_String))
               : pdfium::Optional<FX_STRSIZE>();
 }
 
-pdfium::Optional<FX_STRSIZE> CFX_ByteString::Find(const CFX_ByteStringC& pSub,
-                                                  FX_STRSIZE nStart) const {
+pdfium::Optional<FX_STRSIZE> CFX_ByteString::Find(const CFX_ByteStringC& subStr,
+                                                  FX_STRSIZE start) const {
   if (!m_pData)
     return pdfium::Optional<FX_STRSIZE>();
 
-  FX_STRSIZE nLength = m_pData->m_nDataLength;
-  if (nStart > nLength)
+  if (!IsValidIndex(start))
     return pdfium::Optional<FX_STRSIZE>();
 
   const char* pStr =
-      FX_strstr(m_pData->m_String + nStart, m_pData->m_nDataLength - nStart,
-                pSub.unterminated_c_str(), pSub.GetLength());
+      FX_strstr(m_pData->m_String + start, m_pData->m_nDataLength - start,
+                subStr.unterminated_c_str(), subStr.GetLength());
   return pStr ? pdfium::Optional<FX_STRSIZE>(
                     static_cast<FX_STRSIZE>(pStr - m_pData->m_String))
               : pdfium::Optional<FX_STRSIZE>();
@@ -653,7 +641,7 @@ FX_STRSIZE CFX_ByteString::Remove(char chRemove) {
   }
 
   *pstrDest = 0;
-  FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest);
+  FX_STRSIZE nCount = static_cast<FX_STRSIZE>(pstrSource - pstrDest);
   m_pData->m_nDataLength -= nCount;
   return nCount;
 }
@@ -669,7 +657,7 @@ FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld,
   const char* pStart = m_pData->m_String;
   char* pEnd = m_pData->m_String + m_pData->m_nDataLength;
   while (1) {
-    const char* pTarget = FX_strstr(pStart, (FX_STRSIZE)(pEnd - pStart),
+    const char* pTarget = FX_strstr(pStart, static_cast<int>(pEnd - pStart),
                                     pOld.unterminated_c_str(), nSourceLen);
     if (!pTarget)
       break;
@@ -692,7 +680,7 @@ FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld,
   pStart = m_pData->m_String;
   char* pDest = pNewData->m_String;
   for (FX_STRSIZE i = 0; i < nCount; i++) {
-    const char* pTarget = FX_strstr(pStart, (FX_STRSIZE)(pEnd - pStart),
+    const char* pTarget = FX_strstr(pStart, static_cast<int>(pEnd - pStart),
                                     pOld.unterminated_c_str(), nSourceLen);
     memcpy(pDest, pStart, pTarget - pStart);
     pDest += pTarget - pStart;
@@ -708,7 +696,7 @@ FX_STRSIZE CFX_ByteString::Replace(const CFX_ByteStringC& pOld,
 CFX_WideString CFX_ByteString::UTF8Decode() const {
   CFX_UTF8Decoder decoder;
   for (FX_STRSIZE i = 0; i < GetLength(); i++) {
-    decoder.Input((uint8_t)m_pData->m_String[i]);
+    decoder.Input(static_cast<uint8_t>(m_pData->m_String[i]));
   }
   return CFX_WideString(decoder.GetResult());
 }
@@ -722,14 +710,14 @@ int CFX_ByteString::Compare(const CFX_ByteStringC& str) const {
   if (!m_pData) {
     return str.IsEmpty() ? 0 : -1;
   }
-  int this_len = m_pData->m_nDataLength;
-  int that_len = str.GetLength();
-  int min_len = this_len < that_len ? this_len : that_len;
-  for (int i = 0; i < min_len; i++) {
-    if ((uint8_t)m_pData->m_String[i] < str[i]) {
+  FX_STRSIZE this_len = m_pData->m_nDataLength;
+  FX_STRSIZE that_len = str.GetLength();
+  FX_STRSIZE min_len = std::min(this_len, that_len);
+  for (FX_STRSIZE i = 0; i < min_len; i++) {
+    if (static_cast<uint8_t>(m_pData->m_String[i]) < str[i]) {
       return -1;
     }
-    if ((uint8_t)m_pData->m_String[i] > str[i]) {
+    if (static_cast<uint8_t>(m_pData->m_String[i]) > str[i]) {
       return 1;
     }
   }
@@ -836,7 +824,7 @@ FX_STRSIZE FX_ftoa(float d, char* buf) {
     return 1;
   }
   char buf2[32];
-  int buf_size = 0;
+  FX_STRSIZE buf_size = 0;
   if (bNegative) {
     buf[buf_size++] = '-';
   }
diff --git a/core/fxcrt/cfx_bytestring.h b/core/fxcrt/cfx_bytestring.h
index 10675be7a5..a8a28169a5 100644
--- a/core/fxcrt/cfx_bytestring.h
+++ b/core/fxcrt/cfx_bytestring.h
@@ -84,6 +84,14 @@ class CFX_ByteString {
   }
   bool IsEmpty() const { return !GetLength(); }
 
+  bool IsValidIndex(FX_STRSIZE index) const {
+    return 0 <= index && index < GetLength();
+  }
+
+  bool IsValidLength(FX_STRSIZE length) const {
+    return 0 <= length && length <= GetLength();
+  }
+
   int Compare(const CFX_ByteStringC& str) const;
   bool EqualNoCase(const CFX_ByteStringC& str) const;
 
@@ -109,7 +117,7 @@ class CFX_ByteString {
   const CFX_ByteString& operator+=(const CFX_ByteStringC& bstrc);
 
   CharType operator[](const FX_STRSIZE index) const {
-    ASSERT(index >= 0 && index < GetLength());
+    ASSERT(IsValidIndex(index));
     return m_pData ? m_pData->m_String[index] : 0;
   }
 
diff --git a/core/fxcrt/cfx_bytestring_unittest.cpp b/core/fxcrt/cfx_bytestring_unittest.cpp
index e7281546fb..497e0b6c2f 100644
--- a/core/fxcrt/cfx_bytestring_unittest.cpp
+++ b/core/fxcrt/cfx_bytestring_unittest.cpp
@@ -291,7 +291,7 @@ TEST(fxcrt, ByteStringOperatorNE) {
 TEST(fxcrt, ByteStringCNull) {
   CFX_ByteStringC null_string;
   EXPECT_FALSE(null_string.raw_str());
-  EXPECT_EQ(null_string.GetLength(), 0);
+  EXPECT_EQ(0, null_string.GetLength());
   EXPECT_TRUE(null_string.IsEmpty());
 
   CFX_ByteStringC another_null_string;
@@ -299,27 +299,27 @@ TEST(fxcrt, ByteStringCNull) {
 
   CFX_ByteStringC copied_null_string(null_string);
   EXPECT_FALSE(copied_null_string.raw_str());
-  EXPECT_EQ(copied_null_string.GetLength(), 0);
+  EXPECT_EQ(0, copied_null_string.GetLength());
   EXPECT_TRUE(copied_null_string.IsEmpty());
   EXPECT_EQ(null_string, copied_null_string);
 
   CFX_ByteStringC empty_string("");  // Pointer to NUL, not NULL pointer.
   EXPECT_TRUE(empty_string.raw_str());
-  EXPECT_EQ(empty_string.GetLength(), 0);
+  EXPECT_EQ(0, empty_string.GetLength());
   EXPECT_TRUE(empty_string.IsEmpty());
   EXPECT_EQ(null_string, empty_string);
 
   CFX_ByteStringC assigned_null_string("initially not nullptr");
   assigned_null_string = null_string;
   EXPECT_FALSE(assigned_null_string.raw_str());
-  EXPECT_EQ(assigned_null_string.GetLength(), 0);
+  EXPECT_EQ(0, assigned_null_string.GetLength());
   EXPECT_TRUE(assigned_null_string.IsEmpty());
   EXPECT_EQ(null_string, assigned_null_string);
 
   CFX_ByteStringC assigned_nullptr_string("initially not nullptr");
   assigned_nullptr_string = nullptr;
   EXPECT_FALSE(assigned_nullptr_string.raw_str());
-  EXPECT_EQ(assigned_nullptr_string.GetLength(), 0);
+  EXPECT_EQ(0, assigned_nullptr_string.GetLength());
   EXPECT_TRUE(assigned_nullptr_string.IsEmpty());
   EXPECT_EQ(null_string, assigned_nullptr_string);
 
@@ -345,13 +345,6 @@ TEST(fxcrt, ByteStringConcat) {
   fred.Concat("DY", 2);
   EXPECT_EQ("FREDDY", fred);
   EXPECT_EQ("FRED", copy);
-
-  // Test invalid arguments.
-  copy = fred;
-  fred.Concat("freddy", -6);
-  CFX_ByteString not_aliased("xxxxxx");
-  EXPECT_EQ("FREDDY", fred);
-  EXPECT_EQ("xxxxxx", not_aliased);
 }
 
 TEST(fxcrt, ByteStringRemove) {
@@ -524,10 +517,10 @@ TEST(fxcrt, ByteStringMid) {
   EXPECT_EQ("D", fred.Mid(3, 1));
   EXPECT_EQ("FR", fred.Mid(0, 2));
   EXPECT_EQ("FRED", fred.Mid(0, 4));
-  EXPECT_EQ("FRED", fred.Mid(0, 10));
+  EXPECT_EQ("", fred.Mid(0, 10));
 
-  EXPECT_EQ("FR", fred.Mid(-1, 2));
-  EXPECT_EQ("RED", fred.Mid(1, 4));
+  EXPECT_EQ("", fred.Mid(-1, 2));
+  EXPECT_EQ("RED", fred.Mid(1, 3));
   EXPECT_EQ("", fred.Mid(4, 1));
 
   CFX_ByteString empty;
@@ -542,7 +535,7 @@ TEST(fxcrt, ByteStringLeft) {
   EXPECT_EQ("FRE", fred.Left(3));
   EXPECT_EQ("FRED", fred.Left(4));
 
-  EXPECT_EQ("FRED", fred.Left(5));
+  EXPECT_EQ("", fred.Left(5));
   EXPECT_EQ("", fred.Left(-1));
 
   CFX_ByteString empty;
@@ -559,7 +552,7 @@ TEST(fxcrt, ByteStringRight) {
   EXPECT_EQ("RED", fred.Right(3));
   EXPECT_EQ("FRED", fred.Right(4));
 
-  EXPECT_EQ("FRED", fred.Right(5));
+  EXPECT_EQ("", fred.Right(5));
   EXPECT_EQ("", fred.Right(-1));
 
   CFX_ByteString empty;
@@ -1004,31 +997,31 @@ TEST(fxcrt, ByteStringCMid) {
   EXPECT_EQ(null_string, null_string.Mid(1, 1));
 
   CFX_ByteStringC empty_string("");
-  EXPECT_EQ(empty_string, empty_string.Mid(0, 1));
-  EXPECT_EQ(empty_string, empty_string.Mid(1, 1));
+  EXPECT_EQ("", empty_string.Mid(0, 1));
+  EXPECT_EQ("", empty_string.Mid(1, 1));
 
   CFX_ByteStringC single_character("a");
-  EXPECT_EQ(empty_string, single_character.Mid(0, 0));
+  EXPECT_EQ("", single_character.Mid(0, 0));
   EXPECT_EQ(single_character, single_character.Mid(0, 1));
-  EXPECT_EQ(empty_string, single_character.Mid(1, 0));
-  EXPECT_EQ(empty_string, single_character.Mid(1, 1));
+  EXPECT_EQ("", single_character.Mid(1, 0));
+  EXPECT_EQ("", single_character.Mid(1, 1));
 
   CFX_ByteStringC longer_string("abcdef");
   EXPECT_EQ(longer_string, longer_string.Mid(0, 6));
-  EXPECT_EQ(longer_string, longer_string.Mid(0, 187));
-  EXPECT_EQ(longer_string, longer_string.Mid(-42, 6));
-  EXPECT_EQ(longer_string, longer_string.Mid(-42, 187));
+  EXPECT_EQ("", longer_string.Mid(0, 187));
+  EXPECT_EQ("", longer_string.Mid(-42, 6));
+  EXPECT_EQ("", longer_string.Mid(-42, 187));
 
   CFX_ByteStringC leading_substring("ab");
   EXPECT_EQ(leading_substring, longer_string.Mid(0, 2));
-  EXPECT_EQ(leading_substring, longer_string.Mid(-1, 2));
+  EXPECT_EQ("", longer_string.Mid(-1, 2));
 
   CFX_ByteStringC middle_substring("bcde");
   EXPECT_EQ(middle_substring, longer_string.Mid(1, 4));
 
   CFX_ByteStringC trailing_substring("ef");
   EXPECT_EQ(trailing_substring, longer_string.Mid(4, 2));
-  EXPECT_EQ(trailing_substring, longer_string.Mid(4, 3));
+  EXPECT_EQ("", longer_string.Mid(4, 3));
 }
 
 TEST(fxcrt, ByteStringCElementAccess) {
diff --git a/core/fxcrt/cfx_string_c_template.h b/core/fxcrt/cfx_string_c_template.h
index db8b274410..c46b0c4671 100644
--- a/core/fxcrt/cfx_string_c_template.h
+++ b/core/fxcrt/cfx_string_c_template.h
@@ -119,15 +119,24 @@ class CFX_StringCTemplate {
   }
 
   FX_STRSIZE GetLength() const { return m_Length; }
+
   bool IsEmpty() const { return m_Length == 0; }
 
+  bool IsValidIndex(FX_STRSIZE index) const {
+    return 0 <= index && index < GetLength();
+  }
+
+  bool IsValidLength(FX_STRSIZE length) const {
+    return 0 <= length && length <= GetLength();
+  }
+
   const UnsignedType& operator[](const FX_STRSIZE index) const {
-    ASSERT(index >= 0 && index < GetLength());
+    ASSERT(IsValidIndex(index));
     return m_Ptr.Get()[index];
   }
 
   const CharType CharAt(const FX_STRSIZE index) const {
-    ASSERT(index >= 0 && index < GetLength());
+    ASSERT(IsValidIndex(index));
     return static_cast<CharType>(m_Ptr.Get()[index]);
   }
 
@@ -141,33 +150,32 @@ class CFX_StringCTemplate {
 
   bool Contains(CharType ch) const { return Find(ch).has_value(); }
 
-  CFX_StringCTemplate Mid(FX_STRSIZE index, FX_STRSIZE count) const {
-    ASSERT(count >= 0);
-    if (index > m_Length)
+  CFX_StringCTemplate Mid(FX_STRSIZE first, FX_STRSIZE count) const {
+    if (!m_Ptr.Get())
       return CFX_StringCTemplate();
 
-    index = pdfium::clamp(index, 0, m_Length);
-    count = pdfium::clamp(count, 0, m_Length - index);
-    if (count == 0)
+    if (!IsValidIndex(first))
       return CFX_StringCTemplate();
 
-    return CFX_StringCTemplate(m_Ptr.Get() + index, count);
+    if (count == 0 || !IsValidLength(count))
+      return CFX_StringCTemplate();
+
+    if (!IsValidIndex(first + count - 1))
+      return CFX_StringCTemplate();
+
+    return CFX_StringCTemplate(m_Ptr.Get() + first, count);
   }
 
   CFX_StringCTemplate Left(FX_STRSIZE count) const {
-    count = pdfium::clamp(count, 0, m_Length);
-    if (count == 0)
+    if (count == 0 || !IsValidLength(count))
       return CFX_StringCTemplate();
-
-    return CFX_StringCTemplate(m_Ptr.Get(), count);
+    return Mid(0, count);
   }
 
   CFX_StringCTemplate Right(FX_STRSIZE count) const {
-    count = pdfium::clamp(count, 0, m_Length);
-    if (count == 0)
+    if (count == 0 || !IsValidLength(count))
       return CFX_StringCTemplate();
-
-    return CFX_StringCTemplate(m_Ptr.Get() + m_Length - count, count);
+    return Mid(GetLength() - count, count);
   }
 
   bool operator<(const CFX_StringCTemplate& that) const {
diff --git a/core/fxcrt/cfx_widestring.cpp b/core/fxcrt/cfx_widestring.cpp
index 76fdf24b2c..6e4e82d09d 100644
--- a/core/fxcrt/cfx_widestring.cpp
+++ b/core/fxcrt/cfx_widestring.cpp
@@ -459,7 +459,7 @@ void CFX_WideString::ReallocBeforeWrite(FX_STRSIZE nNewLength) {
   if (m_pData && m_pData->CanOperateInPlace(nNewLength))
     return;
 
-  if (nNewLength <= 0) {
+  if (nNewLength == 0) {
     clear();
     return;
   }
@@ -552,7 +552,7 @@ FX_STRSIZE CFX_WideString::Delete(FX_STRSIZE index, FX_STRSIZE count) {
     return old_length;
 
   ReallocBeforeWrite(old_length);
-  int chars_to_copy = old_length - removal_length + 1;
+  FX_STRSIZE chars_to_copy = old_length - removal_length + 1;
   wmemmove(m_pData->m_String + index, m_pData->m_String + removal_length,
            chars_to_copy);
   m_pData->m_nDataLength = old_length - count;
@@ -602,24 +602,39 @@ CFX_ByteString CFX_WideString::UTF16LE_Encode() const {
   return result;
 }
 
-CFX_WideString CFX_WideString::Mid(FX_STRSIZE nFirst, FX_STRSIZE nCount) const {
-  ASSERT(nCount >= 0);
+CFX_WideString CFX_WideString::Mid(FX_STRSIZE first, FX_STRSIZE count) const {
   if (!m_pData)
     return CFX_WideString();
 
-  nFirst = pdfium::clamp(nFirst, 0, m_pData->m_nDataLength);
-  nCount = pdfium::clamp(nCount, 0, m_pData->m_nDataLength - nFirst);
-  if (nCount == 0)
+  if (!IsValidIndex(first))
+    return CFX_WideString();
+
+  if (count == 0 || !IsValidLength(count))
     return CFX_WideString();
 
-  if (nFirst == 0 && nCount == m_pData->m_nDataLength)
+  if (!IsValidIndex(first + count - 1))
+    return CFX_WideString();
+
+  if (first == 0 && count == GetLength())
     return *this;
 
   CFX_WideString dest;
-  AllocCopy(dest, nCount, nFirst);
+  AllocCopy(dest, count, first);
   return dest;
 }
 
+CFX_WideString CFX_WideString::Left(FX_STRSIZE count) const {
+  if (count == 0 || !IsValidLength(count))
+    return CFX_WideString();
+  return Mid(0, count);
+}
+
+CFX_WideString CFX_WideString::Right(FX_STRSIZE count) const {
+  if (count == 0 || !IsValidLength(count))
+    return CFX_WideString();
+  return Mid(GetLength() - count, count);
+}
+
 void CFX_WideString::AllocCopy(CFX_WideString& dest,
                                FX_STRSIZE nCopyLen,
                                FX_STRSIZE nCopyIndex) const {
@@ -651,24 +666,25 @@ bool CFX_WideString::TryVSWPrintf(FX_STRSIZE size,
   return bSufficientBuffer;
 }
 
-void CFX_WideString::FormatV(const wchar_t* pFormat, va_list argList) {
+void CFX_WideString::FormatV(const wchar_t* format, va_list argList) {
   va_list argListCopy;
   FX_VA_COPY(argListCopy, argList);
-  FX_STRSIZE nMaxLen = vswprintf(nullptr, 0, pFormat, argListCopy);
+  int maxLen = vswprintf(nullptr, 0, format, argListCopy);
   va_end(argListCopy);
-  if (nMaxLen <= 0) {
-    auto guess = GuessSizeForVSWPrintf(pFormat, argListCopy);
+  if (maxLen <= 0) {
+    auto guess = GuessSizeForVSWPrintf(format, argListCopy);
     if (!guess.has_value())
       return;
-    nMaxLen = guess.value();
+    maxLen = guess.value();
   }
-  while (nMaxLen < 32 * 1024) {
+  while (maxLen < 32 * 1024) {
     FX_VA_COPY(argListCopy, argList);
-    bool bSufficientBuffer = TryVSWPrintf(nMaxLen, pFormat, argListCopy);
+    bool bSufficientBuffer =
+        TryVSWPrintf(static_cast<FX_STRSIZE>(maxLen), format, argListCopy);
     va_end(argListCopy);
     if (bSufficientBuffer)
       break;
-    nMaxLen *= 2;
+    maxLen *= 2;
   }
 }
 
@@ -679,73 +695,46 @@ void CFX_WideString::Format(const wchar_t* pFormat, ...) {
   va_end(argList);
 }
 
-FX_STRSIZE CFX_WideString::Insert(FX_STRSIZE index, wchar_t ch) {
+FX_STRSIZE CFX_WideString::Insert(FX_STRSIZE location, wchar_t ch) {
   const FX_STRSIZE cur_length = m_pData ? m_pData->m_nDataLength : 0;
-  if (index != pdfium::clamp(index, 0, cur_length))
+  if (!IsValidLength(location))
     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;
+  wmemmove(m_pData->m_String + location + 1, m_pData->m_String + location,
+           new_length - location);
+  m_pData->m_String[location] = ch;
   m_pData->m_nDataLength = new_length;
   return new_length;
 }
 
-CFX_WideString CFX_WideString::Right(FX_STRSIZE nCount) const {
-  if (!m_pData)
-    return CFX_WideString();
-
-  nCount = std::max(nCount, 0);
-  if (nCount >= m_pData->m_nDataLength)
-    return *this;
-
-  CFX_WideString dest;
-  AllocCopy(dest, nCount, m_pData->m_nDataLength - nCount);
-  return dest;
-}
-
-CFX_WideString CFX_WideString::Left(FX_STRSIZE nCount) const {
-  if (!m_pData)
-    return CFX_WideString();
-
-  nCount = std::max(nCount, 0);
-  if (nCount >= m_pData->m_nDataLength)
-    return *this;
-
-  CFX_WideString dest;
-  AllocCopy(dest, nCount, 0);
-  return dest;
-}
-
 pdfium::Optional<FX_STRSIZE> CFX_WideString::Find(wchar_t ch,
-                                                  FX_STRSIZE nStart) const {
+                                                  FX_STRSIZE start) const {
   if (!m_pData)
     return pdfium::Optional<FX_STRSIZE>();
 
-  if (nStart < 0 || nStart >= m_pData->m_nDataLength)
+  if (!IsValidIndex(start))
     return pdfium::Optional<FX_STRSIZE>();
 
   const wchar_t* pStr =
-      wmemchr(m_pData->m_String + nStart, ch, m_pData->m_nDataLength - nStart);
+      wmemchr(m_pData->m_String + start, ch, m_pData->m_nDataLength - start);
   return pStr ? pdfium::Optional<FX_STRSIZE>(
                     static_cast<FX_STRSIZE>(pStr - m_pData->m_String))
               : pdfium::Optional<FX_STRSIZE>();
 }
 
-pdfium::Optional<FX_STRSIZE> CFX_WideString::Find(const CFX_WideStringC& pSub,
-                                                  FX_STRSIZE nStart) const {
+pdfium::Optional<FX_STRSIZE> CFX_WideString::Find(const CFX_WideStringC& subStr,
+                                                  FX_STRSIZE start) const {
   if (!m_pData)
     return pdfium::Optional<FX_STRSIZE>();
 
-  FX_STRSIZE nLength = m_pData->m_nDataLength;
-  if (nStart > nLength)
+  if (!IsValidIndex(start))
     return pdfium::Optional<FX_STRSIZE>();
 
   const wchar_t* pStr =
-      FX_wcsstr(m_pData->m_String + nStart, m_pData->m_nDataLength - nStart,
-                pSub.unterminated_c_str(), pSub.GetLength());
+      FX_wcsstr(m_pData->m_String + start, m_pData->m_nDataLength - start,
+                subStr.unterminated_c_str(), subStr.GetLength());
   return pStr ? pdfium::Optional<FX_STRSIZE>(
                     static_cast<FX_STRSIZE>(pStr - m_pData->m_String))
               : pdfium::Optional<FX_STRSIZE>();
@@ -796,9 +785,9 @@ FX_STRSIZE CFX_WideString::Remove(wchar_t chRemove) {
   }
 
   *pstrDest = 0;
-  FX_STRSIZE nCount = (FX_STRSIZE)(pstrSource - pstrDest);
-  m_pData->m_nDataLength -= nCount;
-  return nCount;
+  FX_STRSIZE count = static_cast<FX_STRSIZE>(pstrSource - pstrDest);
+  m_pData->m_nDataLength -= count;
+  return count;
 }
 
 FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld,
@@ -808,7 +797,7 @@ FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld,
 
   FX_STRSIZE nSourceLen = pOld.GetLength();
   FX_STRSIZE nReplacementLen = pNew.GetLength();
-  FX_STRSIZE nCount = 0;
+  FX_STRSIZE count = 0;
   const wchar_t* pStart = m_pData->m_String;
   wchar_t* pEnd = m_pData->m_String + m_pData->m_nDataLength;
   while (1) {
@@ -817,24 +806,24 @@ FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld,
     if (!pTarget)
       break;
 
-    nCount++;
+    count++;
     pStart = pTarget + nSourceLen;
   }
-  if (nCount == 0)
+  if (count == 0)
     return 0;
 
   FX_STRSIZE nNewLength =
-      m_pData->m_nDataLength + (nReplacementLen - nSourceLen) * nCount;
+      m_pData->m_nDataLength + (nReplacementLen - nSourceLen) * count;
 
   if (nNewLength == 0) {
     clear();
-    return nCount;
+    return count;
   }
 
   CFX_RetainPtr<StringData> pNewData(StringData::Create(nNewLength));
   pStart = m_pData->m_String;
   wchar_t* pDest = pNewData->m_String;
-  for (FX_STRSIZE i = 0; i < nCount; i++) {
+  for (FX_STRSIZE i = 0; i < count; i++) {
     const wchar_t* pTarget = FX_wcsstr(pStart, (FX_STRSIZE)(pEnd - pStart),
                                        pOld.unterminated_c_str(), nSourceLen);
     wmemcpy(pDest, pStart, pTarget - pStart);
@@ -845,7 +834,7 @@ FX_STRSIZE CFX_WideString::Replace(const CFX_WideStringC& pOld,
   }
   wmemcpy(pDest, pStart, pEnd - pStart);
   m_pData.Swap(pNewData);
-  return nCount;
+  return count;
 }
 
 // static
@@ -874,13 +863,13 @@ CFX_WideString CFX_WideString::FromUTF8(const CFX_ByteStringC& str) {
 // static
 CFX_WideString CFX_WideString::FromUTF16LE(const unsigned short* wstr,
                                            FX_STRSIZE wlen) {
-  if (!wstr || 0 == wlen) {
+  if (!wstr || wlen <= 0) {
     return CFX_WideString();
   }
 
   CFX_WideString result;
   wchar_t* buf = result.GetBuffer(wlen);
-  for (int i = 0; i < wlen; i++) {
+  for (FX_STRSIZE i = 0; i < wlen; i++) {
     buf[i] = wstr[i];
   }
   result.ReleaseBuffer(wlen);
@@ -888,7 +877,7 @@ CFX_WideString CFX_WideString::FromUTF16LE(const unsigned short* wstr,
 }
 
 void CFX_WideString::SetAt(FX_STRSIZE index, wchar_t c) {
-  ASSERT(index >= 0 && index < GetLength());
+  ASSERT(IsValidIndex(index));
   ReallocBeforeWrite(m_pData->m_nDataLength);
   m_pData->m_String[index] = c;
 }
@@ -909,10 +898,10 @@ int CFX_WideString::Compare(const CFX_WideString& str) const {
   if (!str.m_pData) {
     return 1;
   }
-  int this_len = m_pData->m_nDataLength;
-  int that_len = str.m_pData->m_nDataLength;
-  int min_len = this_len < that_len ? this_len : that_len;
-  for (int i = 0; i < min_len; i++) {
+  FX_STRSIZE this_len = m_pData->m_nDataLength;
+  FX_STRSIZE that_len = str.m_pData->m_nDataLength;
+  FX_STRSIZE min_len = std::min(this_len, that_len);
+  for (FX_STRSIZE i = 0; i < min_len; i++) {
     if (m_pData->m_String[i] < str.m_pData->m_String[i]) {
       return -1;
     }
@@ -1035,7 +1024,7 @@ float FX_wtof(const wchar_t* str, int len) {
       cc++;
     }
   }
-  fraction += (float)integer;
+  fraction += static_cast<float>(integer);
   return bNegative ? -fraction : fraction;
 }
 
diff --git a/core/fxcrt/cfx_widestring.h b/core/fxcrt/cfx_widestring.h
index a6d0eca044..242138853b 100644
--- a/core/fxcrt/cfx_widestring.h
+++ b/core/fxcrt/cfx_widestring.h
@@ -81,6 +81,14 @@ class CFX_WideString {
   }
   bool IsEmpty() const { return !GetLength(); }
 
+  bool IsValidIndex(FX_STRSIZE index) const {
+    return 0 <= index && index < GetLength();
+  }
+
+  bool IsValidLength(FX_STRSIZE length) const {
+    return 0 <= length && length <= GetLength();
+  }
+
   const CFX_WideString& operator=(const wchar_t* str);
   const CFX_WideString& operator=(const CFX_WideString& stringSrc);
   const CFX_WideString& operator=(const CFX_WideStringC& stringSrc);
@@ -103,7 +111,7 @@ class CFX_WideString {
   bool operator<(const CFX_WideString& str) const;
 
   CharType operator[](const FX_STRSIZE index) const {
-    ASSERT(index >= 0 && index < GetLength());
+    ASSERT(IsValidIndex(index));
     return m_pData ? m_pData->m_String[index] : 0;
   }
 
diff --git a/core/fxcrt/cfx_widestring_unittest.cpp b/core/fxcrt/cfx_widestring_unittest.cpp
index e688a5334e..0005cb3754 100644
--- a/core/fxcrt/cfx_widestring_unittest.cpp
+++ b/core/fxcrt/cfx_widestring_unittest.cpp
@@ -304,13 +304,6 @@ TEST(fxcrt, WideStringConcatInPlace) {
   fred.Concat(L"DY", 2);
   EXPECT_EQ(L"FREDDY", fred);
   EXPECT_EQ(L"FRED", copy);
-
-  // Test invalid arguments.
-  copy = fred;
-  fred.Concat(L"freddy", -6);
-  CFX_WideString not_aliased(L"xxxxxx");
-  EXPECT_EQ(L"FREDDY", fred);
-  EXPECT_EQ(L"xxxxxx", not_aliased);
 }
 
 TEST(fxcrt, WideStringRemove) {
@@ -483,10 +476,10 @@ TEST(fxcrt, WideStringMid) {
   EXPECT_EQ(L"D", fred.Mid(3, 1));
   EXPECT_EQ(L"FR", fred.Mid(0, 2));
   EXPECT_EQ(L"FRED", fred.Mid(0, 4));
-  EXPECT_EQ(L"FRED", fred.Mid(0, 10));
+  EXPECT_EQ(L"", fred.Mid(0, 10));
 
-  EXPECT_EQ(L"FR", fred.Mid(-1, 2));
-  EXPECT_EQ(L"RED", fred.Mid(1, 4));
+  EXPECT_EQ(L"", fred.Mid(-1, 2));
+  EXPECT_EQ(L"", fred.Mid(1, 4));
   EXPECT_EQ(L"", fred.Mid(4, 1));
 
   CFX_WideString empty;
@@ -501,7 +494,7 @@ TEST(fxcrt, WideStringLeft) {
   EXPECT_EQ(L"FRE", fred.Left(3));
   EXPECT_EQ(L"FRED", fred.Left(4));
 
-  EXPECT_EQ(L"FRED", fred.Left(5));
+  EXPECT_EQ(L"", fred.Left(5));
   EXPECT_EQ(L"", fred.Left(-1));
 
   CFX_WideString empty;
@@ -518,7 +511,7 @@ TEST(fxcrt, WideStringRight) {
   EXPECT_EQ(L"RED", fred.Right(3));
   EXPECT_EQ(L"FRED", fred.Right(4));
 
-  EXPECT_EQ(L"FRED", fred.Right(5));
+  EXPECT_EQ(L"", fred.Right(5));
   EXPECT_EQ(L"", fred.Right(-1));
 
   CFX_WideString empty;