commit 0d8c826348c17be0b824fa2af9b69aa52e770cf5
author Tom Sepez <tsepez@chromium.org> Wed Jan 05 00:43:58 2022 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Jan 05 00:43:58 2022 +0000
tree bb759802133dbd76757bde379e45710838528110
parent edf56be438e4f36461c689bb6503ccc3ea2daf96

Avoid CHECK() in CFXJSE_FormCalcContext::Stuff()

Strict ordering of clamp() args may not be preserved with a zero-length
string. Then beware of start + count overflows.

-- Improve test coverage.

Bug: chromium:1284171
Change-Id: I4122aa2f77863c0b6c9e0021189fad57b7b000d2
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/88870
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

fxjs/xfa/cfxjse_formcalc_context.cpp

diff --git a/fxjs/xfa/cfxjse_formcalc_context.cpp b/fxjs/xfa/cfxjse_formcalc_context.cpp
index 88d2616..0d2bf43 100644
--- a/fxjs/xfa/cfxjse_formcalc_context.cpp
+++ b/fxjs/xfa/cfxjse_formcalc_context.cpp
@@ -30,6 +30,7 @@
 #include "third_party/abseil-cpp/absl/types/optional.h"
 #include "third_party/base/check.h"
 #include "third_party/base/cxx17_backports.h"
+#include "third_party/base/numerics/safe_conversions.h"
 #include "v8/include/v8-container.h"
 #include "v8/include/v8-function-callback.h"
 #include "v8/include/v8-object.h"
@@ -4297,31 +4298,35 @@
     return;
   }
 
-  ByteString bsSource;
-  ByteString bsInsert;
-  int32_t iLength = 0;
-  int32_t iStart = 0;
-  int32_t iDelete = 0;
   v8::Local<v8::Value> sourceValue = GetSimpleValue(info, 0);
   v8::Local<v8::Value> startValue = GetSimpleValue(info, 1);
   v8::Local<v8::Value> deleteValue = GetSimpleValue(info, 2);
-  if (!fxv8::IsNull(sourceValue) && !fxv8::IsNull(startValue) &&
-      !fxv8::IsNull(deleteValue)) {
-    bsSource = ValueToUTF8String(info.GetIsolate(), sourceValue);
-    iLength = bsSource.GetLength();
+  if (fxv8::IsNull(sourceValue) || fxv8::IsNull(startValue) ||
+      fxv8::IsNull(deleteValue)) {
+    info.GetReturnValue().SetNull();
+    return;
+  }
+
+  int32_t iStart = 1;  // one-based character indexing.
+  int32_t iDelete = 0;
+  ByteString bsSource = ValueToUTF8String(info.GetIsolate(), sourceValue);
+  int32_t iLength = pdfium::base::checked_cast<int32_t>(bsSource.GetLength());
+  if (iLength) {
     iStart = pdfium::clamp(
         static_cast<int32_t>(ValueToFloat(info.GetIsolate(), startValue)), 1,
         iLength);
-    iDelete = std::max(
-        0, static_cast<int32_t>(ValueToFloat(info.GetIsolate(), deleteValue)));
+    iDelete = pdfium::clamp(
+        static_cast<int32_t>(ValueToFloat(info.GetIsolate(), deleteValue)), 0,
+        iLength - iStart + 1);
   }
 
+  ByteString bsInsert;
   if (argc > 3) {
     v8::Local<v8::Value> insertValue = GetSimpleValue(info, 3);
     bsInsert = ValueToUTF8String(info.GetIsolate(), insertValue);
   }
 
-  --iStart;
+  --iStart;  // now zero-based.
   std::ostringstream szResult;
   int32_t i = 0;
   while (i < iStart) {

fxjs/xfa/cfxjse_formcalc_context_embeddertest.cpp

diff --git a/fxjs/xfa/cfxjse_formcalc_context_embeddertest.cpp b/fxjs/xfa/cfxjse_formcalc_context_embeddertest.cpp
index 19ab774..ab550e5 100644
--- a/fxjs/xfa/cfxjse_formcalc_context_embeddertest.cpp
+++ b/fxjs/xfa/cfxjse_formcalc_context_embeddertest.cpp
@@ -825,10 +825,41 @@
 TEST_F(CFXJSE_FormCalcContextEmbedderTest, Stuff) {
   ASSERT_TRUE(OpenDocument("simple_xfa.pdf"));
 
+  // Test wrong number of parameters.
+  ExecuteExpectError("Stuff(1, 2)");
+  ExecuteExpectError("Stuff(1, 2, 3, 4, 5)");
+
+  // Test null arguments.
+  ExecuteExpectNull("Stuff(null, 0, 4)");
+  ExecuteExpectNull("Stuff(\"ABCDEFG\", null, 4)");
+  ExecuteExpectNull("Stuff(\"ABCDEFG\", 0, null)");
+
+  // Insertions.
+  ExecuteExpectString("Stuff(\"\", 0, 0, \"clams\")", "clams");
   ExecuteExpectString("Stuff(\"TonyBlue\", 5, 0, \" \")", "Tony Blue");
+
+  // Deletions.
+  ExecuteExpectString("Stuff(\"A\", 1, 0)", "A");
+  ExecuteExpectString("Stuff(\"A\", 1, 1)", "");
   ExecuteExpectString("Stuff(\"ABCDEFGH\", 4, 2)", "ABCFGH");
-  ExecuteExpectString("Stuff(\"members-list@myweb.com\", 0, 0, \"cc:\")",
-                      "cc:members-list@myweb.com");
+  ExecuteExpectString("Stuff(\"ABCDEFGH\", 7, 2)", "ABCDEF");
+
+  // Test index clamping.
+  ExecuteExpectString("Stuff(\"ABCDEFGH\", -400, 400)", "");
+
+  // Need significant amount of text to test start + count overflow due to
+  // intermediate float representation of count not being able to hold
+  // INT_MAX.
+  ExecuteExpectString(
+      "Stuff(\""
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "\", 133, 2147483520)",
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345678900"
+      "abcd");
 }
 
 TEST_F(CFXJSE_FormCalcContextEmbedderTest, Substr) {