[CVE-2017-0152] MSFT: 10592731 : Issue with Function name capturing in param scope

aneeshdk · MikeHolman · commit 9da019424601 · 2017-03-16T15:40:16.000-07:00
In a function expression with name, where the name is captured in one
of the param scope functions, if there is a function or var declaration
with the same name as the function expression name we were marking the
function expression name as shadowed. In non-eval case this causes
issue because the name symbol won't get added to the body. This change is to
fix it in such a way if the name is captured in the param scope then we
split the param and body scope such that the name symbol is added to the
param scope not body scope.
diff --git a/lib/Parser/Parse.cpp b/lib/Parser/Parse.cpp
@@ -5198,6 +5198,19 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
                         paramScope->SetCannotMergeWithBodyScope();
                     }
                 }
+                if (paramScope->GetCanMergeWithBodyScope() && !fDeclaration && pnodeFnc->sxFnc.pnodeName != nullptr)
+                {
+                    Symbol* funcSym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;
+                    if (funcSym->GetPid()->GetTopRef()->GetFuncScopeId() > pnodeFnc->sxFnc.functionId)
+                    {
+                        // This is a function expression with name captured in the param scope. In non-eval, non-split cases the function
+                        // name symbol is added to the body scope to make it accessible in the body. But if there is a function or var
+                        // declaration with the same name in the body then adding to the body will fail. So in this case we have to add
+                        // the name symbol to the param scope by splitting it.
+                        paramScope->SetCannotMergeWithBodyScope();
+                    }
+                }
+
             }
         }
 
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -3319,6 +3319,7 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)
         {
             // Emit bytecode to copy the initial values from param names to their corresponding body bindings.
             // We have to do this after the rest param is marked as false for need declaration.
+            Symbol* funcSym = funcInfo->root->sxFnc.GetFuncSymbol();
             paramScope->ForEachSymbol([&](Symbol* param) {
                 Symbol* varSym = funcInfo->GetBodyScope()->FindLocalSymbol(param->GetName());
                 Assert(varSym || pnode->sxFnc.pnodeName->sxVar.sym == param);
@@ -3327,7 +3328,9 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)
                 {
                     // Do not copy the arguments to the body if it is not used
                 }
-                else if (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister))
+                else if ((funcSym == nullptr || funcSym != param)    // Do not copy the symbol over to body as the function expression symbol
+                                                                     // is expected to stay inside the function expression scope
+                    && (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister)))
                 {
                     // Simulating EmitPropLoad here. We can't directly call the method as we have to use the param scope specifically.
                     // Walking the scope chain is not possible at this time.
diff --git a/test/es6/default-splitscope.js b/test/es6/default-splitscope.js
@@ -159,6 +159,22 @@ var tests = [
             return a;
         }
         assert.areEqual(10, f11()(), "Recursive call to the function from the body scope returns the right value when eval is there in the body");
+
+        function f13() {
+            var a = function jnvgfg(sfgnmj = function ccunlk() { jnvgfg(undefined, 1); }, b) {
+                if (b) {
+                    assert.areEqual(undefined, jnvgfg, "This refers to the instance in the body and the value of the function expression is not copied over");
+                }
+                var jnvgfg = 10;
+                if (!b) {
+                    sfgnmj();
+                    return 100;
+                }
+            };
+            assert.areEqual(100, a(), "After the recursion the right value is returned by the split scoped function");
+        };
+        f13();
+
     } 
  }, 
  { 

Original file line number	Diff line number	Diff line change
`@@ -5198,6 +5198,19 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho`
`5198`	`5198`	`paramScope->SetCannotMergeWithBodyScope();`
`5199`	`5199`	`}`
`5200`	`5200`	`}`
	`5201`	`+ if (paramScope->GetCanMergeWithBodyScope() && !fDeclaration && pnodeFnc->sxFnc.pnodeName != nullptr)`
	`5202`	`+ {`
	`5203`	`+ Symbol* funcSym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;`
	`5204`	`+ if (funcSym->GetPid()->GetTopRef()->GetFuncScopeId() > pnodeFnc->sxFnc.functionId)`
	`5205`	`+ {`
	`5206`	`+ // This is a function expression with name captured in the param scope. In non-eval, non-split cases the function`
	`5207`	`+ // name symbol is added to the body scope to make it accessible in the body. But if there is a function or var`
	`5208`	`+ // declaration with the same name in the body then adding to the body will fail. So in this case we have to add`
	`5209`	`+ // the name symbol to the param scope by splitting it.`
	`5210`	`+ paramScope->SetCannotMergeWithBodyScope();`
	`5211`	`+ }`
	`5212`	`+ }`
	`5213`	`+`
`5201`	`5214`	`}`
`5202`	`5215`	`}`
`5203`	`5216`
Original file line number	Diff line number	Diff line change
`@@ -3319,6 +3319,7 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)`
`3319`	`3319`	`{`
`3320`	`3320`	`// Emit bytecode to copy the initial values from param names to their corresponding body bindings.`
`3321`	`3321`	`// We have to do this after the rest param is marked as false for need declaration.`
	`3322`	`+ Symbol* funcSym = funcInfo->root->sxFnc.GetFuncSymbol();`
`3322`	`3323`	`paramScope->ForEachSymbol([&](Symbol* param) {`
`3323`	`3324`	`Symbol* varSym = funcInfo->GetBodyScope()->FindLocalSymbol(param->GetName());`
`3324`	`3325`	`Assert(varSym \|\| pnode->sxFnc.pnodeName->sxVar.sym == param);`
`@@ -3327,7 +3328,9 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)`
`3327`	`3328`	`{`
`3328`	`3329`	`// Do not copy the arguments to the body if it is not used`
`3329`	`3330`	`}`
`3330`		`- else if (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) \|\| varSym->GetLocation() != Js::Constants::NoRegister))`
	`3331`	`+ else if ((funcSym == nullptr \|\| funcSym != param) // Do not copy the symbol over to body as the function expression symbol`
	`3332`	`+ // is expected to stay inside the function expression scope`
	`3333`	`+ && (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) \|\| varSym->GetLocation() != Js::Constants::NoRegister)))`
`3331`	`3334`	`{`
`3332`	`3335`	`// Simulating EmitPropLoad here. We can't directly call the method as we have to use the param scope specifically.`
`3333`	`3336`	`// Walking the scope chain is not possible at this time.`