Skip to content

Parse-time nested function escape detection #1416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chakrabot merged 1 commit into from
Aug 11, 2016
+81 −9
Merged

Parse-time nested function escape detection #1416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/Common/ConfigFlagsList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ PHASE(All)
PHASE(CachedScope)
PHASE(StackFunc)
PHASE(StackClosure)
PHASE(DisableStackFuncOnDeferredEscape)
PHASE(DelayCapture)
PHASE(DebuggerScope)
PHASE(ByteCodeSerialization)
Expand Down
68 changes: 63 additions & 5 deletions lib/Parser/Parse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ HRESULT Parser::ValidateSyntax(LPCUTF8 pszSrc, size_t encodedCharCount, bool isG
m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;
m_currentNodeFunc = pnodeFnc;
m_currentNodeDeferredFunc = NULL;
m_sourceContextInfo = nullptr;
AssertMsg(m_pstmtCur == NULL, "Statement stack should be empty when we start parse function body");

ParseNodePtr block = StartParseBlock<false>(PnodeBlockType::Function, ScopeType_FunctionBody);
Expand Down Expand Up @@ -2776,7 +2777,8 @@ ParseNodePtr Parser::ParseTerm(BOOL fAllowCall,
_Inout_opt_ IdentToken* pToken /*= nullptr*/,
bool fUnaryOrParen /*= false*/,
_Out_opt_ BOOL* pfCanAssign /*= nullptr*/,
_Inout_opt_ BOOL* pfLikelyPattern /*= nullptr*/)
_Inout_opt_ BOOL* pfLikelyPattern /*= nullptr*/,
_Out_opt_ bool* pfIsDotOrIndex /*= nullptr*/)
{
ParseNodePtr pnode = nullptr;
charcount_t ichMin = 0;
Expand Down Expand Up @@ -3152,7 +3154,7 @@ LFunction :
break;
}

pnode = ParsePostfixOperators<buildAST>(pnode, fAllowCall, fInNew, &fCanAssign, &term);
pnode = ParsePostfixOperators<buildAST>(pnode, fAllowCall, fInNew, &fCanAssign, &term, pfIsDotOrIndex);

