Reject argument lists that are not comma separated

The FormCalc grammar explicitly calls out that argument lists must
have commas separating the simple expressions that make up the
elements. The current implementation will accept the invalid string
!a!b!c, which is 3 variables; !a, !b, and !c.

BUG=chromium:890407

Change-Id: I3e2da4abce9989e9e9b929ce2da030e0f8dfd371
Reviewed-on: https://pdfium-review.googlesource.com/c/43430
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Commit-Queue: Ryan Harrison <rharrison@chromium.org>

3 files changed, 165 insertions, 47 deletions

diff --git a/xfa/fxfa/fm2js/cxfa_fmparser.cpp b/xfa/fxfa/fm2js/cxfa_fmparser.cpp
index 0857573cdf..8bb029e9b9 100644
--- a/xfa/fxfa/fm2js/cxfa_fmparser.cpp
+++ b/xfa/fxfa/fm2js/cxfa_fmparser.cpp
@@ -702,31 +702,13 @@ std::unique_ptr<CXFA_FMSimpleExpression> CXFA_FMParser::ParsePostExpression(
 
     switch (m_token.m_type) {
       case TOKlparen: {
-        if (!NextToken())
+        std::unique_ptr<std::vector<std::unique_ptr<CXFA_FMSimpleExpression>>>
+            expressions = ParseArgumentList();
+        if (!expressions)
           return nullptr;
 
-        std::vector<std::unique_ptr<CXFA_FMSimpleExpression>> expressions;
-        if (m_token.m_type != TOKrparen) {
-          while (m_token.m_type != TOKrparen) {
-            std::unique_ptr<CXFA_FMSimpleExpression> simple_expr =
-                ParseSimpleExpression();
-            if (!simple_expr)
-              return nullptr;
-
-            expressions.push_back(std::move(simple_expr));
-            if (m_token.m_type == TOKcomma) {
-              if (!NextToken())
-                return nullptr;
-            } else if (m_token.m_type == TOKeof ||
-                       m_token.m_type == TOKreserver) {
-              break;
-            }
-          }
-          if (m_token.m_type != TOKrparen)
-            return nullptr;
-        }
         expr = pdfium::MakeUnique<CXFA_FMCallExpression>(
-            std::move(expr), std::move(expressions), false);
+            std::move(expr), std::move(*expressions), false);
         if (!NextToken())
           return nullptr;
         if (m_token.m_type != TOKlbracket)
@@ -750,34 +732,15 @@ std::unique_ptr<CXFA_FMSimpleExpression> CXFA_FMParser::ParsePostExpression(
         if (!NextToken())
           return nullptr;
         if (m_token.m_type == TOKlparen) {
-          std::unique_ptr<CXFA_FMSimpleExpression> pExpCall;
-          if (!NextToken())
+          std::unique_ptr<std::vector<std::unique_ptr<CXFA_FMSimpleExpression>>>
+              expressions = ParseArgumentList();
+          if (!expressions)
             return nullptr;
 
-          std::vector<std::unique_ptr<CXFA_FMSimpleExpression>> expressions;
-          if (m_token.m_type != TOKrparen) {
-            while (m_token.m_type != TOKrparen) {
-              std::unique_ptr<CXFA_FMSimpleExpression> exp =
-                  ParseSimpleExpression();
-              if (!exp)
-                return nullptr;
-
-              expressions.push_back(std::move(exp));
-              if (m_token.m_type == TOKcomma) {
-                if (!NextToken())
-                  return nullptr;
-              } else if (m_token.m_type == TOKeof ||
-                         m_token.m_type == TOKreserver) {
-                break;
-              }
-            }
-            if (m_token.m_type != TOKrparen)
-              return nullptr;
-          }
-          std::unique_ptr<CXFA_FMSimpleExpression> pIdentifier =
+          auto pIdentifier =
               pdfium::MakeUnique<CXFA_FMIdentifierExpression>(tempStr);
-          pExpCall = pdfium::MakeUnique<CXFA_FMCallExpression>(
-              std::move(pIdentifier), std::move(expressions), true);
+          auto pExpCall = pdfium::MakeUnique<CXFA_FMCallExpression>(
+              std::move(pIdentifier), std::move(*expressions), true);
           expr = pdfium::MakeUnique<CXFA_FMMethodCallExpression>(
               std::move(expr), std::move(pExpCall));
           if (!NextToken())
@@ -878,6 +841,36 @@ std::unique_ptr<CXFA_FMSimpleExpression> CXFA_FMParser::ParsePostExpression(
   return expr;
 }
 
+// Argument lists are zero or more comma seperated simple expressions found
+// between '(' and ')'
+std::unique_ptr<std::vector<std::unique_ptr<CXFA_FMSimpleExpression>>>
+CXFA_FMParser::ParseArgumentList() {
+  if (m_token.m_type != TOKlparen || !NextToken())
+    return nullptr;
+
+  auto expressions = pdfium::MakeUnique<
+      std::vector<std::unique_ptr<CXFA_FMSimpleExpression>>>();
+  bool first_arg = true;
+  while (m_token.m_type != TOKrparen) {
+    if (first_arg) {
+      first_arg = false;
+    } else {
+      if (m_token.m_type != TOKcomma || !NextToken())
+        return nullptr;
+    }
+
+    std::unique_ptr<CXFA_FMSimpleExpression> exp = ParseSimpleExpression();
+    if (!exp)
+      return nullptr;
+
+    expressions->push_back(std::move(exp));
+    if (expressions->size() > kMaxPostExpressions)
+      return nullptr;
+  }
+
+  return expressions;
+}
+
 // Index := '[' ('*' | '+' SimpleExpression | '-' SimpleExpression) ']'
 std::unique_ptr<CXFA_FMSimpleExpression> CXFA_FMParser::ParseIndexExpression() {
   AutoRestorer<unsigned long> restorer(&m_parse_depth);
diff --git a/xfa/fxfa/fm2js/cxfa_fmparser.h b/xfa/fxfa/fm2js/cxfa_fmparser.h
index 30fd60fd71..bd66aa70e8 100644
--- a/xfa/fxfa/fm2js/cxfa_fmparser.h
+++ b/xfa/fxfa/fm2js/cxfa_fmparser.h
@@ -52,6 +52,8 @@ class CXFA_FMParser {
   std::unique_ptr<CXFA_FMSimpleExpression> ParsePrimaryExpression();
   std::unique_ptr<CXFA_FMSimpleExpression> ParsePostExpression(
       std::unique_ptr<CXFA_FMSimpleExpression> e);
+  std::unique_ptr<std::vector<std::unique_ptr<CXFA_FMSimpleExpression>>>
+  ParseArgumentList();
   std::unique_ptr<CXFA_FMSimpleExpression> ParseIndexExpression();
   std::unique_ptr<CXFA_FMSimpleExpression> ParseLiteral();
 
diff --git a/xfa/fxfa/fm2js/cxfa_fmparser_unittest.cpp b/xfa/fxfa/fm2js/cxfa_fmparser_unittest.cpp
index b6517c1466..e1e001eea3 100644
--- a/xfa/fxfa/fm2js/cxfa_fmparser_unittest.cpp
+++ b/xfa/fxfa/fm2js/cxfa_fmparser_unittest.cpp
@@ -392,3 +392,126 @@ return pfm_rt.get_val(pfm_ret);
   EXPECT_TRUE(ast->ToJavaScript(&buf));
   EXPECT_STREQ(ret, buf.MakeString().c_str());
 }
+
+TEST(CXFA_FMParserTest, ParseFunctionCallNoArguments) {
+  const wchar_t input[] = L"P.x()";
+  const wchar_t ret[] =
+      LR"***((function() {
+let pfm_method_runner = function(obj, cb) {
+  if (pfm_rt.is_ary(obj)) {
+    let pfm_method_return = null;
+    for (var idx = obj.length -1; idx > 1; idx--) {
+      pfm_method_return = cb(obj[idx]);
+    }
+    return pfm_method_return;
+  }
+  return cb(obj);
+};
+var pfm_ret = null;
+pfm_ret = pfm_rt.get_val((function() {
+  return pfm_method_runner(P, function(obj) {
+    return obj.x();
+  });
+}).call(this));
+return pfm_rt.get_val(pfm_ret);
+}).call(this);)***";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  EXPECT_FALSE(parser->HasError());
+  CXFA_FMToJavaScriptDepth::Reset();
+  CFX_WideTextBuf buf;
+  EXPECT_TRUE(ast->ToJavaScript(&buf));
+  EXPECT_STREQ(ret, buf.MakeString().c_str());
+}
+
+TEST(CXFA_FMParserTest, ParseFunctionCallSingleArgument) {
+  const wchar_t input[] = L"P.x(foo)";
+  const wchar_t ret[] =
+      LR"***((function() {
+let pfm_method_runner = function(obj, cb) {
+  if (pfm_rt.is_ary(obj)) {
+    let pfm_method_return = null;
+    for (var idx = obj.length -1; idx > 1; idx--) {
+      pfm_method_return = cb(obj[idx]);
+    }
+    return pfm_method_return;
+  }
+  return cb(obj);
+};
+var pfm_ret = null;
+pfm_ret = pfm_rt.get_val((function() {
+  return pfm_method_runner(P, function(obj) {
+    return obj.x(pfm_rt.get_jsobj(foo));
+  });
+}).call(this));
+return pfm_rt.get_val(pfm_ret);
+}).call(this);)***";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  EXPECT_FALSE(parser->HasError());
+  CXFA_FMToJavaScriptDepth::Reset();
+  CFX_WideTextBuf buf;
+  EXPECT_TRUE(ast->ToJavaScript(&buf));
+  EXPECT_STREQ(ret, buf.MakeString().c_str());
+}
+
+TEST(CXFA_FMParserTest, ParseFunctionCallMultipleArguments) {
+  const wchar_t input[] = L"P.x(foo, bar, baz)";
+  const wchar_t ret[] =
+      LR"***((function() {
+let pfm_method_runner = function(obj, cb) {
+  if (pfm_rt.is_ary(obj)) {
+    let pfm_method_return = null;
+    for (var idx = obj.length -1; idx > 1; idx--) {
+      pfm_method_return = cb(obj[idx]);
+    }
+    return pfm_method_return;
+  }
+  return cb(obj);
+};
+var pfm_ret = null;
+pfm_ret = pfm_rt.get_val((function() {
+  return pfm_method_runner(P, function(obj) {
+    return obj.x(pfm_rt.get_jsobj(foo), pfm_rt.get_val(bar), pfm_rt.get_val(baz));
+  });
+}).call(this));
+return pfm_rt.get_val(pfm_ret);
+}).call(this);)***";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  EXPECT_FALSE(parser->HasError());
+  CXFA_FMToJavaScriptDepth::Reset();
+  CFX_WideTextBuf buf;
+  EXPECT_TRUE(ast->ToJavaScript(&buf));
+  EXPECT_STREQ(ret, buf.MakeString().c_str());
+}
+
+TEST(CXFA_FMParserTest, ParseFunctionCallMissingCommas) {
+  const wchar_t input[] = L"P.x(!foo!bar!baz)";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  ASSERT_TRUE(ast == nullptr);
+  EXPECT_TRUE(parser->HasError());
+}
+
+TEST(CXFA_FMParserTest, ParseFunctionCallTrailingComma) {
+  const wchar_t input[] = L"P.x(foo,bar,baz,)";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  ASSERT_TRUE(ast == nullptr);
+  EXPECT_TRUE(parser->HasError());
+}
+
+TEST(CXFA_FMParserTest, ParseFunctionCallExtraComma) {
+  const wchar_t input[] = L"P.x(foo,bar,,baz)";
+
+  auto parser = pdfium::MakeUnique<CXFA_FMParser>(input);
+  std::unique_ptr<CXFA_FMAST> ast = parser->Parse();
+  ASSERT_TRUE(ast == nullptr);
+  EXPECT_TRUE(parser->HasError());
+}