Re-land "Return pdfium::span<char> from ByteString::GetBuffer().""

This reverts commit 3d523e3cf89440e2ffc6571b1c687ad5e3f0318f.
Fixes bounding errors now caught by tests.

Change-Id: I4d0f1791bdcc45a10615a62abf7a4d20e7e538f2
Reviewed-on: https://pdfium-review.googlesource.com/30799
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>

12 files changed, 171 insertions, 129 deletions

diff --git a/core/fpdfapi/page/cpdf_image.cpp b/core/fpdfapi/page/cpdf_image.cpp
index 68a6a3243c..9ce4268a36 100644
--- a/core/fpdfapi/page/cpdf_image.cpp
+++ b/core/fpdfapi/page/cpdf_image.cpp
@@ -202,13 +202,16 @@ void CPDF_Image::SetImage(const RetainPtr<CFX_DIBitmap>& pBitmap) {
       pCS->AddNew<CPDF_Name>("DeviceRGB");
       pCS->AddNew<CPDF_Number>(1);
       ByteString ct;
-      char* pBuf = ct.GetBuffer(6);
-      pBuf[0] = (char)reset_r;
-      pBuf[1] = (char)reset_g;
-      pBuf[2] = (char)reset_b;
-      pBuf[3] = (char)set_r;
-      pBuf[4] = (char)set_g;
-      pBuf[5] = (char)set_b;
+      {
+        // Span's lifetime must end before ReleaseBuffer() below.
+        pdfium::span<char> pBuf = ct.GetBuffer(6);
+        pBuf[0] = (char)reset_r;
+        pBuf[1] = (char)reset_g;
+        pBuf[2] = (char)reset_b;
+        pBuf[3] = (char)set_r;
+        pBuf[4] = (char)set_g;
+        pBuf[5] = (char)set_b;
+      }
       ct.ReleaseBuffer(6);
       pCS->AddNew<CPDF_String>(ct, true);
     }
diff --git a/core/fpdfapi/parser/fpdf_parser_decode.cpp b/core/fpdfapi/parser/fpdf_parser_decode.cpp
index e879615ddd..edfbaf4331 100644
--- a/core/fpdfapi/parser/fpdf_parser_decode.cpp
+++ b/core/fpdfapi/parser/fpdf_parser_decode.cpp
@@ -470,20 +470,22 @@ ByteString PDF_EncodeText(const wchar_t* pString, int len) {
   if (len == -1)
     len = wcslen(pString);
 
-  ByteString result;
-  char* dest_buf1 = result.GetBuffer(len);
   int i;
-  for (i = 0; i < len; ++i) {
-    int code;
-    for (code = 0; code < 256; ++code) {
-      if (PDFDocEncoding[code] == pString[i])
-        break;
-    }
+  ByteString result;
+  {
+    pdfium::span<char> dest_buf = result.GetBuffer(len);
+    for (i = 0; i < len; ++i) {
+      int code;
+      for (code = 0; code < 256; ++code) {
+        if (PDFDocEncoding[code] == pString[i])
+          break;
+      }
 
-    if (code == 256)
-      break;
+      if (code == 256)
+        break;
 
-    dest_buf1[i] = code;
+      dest_buf[i] = code;
+    }
   }
   result.ReleaseBuffer(i);
   if (i == len)
@@ -494,15 +496,18 @@ ByteString PDF_EncodeText(const wchar_t* pString, int len) {
     return result;
   }
 
-  int encLen = len * 2 + 2;
-
-  uint8_t* dest_buf2 = reinterpret_cast<uint8_t*>(result.GetBuffer(encLen));
-  dest_buf2[0] = 0xfe;
-  dest_buf2[1] = 0xff;
-  dest_buf2 += 2;
-  for (int j = 0; j < len; ++j) {
-    *dest_buf2++ = pString[j] >> 8;
-    *dest_buf2++ = static_cast<uint8_t>(pString[j]);
+  size_t dest_index = 0;
+  size_t encLen = len * 2 + 2;
+  {
+    pdfium::span<char> cspan = result.GetBuffer(encLen);
+    auto dest_buf = pdfium::make_span(reinterpret_cast<uint8_t*>(cspan.data()),
+                                      cspan.size());
+    dest_buf[dest_index++] = 0xfe;
+    dest_buf[dest_index++] = 0xff;
+    for (int j = 0; j < len; ++j) {
+      dest_buf[dest_index++] = pString[j] >> 8;
+      dest_buf[dest_index++] = static_cast<uint8_t>(pString[j]);
+    }
   }
   result.ReleaseBuffer(encLen);
   return result;
diff --git a/core/fpdfapi/parser/fpdf_parser_utility.cpp b/core/fpdfapi/parser/fpdf_parser_utility.cpp
index c486a76706..2c0b0818f1 100644
--- a/core/fpdfapi/parser/fpdf_parser_utility.cpp
+++ b/core/fpdfapi/parser/fpdf_parser_utility.cpp
@@ -93,20 +93,23 @@ ByteString PDF_NameDecode(const ByteStringView& bstr) {
   if (!bstr.Contains('#'))
     return ByteString(bstr);
 
-  int size = bstr.GetLength();
+  size_t src_size = bstr.GetLength();
+  size_t out_index = 0;
   ByteString result;
-  char* pDestStart = result.GetBuffer(size);
-  char* pDest = pDestStart;
-  for (int i = 0; i < size; i++) {
-    if (bstr[i] == '#' && i < size - 2) {
-      *pDest++ = FXSYS_HexCharToInt(bstr[i + 1]) * 16 +
-                 FXSYS_HexCharToInt(bstr[i + 2]);
-      i += 2;
-    } else {
-      *pDest++ = bstr[i];
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> pDest = result.GetBuffer(src_size);
+    for (size_t i = 0; i < src_size; i++) {
+      if (bstr[i] == '#' && i + 2 < src_size) {
+        pDest[out_index++] = FXSYS_HexCharToInt(bstr[i + 1]) * 16 +
+                             FXSYS_HexCharToInt(bstr[i + 2]);
+        i += 2;
+      } else {
+        pDest[out_index++] = bstr[i];
+      }
     }
   }
-  result.ReleaseBuffer(static_cast<size_t>(pDest - pDestStart));
+  result.ReleaseBuffer(out_index);
   return result;
 }
 
@@ -128,21 +131,23 @@ ByteString PDF_NameEncode(const ByteString& orig) {
     return orig;
 
   ByteString res;
-  char* dest_buf = res.GetBuffer(dest_len);
-  dest_len = 0;
-  for (i = 0; i < src_len; i++) {
-    uint8_t ch = src_buf[i];
-    if (ch >= 0x80 || PDFCharIsWhitespace(ch) || ch == '#' ||
-        PDFCharIsDelimiter(ch)) {
-      dest_buf[dest_len++] = '#';
-      FXSYS_IntToTwoHexChars(ch, dest_buf + dest_len);
-      dest_len += 2;
-    } else {
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> dest_buf = res.GetBuffer(dest_len);
+    dest_len = 0;
+    for (i = 0; i < src_len; i++) {
+      uint8_t ch = src_buf[i];
+      if (ch >= 0x80 || PDFCharIsWhitespace(ch) || ch == '#' ||
+          PDFCharIsDelimiter(ch)) {
+        dest_buf[dest_len++] = '#';
+        FXSYS_IntToTwoHexChars(ch, &dest_buf[dest_len]);
+        dest_len += 2;
+        continue;
+      }
       dest_buf[dest_len++] = ch;
     }
   }
-  dest_buf[dest_len] = 0;
-  res.ReleaseBuffer(res.GetStringLength());
+  res.ReleaseBuffer(dest_len);
   return res;
 }
 
diff --git a/core/fxcrt/bytestring.cpp b/core/fxcrt/bytestring.cpp
index f5687c591e..872de065ba 100644
--- a/core/fxcrt/bytestring.cpp
+++ b/core/fxcrt/bytestring.cpp
@@ -18,6 +18,7 @@
 #include "core/fxcrt/fx_safe_types.h"
 #include "core/fxcrt/string_pool_template.h"
 #include "third_party/base/numerics/safe_math.h"
+#include "third_party/base/span.h"
 #include "third_party/base/stl_util.h"
 
 template class fxcrt::StringDataTemplate<char>;
@@ -81,9 +82,12 @@ ByteString GetByteString(uint16_t codepage, const WideStringView& wstr) {
     return ByteString();
 
   ByteString bstr;
-  char* dest_buf = bstr.GetBuffer(dest_len);
-  FXSYS_WideCharToMultiByte(codepage, 0, wstr.unterminated_c_str(), src_len,
-                            dest_buf, dest_len, nullptr, nullptr);
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> dest_buf = bstr.GetBuffer(dest_len);
+    FXSYS_WideCharToMultiByte(codepage, 0, wstr.unterminated_c_str(), src_len,
+                              dest_buf.data(), dest_len, nullptr, nullptr);
+  }
   bstr.ReleaseBuffer(dest_len);
   return bstr;
 }
@@ -120,19 +124,21 @@ ByteString ByteString::FormatV(const char* pFormat, va_list argList) {
   va_end(argListCopy);
 
   if (nMaxLen <= 0)
-    return "";
+    return ByteString();
 
   ByteString ret;
-  char* buf = ret.GetBuffer(nMaxLen);
-  if (buf) {
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> buf = ret.GetBuffer(nMaxLen);
+
     // In the following two calls, there's always space in the buffer for
     // a terminating NUL that's not included in nMaxLen.
-    memset(buf, 0, nMaxLen + 1);
+    memset(buf.data(), 0, nMaxLen + 1);
     va_copy(argListCopy, argList);
-    vsnprintf(buf, nMaxLen + 1, pFormat, argListCopy);
+    vsnprintf(buf.data(), nMaxLen + 1, pFormat, argListCopy);
     va_end(argListCopy);
-    ret.ReleaseBuffer(ret.GetStringLength());
   }
+  ret.ReleaseBuffer(ret.GetStringLength());
   return ret;
 }
 
@@ -419,29 +425,29 @@ void ByteString::Reserve(size_t len) {
   GetBuffer(len);
 }
 
-char* ByteString::GetBuffer(size_t nMinBufLength) {
+pdfium::span<char> ByteString::GetBuffer(size_t nMinBufLength) {
   if (!m_pData) {
     if (nMinBufLength == 0)
-      return nullptr;
+      return pdfium::span<char>();
 
     m_pData.Reset(StringData::Create(nMinBufLength));
     m_pData->m_nDataLength = 0;
     m_pData->m_String[0] = 0;
-    return m_pData->m_String;
+    return pdfium::span<char>(m_pData->m_String, m_pData->m_nAllocLength);
   }
 
   if (m_pData->CanOperateInPlace(nMinBufLength))
-    return m_pData->m_String;
+    return pdfium::span<char>(m_pData->m_String, m_pData->m_nAllocLength);
 
   nMinBufLength = std::max(nMinBufLength, m_pData->m_nDataLength);
   if (nMinBufLength == 0)
-    return nullptr;
+    return pdfium::span<char>();
 
   RetainPtr<StringData> pNewData(StringData::Create(nMinBufLength));
   pNewData->CopyContents(*m_pData);
   pNewData->m_nDataLength = m_pData->m_nDataLength;
   m_pData.Swap(pNewData);
-  return m_pData->m_String;
+  return pdfium::span<char>(m_pData->m_String, m_pData->m_nAllocLength);
 }
 
 size_t ByteString::Delete(size_t index, size_t count) {
diff --git a/core/fxcrt/bytestring.h b/core/fxcrt/bytestring.h
index ccb539b2ef..247238573d 100644
--- a/core/fxcrt/bytestring.h
+++ b/core/fxcrt/bytestring.h
@@ -17,6 +17,7 @@
 #include "core/fxcrt/string_data_template.h"
 #include "core/fxcrt/string_view_template.h"
 #include "third_party/base/optional.h"
+#include "third_party/base/span.h"
 
 namespace fxcrt {
 
@@ -153,7 +154,10 @@ class ByteString {
   size_t Delete(size_t index, size_t count = 1);
 
   void Reserve(size_t len);
-  char* GetBuffer(size_t len);
+
+  // Note: any modification of the string (including ReleaseBuffer()) may
+  // invalidate the span, which must not outlive its buffer.
+  pdfium::span<char> GetBuffer(size_t len);
   void ReleaseBuffer(size_t len);
 
   ByteString Mid(size_t first, size_t count) const;
diff --git a/core/fxcrt/bytestring_unittest.cpp b/core/fxcrt/bytestring_unittest.cpp
index f7e1559af9..d030535edd 100644
--- a/core/fxcrt/bytestring_unittest.cpp
+++ b/core/fxcrt/bytestring_unittest.cpp
@@ -843,22 +843,23 @@ TEST(ByteString, Reserve) {
 }
 
 TEST(ByteString, GetBuffer) {
+  ByteString str1;
   {
-    ByteString str;
-    char* buffer = str.GetBuffer(12);
+    pdfium::span<char> buffer = str1.GetBuffer(12);
     // NOLINTNEXTLINE(runtime/printf)
-    strcpy(buffer, "clams");
-    str.ReleaseBuffer(str.GetStringLength());
-    EXPECT_EQ("clams", str);
+    strcpy(buffer.data(), "clams");
   }
+  str1.ReleaseBuffer(str1.GetStringLength());
+  EXPECT_EQ("clams", str1);
+
+  ByteString str2("cl");
   {
-    ByteString str("cl");
-    char* buffer = str.GetBuffer(12);
+    pdfium::span<char> buffer = str2.GetBuffer(12);
     // NOLINTNEXTLINE(runtime/printf)
-    strcpy(buffer + 2, "ams");
-    str.ReleaseBuffer(str.GetStringLength());
-    EXPECT_EQ("clams", str);
+    strcpy(&buffer[2], "ams");
   }
+  str2.ReleaseBuffer(str2.GetStringLength());
+  EXPECT_EQ("clams", str2);
 }
 
 TEST(ByteString, ReleaseBuffer) {
diff --git a/core/fxcrt/widestring.cpp b/core/fxcrt/widestring.cpp
index a3525593ee..25f253ea11 100644
--- a/core/fxcrt/widestring.cpp
+++ b/core/fxcrt/widestring.cpp
@@ -667,18 +667,21 @@ ByteString WideString::UTF8Encode() const {
 }
 
 ByteString WideString::UTF16LE_Encode() const {
-  if (!m_pData) {
+  if (!m_pData)
     return ByteString("\0\0", 2);
-  }
-  int len = m_pData->m_nDataLength;
+
   ByteString result;
-  char* buffer = result.GetBuffer(len * 2 + 2);
-  for (int i = 0; i < len; i++) {
-    buffer[i * 2] = m_pData->m_String[i] & 0xff;
-    buffer[i * 2 + 1] = m_pData->m_String[i] >> 8;
+  int len = m_pData->m_nDataLength;
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> buffer = result.GetBuffer(len * 2 + 2);
+    for (int i = 0; i < len; i++) {
+      buffer[i * 2] = m_pData->m_String[i] & 0xff;
+      buffer[i * 2 + 1] = m_pData->m_String[i] >> 8;
+    }
+    buffer[len * 2] = 0;
+    buffer[len * 2 + 1] = 0;
   }
-  buffer[len * 2] = 0;
-  buffer[len * 2 + 1] = 0;
   result.ReleaseBuffer(len * 2 + 2);
   return result;
 }
diff --git a/core/fxge/cfx_folderfontinfo.cpp b/core/fxge/cfx_folderfontinfo.cpp
index 532824d248..b39c57637f 100644
--- a/core/fxge/cfx_folderfontinfo.cpp
+++ b/core/fxge/cfx_folderfontinfo.cpp
@@ -44,11 +44,15 @@ const struct {
 };
 
 ByteString FPDF_ReadStringFromFile(FILE* pFile, uint32_t size) {
-  ByteString buffer;
-  if (!fread(buffer.GetBuffer(size), size, 1, pFile))
-    return ByteString();
-  buffer.ReleaseBuffer(size);
-  return buffer;
+  ByteString result;
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> buffer = result.GetBuffer(size);
+    if (!fread(buffer.data(), size, 1, pFile))
+      return ByteString();
+  }
+  result.ReleaseBuffer(size);
+  return result;
 }
 
 ByteString FPDF_LoadTableFromTT(FILE* pFile,
diff --git a/core/fxge/win32/fx_win32_dib.cpp b/core/fxge/win32/fx_win32_dib.cpp
index 452d033f64..b6bed7a987 100644
--- a/core/fxge/win32/fx_win32_dib.cpp
+++ b/core/fxge/win32/fx_win32_dib.cpp
@@ -13,39 +13,43 @@
 
 ByteString CFX_WindowsDIB::GetBitmapInfo(
     const RetainPtr<CFX_DIBitmap>& pBitmap) {
-  ByteString result;
   int len = sizeof(BITMAPINFOHEADER);
-  if (pBitmap->GetBPP() == 1 || pBitmap->GetBPP() == 8) {
+  if (pBitmap->GetBPP() == 1 || pBitmap->GetBPP() == 8)
     len += sizeof(DWORD) * (int)(1 << pBitmap->GetBPP());
-  }
-  BITMAPINFOHEADER* pbmih = (BITMAPINFOHEADER*)result.GetBuffer(len);
-  memset(pbmih, 0, sizeof(BITMAPINFOHEADER));
-  pbmih->biSize = sizeof(BITMAPINFOHEADER);
-  pbmih->biBitCount = pBitmap->GetBPP();
-  pbmih->biCompression = BI_RGB;
-  pbmih->biHeight = -(int)pBitmap->GetHeight();
-  pbmih->biPlanes = 1;
-  pbmih->biWidth = pBitmap->GetWidth();
-  if (pBitmap->GetBPP() == 8) {
-    uint32_t* pPalette = (uint32_t*)(pbmih + 1);
-    if (pBitmap->GetPalette()) {
-      for (int i = 0; i < 256; i++) {
-        pPalette[i] = pBitmap->GetPalette()[i];
-      }
-    } else {
-      for (int i = 0; i < 256; i++) {
-        pPalette[i] = i * 0x010101;
+
+  ByteString result;
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> cspan = result.GetBuffer(len);
+    BITMAPINFOHEADER* pbmih = reinterpret_cast<BITMAPINFOHEADER*>(cspan.data());
+    memset(pbmih, 0, sizeof(BITMAPINFOHEADER));
+    pbmih->biSize = sizeof(BITMAPINFOHEADER);
+    pbmih->biBitCount = pBitmap->GetBPP();
+    pbmih->biCompression = BI_RGB;
+    pbmih->biHeight = -(int)pBitmap->GetHeight();
+    pbmih->biPlanes = 1;
+    pbmih->biWidth = pBitmap->GetWidth();
+    if (pBitmap->GetBPP() == 8) {
+      uint32_t* pPalette = (uint32_t*)(pbmih + 1);
+      if (pBitmap->GetPalette()) {
+        for (int i = 0; i < 256; i++) {
+          pPalette[i] = pBitmap->GetPalette()[i];
+        }
+      } else {
+        for (int i = 0; i < 256; i++) {
+          pPalette[i] = i * 0x010101;
+        }
       }
     }
-  }
-  if (pBitmap->GetBPP() == 1) {
-    uint32_t* pPalette = (uint32_t*)(pbmih + 1);
-    if (pBitmap->GetPalette()) {
-      pPalette[0] = pBitmap->GetPalette()[0];
-      pPalette[1] = pBitmap->GetPalette()[1];
-    } else {
-      pPalette[0] = 0;
-      pPalette[1] = 0xffffff;
+    if (pBitmap->GetBPP() == 1) {
+      uint32_t* pPalette = (uint32_t*)(pbmih + 1);
+      if (pBitmap->GetPalette()) {
+        pPalette[0] = pBitmap->GetPalette()[0];
+        pPalette[1] = pBitmap->GetPalette()[1];
+      } else {
+        pPalette[0] = 0;
+        pPalette[1] = 0xffffff;
+      }
     }
   }
   result.ReleaseBuffer(len);
diff --git a/fpdfsdk/cpdfsdk_formfillenvironment.cpp b/fpdfsdk/cpdfsdk_formfillenvironment.cpp
index c1846449cd..262a56e2d1 100644
--- a/fpdfsdk/cpdfsdk_formfillenvironment.cpp
+++ b/fpdfsdk/cpdfsdk_formfillenvironment.cpp
@@ -25,7 +25,7 @@ FPDF_WIDESTRING AsFPDFWideString(ByteString* bsUTF16LE) {
   // to the embedder. Should the embedder modify it by accident, it won't
   // corrupt other shares of the string beyond |bsUTF16LE|.
   return reinterpret_cast<FPDF_WIDESTRING>(
-      bsUTF16LE->GetBuffer(bsUTF16LE->GetLength()));
+      bsUTF16LE->GetBuffer(bsUTF16LE->GetLength()).data());
 }
 
 CPDFSDK_FormFillEnvironment::CPDFSDK_FormFillEnvironment(
diff --git a/fxjs/cfxjse_formcalc_context.cpp b/fxjs/cfxjse_formcalc_context.cpp
index 85d0ef8d57..a6d151b468 100644
--- a/fxjs/cfxjse_formcalc_context.cpp
+++ b/fxjs/cfxjse_formcalc_context.cpp
@@ -509,12 +509,16 @@ ByteString GUIDString(bool bSeparator) {
   data[6] = (data[6] & 0x0F) | 0x40;
 
   ByteString bsStr;
-  char* pBuf = bsStr.GetBuffer(40);
-  for (int32_t i = 0; i < 16; ++i, pBuf += 2) {
-    if (bSeparator && (i == 4 || i == 6 || i == 8 || i == 10))
-      *pBuf++ = L'-';
-
-    FXSYS_IntToTwoHexChars(data[i], pBuf);
+  {
+    // Span's lifetime must end before ReleaseBuffer() below.
+    pdfium::span<char> pBuf = bsStr.GetBuffer(40);
+    size_t out_index = 0;
+    for (size_t i = 0; i < 16; ++i, out_index += 2) {
+      if (bSeparator && (i == 4 || i == 6 || i == 8 || i == 10))
+        pBuf[out_index++] = L'-';
+
+      FXSYS_IntToTwoHexChars(data[i], &pBuf[out_index]);
+    }
   }
   bsStr.ReleaseBuffer(bSeparator ? 36 : 32);
   return bsStr;
diff --git a/fxjs/cjs_publicmethods.cpp b/fxjs/cjs_publicmethods.cpp
index 375eb6fd88..8dc69c4a00 100644
--- a/fxjs/cjs_publicmethods.cpp
+++ b/fxjs/cjs_publicmethods.cpp
@@ -10,6 +10,7 @@
 #include <cmath>
 #include <cwctype>
 #include <iomanip>
+#include <iterator>
 #include <limits>
 #include <sstream>
 #include <string>
@@ -1094,10 +1095,12 @@ CJS_Return CJS_PublicMethods::AFPercent_Format(
 
   if (iDec2 < 0) {
     ByteString zeros;
-    char* zeros_ptr = zeros.GetBuffer(abs(iDec2));
-    memset(zeros_ptr, '0', abs(iDec2));
+    {
+      pdfium::span<char> zeros_ptr = zeros.GetBuffer(abs(iDec2));
+      std::fill(std::begin(zeros_ptr), std::end(zeros_ptr), '0');
+    }
+    zeros.ReleaseBuffer(abs(iDec2));
     strValue = zeros + strValue;
-
     iDec2 = 0;
   }
   int iMax = strValue.GetLength();