Bug 1496333 - Do not use AssertedParameterScope.paramNames.length for allocation. r=efaust
authorTooru Fujisawa <arai_a@mac.com>
Mon, 15 Oct 2018 23:51:50 +0900
changeset 499789 3293a3393b4a68e942b7f8fd5f2dd77b337b8676
parent 499788 a298e5b921f7dde9e3e36f25fae60a950e94a20b
child 499790 cd6a56ce613caf1e103b4d51503de6bce2cdee26
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersefaust
bugs1496333
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1496333 - Do not use AssertedParameterScope.paramNames.length for allocation. r=efaust
js/src/frontend/BinSource-auto.cpp
js/src/frontend/BinSource.cpp
js/src/frontend/BinSource.yaml
--- a/js/src/frontend/BinSource-auto.cpp
+++ b/js/src/frontend/BinSource-auto.cpp
@@ -2340,22 +2340,33 @@ BinASTParser<Tok>::parseInterfaceAsserte
     const BinField expected_fields[3] = { BinField::Index, BinField::Name, BinField::IsCaptured };
     MOZ_TRY(tokenizer_->checkFields(kind, fields, expected_fields));
 #endif // defined(DEBUG)
 
     BINJS_MOZ_TRY_DECL(index, tokenizer_->readUnsignedLong());
 
     RootedAtom name(cx_);
     MOZ_TRY_VAR(name, tokenizer_->readIdentifierName());