// Pass back identifier if requested
if (pToken && term.tk == tkID)
Expand Down Expand Up @@ -3248,10 +3250,15 @@ ParseNodePtr Parser::ParsePostfixOperators(
BOOL fAllowCall,
BOOL fInNew,
BOOL *pfCanAssign,
_Inout_ IdentToken* pToken)
_Inout_ IdentToken* pToken,
_Out_opt_ bool* pfIsDotOrIndex /*= nullptr */)
{
uint16 count = 0;
bool callOfConstants = false;
if (pfIsDotOrIndex)
{
*pfIsDotOrIndex = false;
}

for (;;)
{
Expand All @@ -3277,6 +3284,7 @@ ParseNodePtr Parser::ParsePostfixOperators(
}
else
{
pnode = nullptr;
pToken->tk = tkNone; // This is no longer an identifier
}
fInNew = FALSE;
Expand Down Expand Up @@ -3316,6 +3324,7 @@ ParseNodePtr Parser::ParsePostfixOperators(
}
else
{
pnode = nullptr;
if (pToken->tk == tkID && pToken->pid == wellKnownPropertyPids.eval && count > 0) // Detect eval
{
this->MarkEvalCaller();
Expand All @@ -3328,6 +3337,10 @@ ParseNodePtr Parser::ParsePostfixOperators(
{
*pfCanAssign = FALSE;
}
if (pfIsDotOrIndex)
{
*pfIsDotOrIndex = false;
}
break;
}
case tkLBrack:
Expand All @@ -3341,13 +3354,18 @@ ParseNodePtr Parser::ParsePostfixOperators(
}
else
{
pnode = nullptr;
pToken->tk = tkNone; // This is no longer an identifier
}
ChkCurTok(tkRBrack, ERRnoRbrack);
if (pfCanAssign)
{
*pfCanAssign = TRUE;
}
if (pfIsDotOrIndex)
{
*pfIsDotOrIndex = true;
}

if (!buildAST)
{
Expand Down Expand Up @@ -3443,13 +3461,18 @@ ParseNodePtr Parser::ParsePostfixOperators(
}
else
{
pnode = nullptr;
pToken->tk = tkNone;
}

if (pfCanAssign)
{
*pfCanAssign = TRUE;
}
if (pfIsDotOrIndex)
{
*pfIsDotOrIndex = true;
}
m_pscan->Scan();

break;
Expand All @@ -3470,6 +3493,10 @@ ParseNodePtr Parser::ParsePostfixOperators(
{
*pfCanAssign = FALSE;
}
if (pfIsDotOrIndex)
{
*pfIsDotOrIndex = false;
}
break;
}
default:
Expand Down Expand Up @@ -4444,6 +4471,7 @@ ParseNodePtr Parser::ParseFncDecl(ushort flags, LPCOLESTR pNameHint, const bool
pnodeFnc->sxFnc.hintOffset = 0;
pnodeFnc->sxFnc.hintLength = 0;
pnodeFnc->sxFnc.isNameIdentifierRef = true;
pnodeFnc->sxFnc.nestedFuncEscapes = false;
pnodeFnc->sxFnc.pnodeNext = nullptr;
pnodeFnc->sxFnc.pnodeParams = nullptr;
pnodeFnc->sxFnc.pnodeVars = nullptr;
Expand Down Expand Up @@ -5727,6 +5755,7 @@ ParseNodePtr Parser::CreateDummyFuncNode(bool fDeclaration)
pnodeFnc->sxFnc.hintOffset = 0;
pnodeFnc->sxFnc.hintLength = 0;
pnodeFnc->sxFnc.isNameIdentifierRef = true;
pnodeFnc->sxFnc.nestedFuncEscapes = false;
pnodeFnc->sxFnc.pnodeNext = nullptr;
pnodeFnc->sxFnc.pnodeParams = nullptr;
pnodeFnc->sxFnc.pnodeVars = nullptr;
Expand Down Expand Up @@ -6293,6 +6322,7 @@ ParseNodePtr Parser::GenerateEmptyConstructor(bool extends)
pnodeFnc->sxFnc.hintOffset = 0;
pnodeFnc->sxFnc.hintLength = 0;
pnodeFnc->sxFnc.isNameIdentifierRef = true;
pnodeFnc->sxFnc.nestedFuncEscapes = false;
pnodeFnc->sxFnc.pnodeName = nullptr;
pnodeFnc->sxFnc.pnodeScopes = nullptr;
pnodeFnc->sxFnc.pnodeParams = nullptr;
Expand Down Expand Up @@ -7989,7 +8019,20 @@ bool Parser::ParseOptionalExpr(ParseNodePtr* pnode, bool fUnaryOrParen, int oplM
return false;
}

*pnode = ParseExpr<buildAST>(oplMin, pfCanAssign, fAllowIn, fAllowEllipsis, nullptr /*pNameHint*/, nullptr /*pHintLength*/, nullptr /*pShortNameOffset*/, pToken, fUnaryOrParen);
ParseNodePtr pnodeT = ParseExpr<buildAST>(oplMin, pfCanAssign, fAllowIn, fAllowEllipsis, nullptr /*pNameHint*/, nullptr /*pHintLength*/, nullptr /*pShortNameOffset*/, pToken, fUnaryOrParen);
// Detect nested function escapes of the pattern "return function(){...}" or "yield function(){...}".
// Doing so in the parser allows us to disable stack-nested-functions in common cases where an escape
// is not detected at byte code gen time because of deferred parsing.
if (m_currentNodeFunc && pnodeT && pnodeT->nop == knopFncDecl)
{
if (m_sourceContextInfo ?
!PHASE_OFF_RAW(Js::DisableStackFuncOnDeferredEscapePhase, m_sourceContextInfo->sourceContextId, m_currentNodeFunc->sxFnc.functionId) :
!PHASE_OFF1(Js::DisableStackFuncOnDeferredEscapePhase))
{
m_currentNodeFunc->sxFnc.SetNestedFuncEscapes();
}
}
*pnode = pnodeT;
return true;
}

Expand Down Expand Up @@ -8018,6 +8061,7 @@ ParseNodePtr Parser::ParseExpr(int oplMin,
ParseNodePtr pnodeT = nullptr;
BOOL fCanAssign = TRUE;
bool assignmentStmt = false;
bool fIsDotOrIndex = false;
IdentToken term;
RestorePoint termStart;
uint32 hintLength = 0;
Expand Down Expand Up @@ -8221,7 +8265,7 @@ ParseNodePtr Parser::ParseExpr(int oplMin,
{
ichMin = m_pscan->IchMinTok();
BOOL fLikelyPattern = FALSE;
pnode = ParseTerm<buildAST>(TRUE, pNameHint, &hintLength, &hintOffset, &term, fUnaryOrParen, &fCanAssign, IsES6DestructuringEnabled() ? &fLikelyPattern : nullptr);
pnode = ParseTerm<buildAST>(TRUE, pNameHint, &hintLength, &hintOffset, &term, fUnaryOrParen, &fCanAssign, IsES6DestructuringEnabled() ? &fLikelyPattern : nullptr, &fIsDotOrIndex);
if (pfLikelyPattern != nullptr)
{
*pfLikelyPattern = !!fLikelyPattern;
Expand Down Expand Up @@ -8443,6 +8487,19 @@ ParseNodePtr Parser::ParseExpr(int oplMin,
// Parse the operand, make a new node, and look for more
pnodeT = ParseExpr<buildAST>(opl, NULL, fAllowIn, FALSE, pNameHint, &hintLength, &hintOffset, nullptr);

// Detect nested function escapes of the pattern "o.f = function(){...}" or "o[s] = function(){...}".
Copy link
Contributor

@akroshg akroshg Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will it handle the case as
[o.x] = [function() {...}];
?

Copy link
Contributor Author

@pleath pleath Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it won't. It's not actually meant to be exhaustive, just a cheap pattern-match to catch a certain common class of cases.

// Doing so in the parser allows us to disable stack-nested-functions in common cases where an escape
// is not detected at byte code gen time because of deferred parsing.
if (m_currentNodeFunc && pnodeT && pnodeT->nop == knopFncDecl && fIsDotOrIndex && nop == knopAsg)
Copy link
Contributor

@akroshg akroshg Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this m_currentNodeFunc be null when we are at deferred parsing? If yes then how are we making sure to call SetNestedFuncEscapes?

Copy link
Contributor Author

@pleath pleath Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily. It will be non-null if we're enclosed in a non-deferred function, regardless of whether the current (possibly nested) function is deferred. If we're not enclosed in a non-deferred function, we won't generate any stack-nested-function byte code.

Copy link
Contributor

@akroshg akroshg Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good.

In reply to: 74467191 [](ancestors = 74467191)

{
if (m_sourceContextInfo ?
!PHASE_OFF_RAW(Js::DisableStackFuncOnDeferredEscapePhase, m_sourceContextInfo->sourceContextId, m_currentNodeFunc->sxFnc.functionId) :
!PHASE_OFF1(Js::DisableStackFuncOnDeferredEscapePhase))
{
m_currentNodeFunc->sxFnc.SetNestedFuncEscapes();
}
}

if (buildAST)
{
pnode = CreateBinNode(nop, pnode, pnodeT);
Expand Down Expand Up @@ -10886,6 +10943,7 @@ ParseNodePtr Parser::Parse(LPCUTF8 pszSrc, size_t offset, size_t length, charcou
pnodeProg->sxFnc.hintLength = 0;
pnodeProg->sxFnc.hintOffset = 0;
pnodeProg->sxFnc.isNameIdentifierRef = true;
pnodeProg->sxFnc.nestedFuncEscapes = false;

// initialize parsing variables
pnodeProg->sxFnc.pnodeNext = nullptr;
Expand Down
12 changes: 9 additions & 3 deletions lib/Parser/Parse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,9 +827,15 @@ class Parser
_Inout_opt_ IdentToken* pToken = nullptr,
bool fUnaryOrParen = false,
_Out_opt_ BOOL* pfCanAssign = nullptr,
_Inout_opt_ BOOL* pfLikelyPattern = nullptr);
template<bool buildAST> ParseNodePtr ParsePostfixOperators(ParseNodePtr pnode,
BOOL fAllowCall, BOOL fInNew, BOOL *pfCanAssign, _Inout_ IdentToken* pToken);
_Inout_opt_ BOOL* pfLikelyPattern = nullptr,
_Out_opt_ bool* pfIsDotOrIndex = nullptr);
template<bool buildAST> ParseNodePtr ParsePostfixOperators(
ParseNodePtr pnode,
BOOL fAllowCall,
BOOL fInNew,
BOOL *pfCanAssign,
_Inout_ IdentToken* pToken,
_Out_opt_ bool* pfIsDotOrIndex = nullptr);

void ThrowNewTargetSyntaxErrForGlobalScope();

Expand Down
3 changes: 3 additions & 0 deletions lib/Parser/ptree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ struct PnFnc
uint32 hintLength;
uint32 hintOffset;
bool isNameIdentifierRef;
bool nestedFuncEscapes;
ParseNodePtr pnodeScopes;
ParseNodePtr pnodeBodyScope;
ParseNodePtr pnodeParams;
Expand Down Expand Up @@ -309,6 +310,7 @@ struct PnFnc
void SetIsModule(bool set = true) { SetFlags(kFunctionIsModule, set); }
void SetUsesArguments(bool set = true) { SetFlags(kFunctionUsesArguments, set); }
void SetIsDefaultModuleExport(bool set = true) { SetFlags(kFunctionIsDefaultModuleExport, set); }
void SetNestedFuncEscapes(bool set = true) { nestedFuncEscapes = set; }

bool CallsEval() const { return HasFlags(kFunctionCallsEval); }
bool ChildCallsEval() const { return HasFlags(kFunctionChildCallsEval); }
Expand Down Expand Up @@ -344,6 +346,7 @@ struct PnFnc
bool NameIsHidden() const { return HasFlags(kFunctionNameIsHidden); }
bool UsesArguments() const { return HasFlags(kFunctionUsesArguments); }
bool IsDefaultModuleExport() const { return HasFlags(kFunctionIsDefaultModuleExport); }
bool NestedFuncEscapes() const { return nestedFuncEscapes; }

size_t LengthInBytes()
{
Expand Down
4 changes: 4 additions & 0 deletions lib/Runtime/ByteCode/FuncInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ FuncInfo::FuncInfo(
{
paramScope->SetFunc(this);
}
if (pnode && pnode->sxFnc.NestedFuncEscapes())
{
this->SetHasMaybeEscapedNestedFunc(DebugOnly(_u("Child")));
}
}

bool FuncInfo::IsGlobalFunction() const
Expand Down
2 changes: 1 addition & 1 deletion test/stackfunc/rlexe.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@
<default>
<files>funcexpr.js</files>
<baseline>funcexpr.deferparse.baseline</baseline>
<compile-flags>-testtrace:stackfunc -off:simpleJit -on:stackfunc -force:deferparse</compile-flags>
<compile-flags>-testtrace:stackfunc -off:simpleJit -on:stackfunc -force:deferparse -off:disablestackfuncondeferredescape</compile-flags>
<tags>exclude_fre,exclude_dynapogo,exclude_arm</tags>
</default>
</test>
Expand Down