From cab68f4f8fded82d26a06f39f1b985910c548918 Mon Sep 17 00:00:00 2001 From: Jeff Walden Date: Sun, 4 Sep 2016 12:07:58 -0700 Subject: [PATCH] Bug 609756 - {Pre,Post}{in,de}crements on function calls must ToNumber after evaluating the call, in case ToNumber's effects are observable via toString/valueOf. r=arai --HG-- extra : rebase_source : 2c63da1980238a0d1c7c002506b0961064444129 --- js/src/frontend/BytecodeEmitter.cpp | 73 ++++++++------- js/src/frontend/BytecodeEmitter.h | 1 + js/src/frontend/FullParseHandler.h | 4 - js/src/frontend/ParseNode.h | 5 +- js/src/frontend/Parser.cpp | 14 ++- js/src/frontend/Parser.h | 2 +- js/src/frontend/SyntaxParseHandler.h | 4 - .../ecma_5/extensions/inc-dec-functioncall.js | 90 +++++++++++++++++++ 8 files changed, 138 insertions(+), 55 deletions(-) create mode 100644 js/src/tests/ecma_5/extensions/inc-dec-functioncall.js diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp index 1631909412cf..74ed0fd2d3bf 100644 --- a/js/src/frontend/BytecodeEmitter.cpp +++ b/js/src/frontend/BytecodeEmitter.cpp @@ -3425,6 +3425,8 @@ BytecodeEmitter::emitPropIncDec(ParseNode* pn) bool BytecodeEmitter::emitNameIncDec(ParseNode* pn) { + MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME)); + bool post; JSOp binop = GetIncDecInfo(pn->getKind(), &post); @@ -3648,6 +3650,27 @@ BytecodeEmitter::emitElemIncDec(ParseNode* pn) return true; } +bool +BytecodeEmitter::emitCallIncDec(ParseNode* incDec) +{ + MOZ_ASSERT(incDec->isKind(PNK_PREINCREMENT) || + incDec->isKind(PNK_POSTINCREMENT) || + incDec->isKind(PNK_PREDECREMENT) || + incDec->isKind(PNK_POSTDECREMENT)); + + MOZ_ASSERT(incDec->pn_kid->isKind(PNK_CALL)); + + ParseNode* call = incDec->pn_kid; + if (!emitTree(call)) // CALLRESULT + return false; + if (!emit1(JSOP_POS)) // N + return false; + + // The increment/decrement has no side effects, so proceed to throw for + // invalid assignment target. + return emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS); +} + bool BytecodeEmitter::emitNumberOp(double dval) { @@ -4418,17 +4441,9 @@ BytecodeEmitter::emitDestructuringLHS(ParseNode* target, DestructuringFlavor fla } case PNK_CALL: - MOZ_ASSERT(target->pn_xflags & PNX_SETCALL); - if (!emitTree(target)) - return false; - - // Pop the call return value. Below, we pop the RHS too, balancing - // the stack --- presumably for the benefit of bytecode - // analysis. (The interpreter will never reach these instructions - // since we just emitted JSOP_SETCALL, which always throws. It's - // possible no analyses actually depend on this either.) - if (!emit1(JSOP_POP)) - return false; + MOZ_ASSERT_UNREACHABLE("Parser::reportIfNotValidSimpleAssignmentTarget " + "rejects function calls as assignment " + "targets in destructuring assignments"); break; default: @@ -4944,9 +4959,15 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs) case PNK_OBJECT: break; case PNK_CALL: - MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL); if (!emitTree(lhs)) return false; + + // Assignment to function calls is forbidden, but we have to make the + // call first. Now we can throw. + if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS)) + return false; + + // Rebalance the stack to placate stack-depth assertions. if (!emit1(JSOP_POP)) return false; break; @@ -4993,12 +5014,9 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs) break; } case PNK_CALL: - /* - * We just emitted a JSOP_SETCALL (which will always throw) and - * popped the call's return value. Push a random value to make sure - * the stack depth is correct. - */ - MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL); + // We just emitted a JSOP_THROWMSG and popped the call's return + // value. Push a random value to make sure the stack depth is + // correct. if (!emit1(JSOP_NULL)) return false; break; @@ -5028,8 +5046,7 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs) break; } case PNK_CALL: - /* Do nothing. The JSOP_SETCALL we emitted will always throw. */ - MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL); + // We threw above, so nothing to do here. break; case PNK_ELEM: { JSOp setOp = lhs->as().isSuper() ? @@ -7527,7 +7544,6 @@ BytecodeEmitter::emitDeleteExpression(ParseNode* node) return false; if (useful) { - MOZ_ASSERT_IF(expression->isKind(PNK_CALL), !(expression->pn_xflags & PNX_SETCALL)); if (!emitTree(expression)) return false; if (!emit1(JSOP_POP)) @@ -7787,9 +7803,6 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn) switch (pn2->getKind()) { case PNK_NAME: if (emitterMode == BytecodeEmitter::SelfHosting && !spread) { - // We shouldn't see foo(bar) = x in self-hosted code. - MOZ_ASSERT(!(pn->pn_xflags & PNX_SETCALL)); - // Calls to "forceInterpreter", "callFunction", // "callContentFunction", or "resumeGenerator" in self-hosted // code generate inline bytecode. @@ -7953,10 +7966,6 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn) if (!emitUint32Operand(JSOP_LINENO, lineNum)) return false; } - if (pn->pn_xflags & PNX_SETCALL) { - if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS)) - return false; - } return true; } @@ -8065,18 +8074,14 @@ BytecodeEmitter::emitSequenceExpr(ParseNode* pn) MOZ_NEVER_INLINE bool BytecodeEmitter::emitIncOrDec(ParseNode* pn) { - /* Emit lvalue-specialized code for ++/-- operators. */ - ParseNode* pn2 = pn->pn_kid; - switch (pn2->getKind()) { + switch (pn->pn_kid->getKind()) { case PNK_DOT: return emitPropIncDec(pn); case PNK_ELEM: return emitElemIncDec(pn); case PNK_CALL: - MOZ_ASSERT(pn2->pn_xflags & PNX_SETCALL); - return emitTree(pn2); + return emitCallIncDec(pn); default: - MOZ_ASSERT(pn2->isKind(PNK_NAME)); return emitNameIncDec(pn); } diff --git a/js/src/frontend/BytecodeEmitter.h b/js/src/frontend/BytecodeEmitter.h index 1c5fa7d77f69..584bb6bc7b5a 100644 --- a/js/src/frontend/BytecodeEmitter.h +++ b/js/src/frontend/BytecodeEmitter.h @@ -501,6 +501,7 @@ struct MOZ_STACK_CLASS BytecodeEmitter MOZ_MUST_USE bool emitJumpTargetAndPatch(JumpList jump); MOZ_MUST_USE bool emitCall(JSOp op, uint16_t argc, ParseNode* pn = nullptr); + MOZ_MUST_USE bool emitCallIncDec(ParseNode* incDec); MOZ_MUST_USE bool emitLoopHead(ParseNode* nextpn, JumpTarget* top); MOZ_MUST_USE bool emitLoopEntry(ParseNode* nextpn, JumpList entryJump); diff --git a/js/src/frontend/FullParseHandler.h b/js/src/frontend/FullParseHandler.h index 28ae341b1a48..b2a2f66c3b86 100644 --- a/js/src/frontend/FullParseHandler.h +++ b/js/src/frontend/FullParseHandler.h @@ -196,10 +196,6 @@ class FullParseHandler return new_(cond, thenExpr, elseExpr); } - void markAsSetCall(ParseNode* pn) { - pn->pn_xflags |= PNX_SETCALL; - } - ParseNode* newDelete(uint32_t begin, ParseNode* expr) { if (expr->isKind(PNK_NAME)) { expr->setOp(JSOP_DELNAME); diff --git a/js/src/frontend/ParseNode.h b/js/src/frontend/ParseNode.h index 371722708a5a..f008a1a7c165 100644 --- a/js/src/frontend/ParseNode.h +++ b/js/src/frontend/ParseNode.h @@ -621,11 +621,10 @@ class ParseNode /* PN_LIST pn_xflags bits. */ #define PNX_FUNCDEFS 0x01 /* contains top-level function statements */ -#define PNX_SETCALL 0x02 /* call expression in lvalue context */ -#define PNX_ARRAYHOLESPREAD 0x04 /* one or more of +#define PNX_ARRAYHOLESPREAD 0x02 /* one or more of 1. array initialiser has holes 2. array initializer has spread node */ -#define PNX_NONCONST 0x08 /* initialiser has non-constants */ +#define PNX_NONCONST 0x04 /* initialiser has non-constants */ bool functionIsHoisted() const { MOZ_ASSERT(pn_arity == PN_CODE && getKind() == PNK_FUNCTION); diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp index fa9c62f6a344..85ae7f03bd0f 100644 --- a/js/src/frontend/Parser.cpp +++ b/js/src/frontend/Parser.cpp @@ -3741,7 +3741,7 @@ Parser::PossibleError::transferErrorTo(PossibleError* other) template bool -Parser::makeSetCall(Node target, unsigned msg) +Parser::checkAssignmentToCall(Node target, unsigned msg) { MOZ_ASSERT(handler.isFunctionCall(target)); @@ -3749,11 +3749,7 @@ Parser::makeSetCall(Node target, unsigned msg) // concerned about sites using this in dead code, so forbid it only in // strict mode code (or if the werror option has been set), and otherwise // warn. - if (!report(ParseStrictError, pc->sc()->strict(), target, msg)) - return false; - - handler.markAsSetCall(target); - return true; + return report(ParseStrictError, pc->sc()->strict(), target, msg); } template <> @@ -5046,7 +5042,7 @@ Parser::validateForInOrOfLHSExpression(Node target) } if (handler.isFunctionCall(target)) - return makeSetCall(target, JSMSG_BAD_FOR_LEFTSIDE); + return checkAssignmentToCall(target, JSMSG_BAD_FOR_LEFTSIDE); report(ParseError, false, target, JSMSG_BAD_FOR_LEFTSIDE); return false; @@ -7163,7 +7159,7 @@ Parser::checkAndMarkAsAssignmentLhs(Node target, AssignmentFlavor } MOZ_ASSERT(handler.isFunctionCall(target)); - return makeSetCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS); + return checkAssignmentToCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS); } class AutoClearInDestructuringDecl @@ -7483,7 +7479,7 @@ Parser::checkAndMarkAsIncOperand(Node target, AssignmentFlavor fla if (!reportIfArgumentsEvalTarget(target)) return false; } else if (handler.isFunctionCall(target)) { - if (!makeSetCall(target, JSMSG_BAD_INCOP_OPERAND)) + if (!checkAssignmentToCall(target, JSMSG_BAD_INCOP_OPERAND)) return false; } return true; diff --git a/js/src/frontend/Parser.h b/js/src/frontend/Parser.h index a08ae0f0c049..73cc9b527716 100644 --- a/js/src/frontend/Parser.h +++ b/js/src/frontend/Parser.h @@ -1319,7 +1319,7 @@ class Parser final : private JS::AutoGCRooter, public StrictModeGetter bool checkDestructuringObject(Node objectPattern, mozilla::Maybe maybeDecl); bool checkDestructuringName(Node expr, mozilla::Maybe maybeDecl); - bool makeSetCall(Node node, unsigned errnum); + bool checkAssignmentToCall(Node node, unsigned errnum); Node newNumber(const Token& tok) { return handler.newNumber(tok.number(), tok.decimalPoint(), tok.pos); diff --git a/js/src/frontend/SyntaxParseHandler.h b/js/src/frontend/SyntaxParseHandler.h index 89c8d36391f6..cc633bbf4f86 100644 --- a/js/src/frontend/SyntaxParseHandler.h +++ b/js/src/frontend/SyntaxParseHandler.h @@ -219,10 +219,6 @@ class SyntaxParseHandler Node newElision() { return NodeGeneric; } - void markAsSetCall(Node node) { - MOZ_ASSERT(node == NodeFunctionCall); - } - Node newDelete(uint32_t begin, Node expr) { return NodeGeneric; } diff --git a/js/src/tests/ecma_5/extensions/inc-dec-functioncall.js b/js/src/tests/ecma_5/extensions/inc-dec-functioncall.js new file mode 100644 index 000000000000..b4de03c6c05d --- /dev/null +++ b/js/src/tests/ecma_5/extensions/inc-dec-functioncall.js @@ -0,0 +1,90 @@ +/* + * Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/licenses/publicdomain/ + */ + +//----------------------------------------------------------------------------- +var BUGNUMBER = 609756; +var summary = + "Perform ToNumber on the result of the |fun()| in |fun()++| before " + + "throwing"; + +print(BUGNUMBER + ": " + summary); + +/************** + * BEGIN TEST * + **************/ + +var hadSideEffect; + +function f() +{ + return { valueOf: function() { hadSideEffect = true; return 0; } }; +} + +hadSideEffect = false; +assertThrowsInstanceOf(function() { f()++; }, ReferenceError); +assertEq(hadSideEffect, true); + +hadSideEffect = false; +assertThrowsInstanceOf(function() { + for (var i = 0; i < 20; i++) + { + if (i > 18) + f()++; + } +}, ReferenceError); +assertEq(hadSideEffect, true); + + +hadSideEffect = false; +assertThrowsInstanceOf(function() { f()--; }, ReferenceError); +assertEq(hadSideEffect, true); + +hadSideEffect = false; +assertThrowsInstanceOf(function() { + for (var i = 0; i < 20; i++) + { + if (i > 18) + f()--; + } +}, ReferenceError); +assertEq(hadSideEffect, true); + + +hadSideEffect = false; +assertThrowsInstanceOf(function() { ++f(); }, ReferenceError); +assertEq(hadSideEffect, true); + +hadSideEffect = false; +assertThrowsInstanceOf(function() { + for (var i = 0; i < 20; i++) + { + if (i > 18) + ++f(); + } +}, ReferenceError); +assertEq(hadSideEffect, true); + + +hadSideEffect = false; +assertThrowsInstanceOf(function() { --f(); }, ReferenceError); +assertEq(hadSideEffect, true); + +hadSideEffect = false; +assertThrowsInstanceOf(function() { + for (var i = 0; i < 20; i++) + { + if (i > 18) + --f(); + } +}, ReferenceError); +assertEq(hadSideEffect, true); + + +/******************************************************************************/ + +if (typeof reportCompare === "function") + reportCompare(true, true); + +print("Tests complete");