-    // FIXME: The following checks should be performed inside
-    // checkPositionalParameterIndices to match the spec's order
-    // (bug 1490976).
-    if (index >= positionalParams.get().length()) {
-        return raiseError("AssertedPositionalParameterName.length out of range");
+    // `positionalParams` vector can be shorter than the actual
+    // parameter length. Resize on demand.
+    // (see also ListOfAssertedMaybePositionalParameterName)
+    size_t prevLength = positionalParams.get().length();
+    if (index >= prevLength) {
+        // This is implementation limit, which is not in the spec.
+        size_t newLength = index + 1;
+        if (newLength >= ARGNO_LIMIT) {
+            return raiseError("AssertedPositionalParameterName.index is too big");
+        }
+
+        BINJS_TRY(positionalParams.get().resize(newLength));
+        for (uint32_t i = prevLength; i < newLength; i++) {
+            positionalParams.get()[i] = nullptr;
+        }
     }
+
     if (positionalParams.get()[index]) {
         return raiseError("AssertedPositionalParameterName has duplicate entry for the same index");
     }
     positionalParams.get()[index] = name;
     BINJS_MOZ_TRY_DECL(isCaptured, tokenizer_->readBool());
     ParseContext::Scope* scope;
     DeclarationKind declKind;
     MOZ_TRY(getBoundScope(scopeKind, scope, declKind));
@@ -5476,23 +5487,24 @@ BinASTParser<Tok>::parseListOfAssertedMa
 {
     uint32_t length;
     AutoList guard(*tokenizer_);
 
     const auto start = tokenizer_->offset();
     MOZ_TRY(tokenizer_->enterList(length, guard));
     (void) start;
     auto result = Ok();
-    if (length >= ARGNO_LIMIT) {
-        return raiseError("Too many function parameters");
-    }
-    BINJS_TRY(positionalParams.get().resize(length));
-    for (uint32_t i = 0; i < length; i++) {
-        positionalParams.get()[i] = nullptr;
-    }
+    // This list contains also destructuring parameters, and the number of
+    // list items can be greater than the actual parameters, or more than
+    // ARGNO_LIMIT even if the number of parameters fits into ARGNO_LIMIT.
+    // Also, the number of parameters can be greater than this list's length
+    // if one of destructuring parameter is empty.
+    //
+    // We resize `positionalParams` vector on demand, to keep the vector
+    // length match to the known maximum positional parameter index + 1.
 
     for (uint32_t i = 0; i < length; ++i) {
         MOZ_TRY(parseAssertedMaybePositionalParameterName(
             scopeKind, positionalParams));
         // Nothing to do here.
     }
 
     MOZ_TRY(guard.done());
--- a/js/src/frontend/BinSource.cpp
+++ b/js/src/frontend/BinSource.cpp
@@ -473,62 +473,85 @@ BinASTParser<Tok>::checkBinding(JSAtom* 
 }
 
 // Binary AST (revision 8eab67e0c434929a66ff6abe99ff790bca087dda)
 // 3.1.5 CheckPositionalParameterIndices.
 template<typename Tok> JS::Result<Ok>
 BinASTParser<Tok>::checkPositionalParameterIndices(Handle<GCVector<JSAtom*>> positionalParams,
                                                    ListNode* params)
 {
-#ifdef DEBUG
-    // positionalParams should have the same length as non-rest parameters.
-    size_t paramsCount = params->count();
-    if (paramsCount > 0) {
-        if (params->last()->isKind(ParseNodeKind::Spread)) {
-            paramsCount--;
-        }
-    }
-    MOZ_ASSERT(positionalParams.get().length() == paramsCount);
-#endif
+    // positionalParams should have the corresponding entry up to the last
+    // positional parameter.
 
+    // `positionalParams` corresponds to `expectedParams` parameter in the spec.
+    // `params` corresponds to `parseTree` in 3.1.9 CheckAssertedScope, and
+    // `positionalParamNames` parameter
+
+    // Steps 1-3.
+    // PositionalParameterNames (3.1.9 CheckAssertedScope step 5.d) and
+    // CreatePositionalParameterIndices (3.1.5 CheckPositionalParameterIndices
+    // step 1) are done implicitly.
     uint32_t i = 0;
     for (ParseNode* param : params->contents()) {
         if (param->isKind(ParseNodeKind::Assign)) {
             param = param->as<AssignmentNode>().left();
         }
-        if (param->isKind(ParseNodeKind::Spread)) {
-            continue;
-        }
+
+        if (param->isKind(ParseNodeKind::Name)) {
+            // Simple or default parameter.
 
-        MOZ_ASSERT(param->isKind(ParseNodeKind::Name) ||
-                   param->isKind(ParseNodeKind::Object) ||
-                   param->isKind(ParseNodeKind::Array));
+            // Step 2.a.
+            if (i >= positionalParams.get().length()) {
+                return raiseError("AssertedParameterScope.paramNames doesn't have corresponding entry to positional parameter");
+            }
 
-        if (JSAtom* name = positionalParams.get()[i]) {
-            // Simple or default parameter.
-            if (param->isKind(ParseNodeKind::Object) || param->isKind(ParseNodeKind::Array)) {
-                return raiseError("AssertedPositionalParameterName: expected positional parameter, got destructuring parameter");
+            JSAtom* name = positionalParams.get()[i];
+            if (!name) {
+                // Step 2.a.ii.1.
+                return raiseError("AssertedParameterScope.paramNames asserted destructuring/rest parameter, got positional parameter");
             }
-            if (param->isKind(ParseNodeKind::Spread)) {
-                return raiseError("AssertedPositionalParameterName: expected positional parameter, got rest parameter");
+
+            // Step 2.a.i.
+            if (param->name() != name) {
+                // Step 2.a.ii.1.
+                return raiseError("AssertedPositionalParameterName: name mismatch");
             }
 
-            if (param->name() != name) {
-                return raiseError("AssertedPositionalParameterName: name mismatch");
-            }
+            // Step 2.a.i.1.
+            // Implicitly done.
         } else {
             // Destructuring or rest parameter.
-            if (param->isKind(ParseNodeKind::Name)) {
-                return raiseError("AssertedParameterName/AssertedRestParameterName: expected destructuring/rest parameter, got positional parameter");
+
+            MOZ_ASSERT(param->isKind(ParseNodeKind::Object) ||
+                       param->isKind(ParseNodeKind::Array) ||
+                       param->isKind(ParseNodeKind::Spread));
+
+            // Step 3.
+            if (i >= positionalParams.get().length()) {
+                continue;
+            }
+
+            if (positionalParams.get()[i]) {
+                if (param->isKind(ParseNodeKind::Spread)) {
+                    return raiseError("AssertedParameterScope.paramNames asserted positional parameter, got rest parameter");
+                } else {
+                    return raiseError("AssertedParameterScope.paramNames asserted positional parameter, got destructuring parameter");
+                }
             }
         }
 
         i++;
     }
 
+    // Step 3.
+    if (positionalParams.get().length() > params->count()) {
+        // `positionalParams` has unchecked entries.
+        return raiseError("AssertedParameterScope.paramNames has unmatching items than the actual parameters");
+    }
+
     return Ok();
 }
 
 // Binary AST (revision 8eab67e0c434929a66ff6abe99ff790bca087dda)
 // 3.1.13 CheckFunctionLength.
 template<typename Tok> JS::Result<Ok>
 BinASTParser<Tok>::checkFunctionLength(uint32_t expectedLength)
 {
--- a/js/src/frontend/BinSource.yaml
+++ b/js/src/frontend/BinSource.yaml
@@ -272,22 +272,33 @@ AssertedMaybePositionalParameterName:
     extra-args: |
         scopeKind, positionalParams
 
 AssertedPositionalParameterName:
     inherits: AssertedMaybePositionalParameterName
     fields:
         name:
             after: |
-                // FIXME: The following checks should be performed inside
-                // checkPositionalParameterIndices to match the spec's order
-                // (bug 1490976).
-                if (index >= positionalParams.get().length()) {
-                    return raiseError("AssertedPositionalParameterName.length out of range");
+                // `positionalParams` vector can be shorter than the actual
+                // parameter length. Resize on demand.
+                // (see also ListOfAssertedMaybePositionalParameterName)
+                size_t prevLength = positionalParams.get().length();
+                if (index >= prevLength) {
+                    // This is implementation limit, which is not in the spec.
+                    size_t newLength = index + 1;
+                    if (newLength >= ARGNO_LIMIT) {
+                        return raiseError("AssertedPositionalParameterName.index is too big");
+                    }
+
+                    BINJS_TRY(positionalParams.get().resize(newLength));
+                    for (uint32_t i = prevLength; i < newLength; i++) {
+                        positionalParams.get()[i] = nullptr;
+                    }
                 }
+
                 if (positionalParams.get()[index]) {
                     return raiseError("AssertedPositionalParameterName has duplicate entry for the same index");
                 }
                 positionalParams.get()[index] = name;
 
 AssertedRestParameterName:
     inherits: AssertedMaybePositionalParameterName
 
@@ -904,23 +915,24 @@ ListOfAssertedMaybePositionalParameterNa
     extra-params: |
         AssertedScopeKind scopeKind,
         MutableHandle<GCVector<JSAtom*>> positionalParams
     extra-args: |
         scopeKind, positionalParams
     init: |
         (void) start;
         auto result = Ok();
-        if (length >= ARGNO_LIMIT) {
-            return raiseError("Too many function parameters");
-        }
-        BINJS_TRY(positionalParams.get().resize(length));
-        for (uint32_t i = 0; i < length; i++) {
-            positionalParams.get()[i] = nullptr;
-        }
+        // This list contains also destructuring parameters, and the number of
+        // list items can be greater than the actual parameters, or more than
+        // ARGNO_LIMIT even if the number of parameters fits into ARGNO_LIMIT.
+        // Also, the number of parameters can be greater than this list's length
+        // if one of destructuring parameter is empty.
+        //
+        // We resize `positionalParams` vector on demand, to keep the vector
+        // length match to the known maximum positional parameter index + 1.
 
 ListOfAssertedDeclaredName:
     inherits: ListOfAssertedBoundName
 
 ListOfDirective:
     type-ok:
         ListNode*
     init: