From 05441d12b6bbc9dde268914fcfd374db61b83462 Mon Sep 17 00:00:00 2001 From: janekptacijarabaci Date: Sun, 25 Mar 2018 18:21:15 +0200 Subject: Bug 1346862 - Fix IteratorClose due to non-local jumps being catchable by try statements inside for-of Issue #74 --- js/src/frontend/BytecodeEmitter.cpp | 20 +++++------- js/src/frontend/BytecodeEmitter.h | 2 -- js/src/jit/JitFrames.cpp | 31 ++++++++++++++++++- js/src/jsscript.h | 1 + js/src/shell/js.cpp | 6 ++-- .../Statements/for-of-iterator-close-throw.js | 35 +++++++++++++++++++++ js/src/vm/Interpreter.cpp | 36 ++++++++++++++++++++++ 7 files changed, 113 insertions(+), 18 deletions(-) create mode 100644 js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp index 4d6ff6305..b2e48d7ea 100644 --- a/js/src/frontend/BytecodeEmitter.cpp +++ b/js/src/frontend/BytecodeEmitter.cpp @@ -2065,7 +2065,11 @@ class ForOfLoopControl : public LoopControl bool emitIteratorClose(BytecodeEmitter* bce, CompletionKind completionKind = CompletionKind::Normal) { - return bce->emitIteratorClose(completionKind, allowSelfHosted_); + ptrdiff_t start = bce->offset(); + if (!bce->emitIteratorClose(completionKind, allowSelfHosted_)) + return false; + ptrdiff_t end = bce->offset(); + return bce->tryNoteList.append(JSTRY_FOR_OF_ITERCLOSE, 0, start, end); } bool emitPrepareForNonLocalJump(BytecodeEmitter* bce, bool isTarget) { @@ -2614,17 +2618,6 @@ BytecodeEmitter::emitUint32Operand(JSOp op, uint32_t operand) return true; } -bool -BytecodeEmitter::flushPops(int* npops) -{ - MOZ_ASSERT(*npops != 0); - if (!emitUint16Operand(JSOP_POPN, *npops)) - return false; - - *npops = 0; - return true; -} - namespace { class NonLocalExitControl @@ -2718,8 +2711,9 @@ NonLocalExitControl::prepareForNonLocalJump(BytecodeEmitter::NestableControl* ta bool emitIteratorCloseAtTarget = emitIteratorClose && kind_ != Continue; auto flushPops = [&npops](BytecodeEmitter* bce) { - if (npops && !bce->flushPops(&npops)) + if (npops && !bce->emitUint16Operand(JSOP_POPN, npops)) return false; + npops = 0; return true; }; diff --git a/js/src/frontend/BytecodeEmitter.h b/js/src/frontend/BytecodeEmitter.h index 7ff40b462..7ac9e540b 100644 --- a/js/src/frontend/BytecodeEmitter.h +++ b/js/src/frontend/BytecodeEmitter.h @@ -452,8 +452,6 @@ struct MOZ_STACK_CLASS BytecodeEmitter JSOp strictifySetNameOp(JSOp op); - MOZ_MUST_USE bool flushPops(int* npops); - MOZ_MUST_USE bool emitCheck(ptrdiff_t delta, ptrdiff_t* offset); // Emit one bytecode. diff --git a/js/src/jit/JitFrames.cpp b/js/src/jit/JitFrames.cpp index a70356ad4..966d952d3 100644 --- a/js/src/jit/JitFrames.cpp +++ b/js/src/jit/JitFrames.cpp @@ -436,6 +436,8 @@ HandleExceptionIon(JSContext* cx, const InlineFrameIterator& frame, ResumeFromEx if (!script->hasTrynotes()) return; + bool inForOfIterClose = false; + for (TryNoteIterIon tni(cx, frame); !tni.done(); ++tni) { JSTryNote* tn = *tni; @@ -447,12 +449,23 @@ HandleExceptionIon(JSContext* cx, const InlineFrameIterator& frame, ResumeFromEx CloseLiveIteratorIon(cx, frame, tn); break; + case JSTRY_FOR_OF_ITERCLOSE: + inForOfIterClose = true; + break; + case JSTRY_FOR_OF: + inForOfIterClose = false; + break; + case JSTRY_LOOP: break; case JSTRY_CATCH: if (cx->isExceptionPending()) { + // See corresponding comment in ProcessTryNotes. + if (inForOfIterClose) + break; + // Ion can compile try-catch, but bailing out to catch // exceptions is slow. Reset the warm-up counter so that if we // catch many exceptions we won't Ion-compile the script. @@ -583,6 +596,7 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen ResumeFromException* rfe, jsbytecode** pc) { RootedScript script(cx, frame.baselineFrame()->script()); + bool inForOfIterClose = false; for (TryNoteIterBaseline tni(cx, frame.baselineFrame(), *pc); !tni.done(); ++tni) { JSTryNote* tn = *tni; @@ -593,7 +607,11 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen // If we're closing a legacy generator, we have to skip catch // blocks. if (cx->isClosingGenerator()) - continue; + break; + + // See corresponding comment in ProcessTryNotes. + if (inForOfIterClose) + break; SettleOnTryNote(cx, tn, frame, ei, rfe, pc); @@ -609,6 +627,10 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen } case JSTRY_FINALLY: { + // See corresponding comment in ProcessTryNotes. + if (inForOfIterClose) + break; + SettleOnTryNote(cx, tn, frame, ei, rfe, pc); rfe->kind = ResumeFromException::RESUME_FINALLY; rfe->target = script->baselineScript()->nativeCodeForPC(script, *pc); @@ -652,7 +674,14 @@ ProcessTryNotesBaseline(JSContext* cx, const JitFrameIterator& frame, Environmen break; } + case JSTRY_FOR_OF_ITERCLOSE: + inForOfIterClose = true; + break; + case JSTRY_FOR_OF: + inForOfIterClose = false; + break; + case JSTRY_LOOP: break; diff --git a/js/src/jsscript.h b/js/src/jsscript.h index 9cb7c538f..87da79901 100644 --- a/js/src/jsscript.h +++ b/js/src/jsscript.h @@ -85,6 +85,7 @@ enum JSTryNoteKind { JSTRY_FOR_IN, JSTRY_FOR_OF, JSTRY_LOOP, + JSTRY_FOR_OF_ITERCLOSE, JSTRY_DESTRUCTURING_ITERCLOSE }; diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp index 5d6508d59..b53914942 100644 --- a/js/src/shell/js.cpp +++ b/js/src/shell/js.cpp @@ -2599,6 +2599,8 @@ TryNoteName(JSTryNoteKind kind) return "for-of"; case JSTRY_LOOP: return "loop"; + case JSTRY_FOR_OF_ITERCLOSE: + return "for-of-iterclose"; case JSTRY_DESTRUCTURING_ITERCLOSE: return "dstr-iterclose"; } @@ -2612,14 +2614,14 @@ TryNotes(JSContext* cx, HandleScript script, Sprinter* sp) if (!script->hasTrynotes()) return true; - if (sp->put("\nException table:\nkind stack start end\n") < 0) + if (sp->put("\nException table:\nkind stack start end\n") < 0) return false; JSTryNote* tn = script->trynotes()->vector; JSTryNote* tnlimit = tn + script->trynotes()->length; do { uint32_t startOff = script->pcToOffset(script->main()) + tn->start; - if (!sp->jsprintf(" %-14s %6u %8u %8u\n", + if (!sp->jsprintf(" %-16s %6u %8u %8u\n", TryNoteName(static_cast(tn->kind)), tn->stackDepth, startOff, startOff + tn->length)) { diff --git a/js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js b/js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js new file mode 100644 index 000000000..1974e416b --- /dev/null +++ b/js/src/tests/ecma_6/Statements/for-of-iterator-close-throw.js @@ -0,0 +1,35 @@ +function test() { + var returnCalled = 0; + var returnCalledExpected = 0; + var catchEntered = 0; + var finallyEntered = 0; + var finallyEnteredExpected = 0; + var iterable = {}; + iterable[Symbol.iterator] = makeIterator({ + ret: function() { + returnCalled++; + throw 42; + } + }); + + // inner try cannot catch IteratorClose throwing + assertThrowsValue(function() { + for (var x of iterable) { + try { + return; + } catch (e) { + catchEntered++; + } finally { + finallyEntered++; + } + } + }, 42); + assertEq(returnCalled, ++returnCalledExpected); + assertEq(catchEntered, 0); + assertEq(finallyEntered, ++finallyEnteredExpected); +} + +test(); + +if (typeof reportCompare === "function") + reportCompare(0, 0); diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp index eb3000e07..d20e5284d 100644 --- a/js/src/vm/Interpreter.cpp +++ b/js/src/vm/Interpreter.cpp @@ -1156,6 +1156,7 @@ enum HandleErrorContinuation static HandleErrorContinuation ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs) { + bool inForOfIterClose = false; for (TryNoteIterInterpreter tni(cx, regs); !tni.done(); ++tni) { JSTryNote* tn = *tni; @@ -1164,10 +1165,38 @@ ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs) /* Catch cannot intercept the closing of a generator. */ if (cx->isClosingGenerator()) break; + + // If IteratorClose due to abnormal completion threw inside a + // for-of loop, it is not catchable by try statements inside of + // the for-of loop. + // + // This is handled by this weirdness in the exception handler + // instead of in bytecode because it is hard to do so in bytecode: + // + // 1. IteratorClose emitted due to abnormal completion (break, + // throw, return) are emitted inline, at the source location of + // the break, throw, or return statement. For example: + // + // for (x of iter) { + // try { return; } catch (e) { } + // } + // + // From the try-note nesting's perspective, the IteratorClose + // resulting from |return| is covered by the inner try, when it + // should not be. + // + // 2. Try-catch notes cannot be disjoint. That is, we can't have + // multiple notes with disjoint pc ranges jumping to the same + // catch block. + if (inForOfIterClose) + break; SettleOnTryNote(cx, tn, ei, regs); return CatchContinuation; case JSTRY_FINALLY: + // See note above. + if (inForOfIterClose) + break; SettleOnTryNote(cx, tn, ei, regs); return FinallyContinuation; @@ -1206,7 +1235,14 @@ ProcessTryNotes(JSContext* cx, EnvironmentIter& ei, InterpreterRegs& regs) break; } + case JSTRY_FOR_OF_ITERCLOSE: + inForOfIterClose = true; + break; + case JSTRY_FOR_OF: + inForOfIterClose = false; + break; + case JSTRY_LOOP: break; -- cgit v1.2.3