From 500bbf02e9a663a71ca484014217d019fb81f5cd Mon Sep 17 00:00:00 2001 From: janekptacijarabaci Date: Fri, 5 May 2017 21:21:12 +0200 Subject: [PATCH] Make creation of an Error not capture a filename/lineno it shouldn't have access to; A potential crash --- dom/canvas/WebGLContextUtils.cpp | 11 ++++- js/src/frontend/TokenStream.cpp | 1 + js/src/jit-test/tests/debug/bug1148917.js | 14 +++++++ js/src/jscntxt.cpp | 4 +- js/src/jsexn.cpp | 7 ++-- js/src/vm/Debugger.cpp | 13 +++--- js/src/vm/Stack.cpp | 21 ++++++---- js/src/vm/Stack.h | 46 +++++++++++++++------ js/xpconnect/tests/chrome/test_xrayToJS.xul | 6 +-- 9 files changed, 88 insertions(+), 35 deletions(-) create mode 100644 js/src/jit-test/tests/debug/bug1148917.js diff --git a/dom/canvas/WebGLContextUtils.cpp b/dom/canvas/WebGLContextUtils.cpp index 914bbec0ff..e0f24973ec 100644 --- a/dom/canvas/WebGLContextUtils.cpp +++ b/dom/canvas/WebGLContextUtils.cpp @@ -425,7 +425,16 @@ WebGLContext::GenerateWarning(const char* fmt, va_list ap) // no need to print to stderr, as JS_ReportWarning takes care of this for us. - AutoJSContext cx; + if (!mCanvasElement) { + return; + } + + AutoJSAPI api; + if (!api.Init(mCanvasElement->OwnerDoc()->GetScopeObject())) { + return; + } + + JSContext* cx = api.cx(); JS_ReportWarning(cx, "WebGL: %s", buf); if (!ShouldGenerateWarnings()) { JS_ReportWarning(cx, diff --git a/js/src/frontend/TokenStream.cpp b/js/src/frontend/TokenStream.cpp index 2f80c4bcd6..2fcc39dc35 100644 --- a/js/src/frontend/TokenStream.cpp +++ b/js/src/frontend/TokenStream.cpp @@ -637,6 +637,7 @@ TokenStream::reportCompileErrorNumberVA(uint32_t offset, unsigned flags, unsigne if (offset != NoOffset && !err.report.filename && cx->isJSContext()) { NonBuiltinFrameIter iter(cx->asJSContext(), FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED, + FrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK, cx->compartment()->principals); if (!iter.done() && iter.scriptFilename()) { callerFilename = true; diff --git a/js/src/jit-test/tests/debug/bug1148917.js b/js/src/jit-test/tests/debug/bug1148917.js new file mode 100644 index 0000000000..2645f913f3 --- /dev/null +++ b/js/src/jit-test/tests/debug/bug1148917.js @@ -0,0 +1,14 @@ +// |jit-test| error: Error + +var g = newGlobal(); +g.eval('function f(a) { evaluate("f(" + " - 1);", {newContext: true}); }'); +var dbg = new Debugger(g); +var frames = []; +dbg.onEnterFrame = function (frame) { + if (frames.length == 3) + return; + frames.push(frame); + for (var f of frames) + f.eval('a').return +}; +g.f(); diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp index 97cf56d55a..9b1a3d7092 100644 --- a/js/src/jscntxt.cpp +++ b/js/src/jscntxt.cpp @@ -248,9 +248,9 @@ PopulateReportBlame(JSContext* cx, JSErrorReport* report) { /* * Walk stack until we find a frame that is associated with a non-builtin - * rather than a builtin frame. + * rather than a builtin frame and which we're allowed to know about. */ - NonBuiltinFrameIter iter(cx); + NonBuiltinFrameIter iter(cx, cx->compartment()->principals); if (iter.done()) return; diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp index 030fb7a049..f49140e8f4 100644 --- a/js/src/jsexn.cpp +++ b/js/src/jsexn.cpp @@ -271,6 +271,7 @@ js::ComputeStackString(JSContext* cx) RootedAtom atom(cx); SuppressErrorsGuard seg(cx); for (NonBuiltinFrameIter i(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED, + FrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK, cx->compartment()->principals); !i.done(); ++i) @@ -363,8 +364,8 @@ Error(JSContext* cx, unsigned argc, Value* vp) return false; } - /* Find the scripted caller. */ - NonBuiltinFrameIter iter(cx); + /* Find the scripted caller, but only ones we're allowed to know about. */ + NonBuiltinFrameIter iter(cx, cx->compartment()->principals); /* Set the 'fileName' property. */ RootedString fileName(cx); @@ -885,7 +886,7 @@ ErrorReport::populateUncaughtExceptionReportVA(JSContext* cx, va_list ap) // XXXbz this assumes the stack we have right now is still // related to our exception object. It would be better if we // could accept a passed-in stack of some sort instead. - NonBuiltinFrameIter iter(cx); + NonBuiltinFrameIter iter(cx, cx->compartment()->principals); if (!iter.done()) { ownedReport.filename = iter.scriptFilename(); ownedReport.lineno = iter.computeLine(&ownedReport.column); diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index 26262fd462..e8f279d90e 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -5578,7 +5578,7 @@ CheckThisFrame(JSContext* cx, const CallArgs& args, const char* fnname, bool che THIS_FRAME_THISOBJ(cx, argc, vp, fnname, args, thisobj); \ AbstractFramePtr frame = AbstractFramePtr::FromRaw(thisobj->getPrivate()); \ if (frame.isScriptFrameIterData()) { \ - ScriptFrameIter iter(*(ScriptFrameIter::Data*)(frame.raw())); \ + ScriptFrameIter iter(*(ScriptFrameIter::Data*)(frame.raw())); \ frame = iter.abstractFramePtr(); \ } @@ -5588,10 +5588,11 @@ CheckThisFrame(JSContext* cx, const CallArgs& args, const char* fnname, bool che { \ AbstractFramePtr f = AbstractFramePtr::FromRaw(thisobj->getPrivate()); \ if (f.isScriptFrameIterData()) { \ - maybeIter.emplace(*(ScriptFrameIter::Data*)(f.raw())); \ + maybeIter.emplace(*(ScriptFrameIter::Data*)(f.raw())); \ } else { \ maybeIter.emplace(cx, ScriptFrameIter::ALL_CONTEXTS, \ - ScriptFrameIter::GO_THROUGH_SAVED); \ + ScriptFrameIter::GO_THROUGH_SAVED, \ + ScriptFrameIter::IGNORE_DEBUGGER_EVAL_PREV_LINK); \ ScriptFrameIter& iter = *maybeIter; \ while (!iter.hasUsableAbstractFramePtr() || iter.abstractFramePtr() != f) \ ++iter; \ @@ -6294,7 +6295,7 @@ DebuggerObject_checkThis(JSContext* cx, const CallArgs& args, const char* fnname RootedObject obj(cx, DebuggerObject_checkThis(cx, args, fnname)); \ if (!obj) \ return false; \ - obj = (JSObject*) obj->as().getPrivate(); \ + obj = (JSObject*) obj->as().getPrivate(); \ MOZ_ASSERT(obj) #define THIS_DEBUGOBJECT_OWNER_REFERENT(cx, argc, vp, fnname, args, dbg, obj) \ @@ -6303,7 +6304,7 @@ DebuggerObject_checkThis(JSContext* cx, const CallArgs& args, const char* fnname if (!obj) \ return false; \ Debugger* dbg = Debugger::fromChildJSObject(obj); \ - obj = (JSObject*) obj->as().getPrivate(); \ + obj = (JSObject*) obj->as().getPrivate(); \ MOZ_ASSERT(obj) static bool @@ -7227,7 +7228,7 @@ DebuggerEnv_checkThis(JSContext* cx, const CallArgs& args, const char* fnname, NativeObject* envobj = DebuggerEnv_checkThis(cx, args, fnname); \ if (!envobj) \ return false; \ - Rooted env(cx, static_cast(envobj->getPrivate())); \ + Rooted env(cx, static_cast(envobj->getPrivate())); \ MOZ_ASSERT(env); \ MOZ_ASSERT(!env->is()) diff --git a/js/src/vm/Stack.cpp b/js/src/vm/Stack.cpp index a301ed48d0..0b3e0577eb 100644 --- a/js/src/vm/Stack.cpp +++ b/js/src/vm/Stack.cpp @@ -582,10 +582,12 @@ FrameIter::settleOnActivation() } FrameIter::Data::Data(JSContext* cx, SavedOption savedOption, - ContextOption contextOption, JSPrincipals* principals) + ContextOption contextOption, DebuggerEvalOption debuggerEvalOption, + JSPrincipals* principals) : cx_(cx), savedOption_(savedOption), contextOption_(contextOption), + debuggerEvalOption_(debuggerEvalOption), principals_(principals), pc_(nullptr), interpFrames_(nullptr), @@ -600,6 +602,7 @@ FrameIter::Data::Data(const FrameIter::Data& other) : cx_(other.cx_), savedOption_(other.savedOption_), contextOption_(other.contextOption_), + debuggerEvalOption_(other.debuggerEvalOption_), principals_(other.principals_), state_(other.state_), pc_(other.pc_), @@ -612,7 +615,7 @@ FrameIter::Data::Data(const FrameIter::Data& other) } FrameIter::FrameIter(JSContext* cx, SavedOption savedOption) - : data_(cx, savedOption, CURRENT_CONTEXT, nullptr), + : data_(cx, savedOption, CURRENT_CONTEXT, FOLLOW_DEBUGGER_EVAL_PREV_LINK, nullptr), ionInlineFrames_(cx, (js::jit::JitFrameIterator*) nullptr) { // settleOnActivation can only GC if principals are given. @@ -621,8 +624,8 @@ FrameIter::FrameIter(JSContext* cx, SavedOption savedOption) } FrameIter::FrameIter(JSContext* cx, ContextOption contextOption, - SavedOption savedOption) - : data_(cx, savedOption, contextOption, nullptr), + SavedOption savedOption, DebuggerEvalOption debuggerEvalOption) + : data_(cx, savedOption, contextOption, debuggerEvalOption, nullptr), ionInlineFrames_(cx, (js::jit::JitFrameIterator*) nullptr) { // settleOnActivation can only GC if principals are given. @@ -631,8 +634,9 @@ FrameIter::FrameIter(JSContext* cx, ContextOption contextOption, } FrameIter::FrameIter(JSContext* cx, ContextOption contextOption, - SavedOption savedOption, JSPrincipals* principals) - : data_(cx, savedOption, contextOption, principals), + SavedOption savedOption, DebuggerEvalOption debuggerEvalOption, + JSPrincipals* principals) + : data_(cx, savedOption, contextOption, debuggerEvalOption, principals), ionInlineFrames_(cx, (js::jit::JitFrameIterator*) nullptr) { settleOnActivation(); @@ -709,7 +713,10 @@ FrameIter::operator++() case DONE: MOZ_CRASH("Unexpected state"); case INTERP: - if (interpFrame()->isDebuggerEvalFrame() && interpFrame()->evalInFramePrev()) { + if (interpFrame()->isDebuggerEvalFrame() && + interpFrame()->evalInFramePrev() && + data_.debuggerEvalOption_ == FOLLOW_DEBUGGER_EVAL_PREV_LINK) + { AbstractFramePtr eifPrev = interpFrame()->evalInFramePrev(); // Eval-in-frame can cross contexts and works across saved frame diff --git a/js/src/vm/Stack.h b/js/src/vm/Stack.h index 07647c9048..29a6869e2d 100644 --- a/js/src/vm/Stack.h +++ b/js/src/vm/Stack.h @@ -1549,6 +1549,8 @@ class FrameIter public: enum SavedOption { STOP_AT_SAVED, GO_THROUGH_SAVED }; enum ContextOption { CURRENT_CONTEXT, ALL_CONTEXTS }; + enum DebuggerEvalOption { FOLLOW_DEBUGGER_EVAL_PREV_LINK, + IGNORE_DEBUGGER_EVAL_PREV_LINK }; enum State { DONE, INTERP, JIT, ASMJS }; // Unlike ScriptFrameIter itself, ScriptFrameIter::Data can be allocated on @@ -1558,6 +1560,7 @@ class FrameIter JSContext * cx_; SavedOption savedOption_; ContextOption contextOption_; + DebuggerEvalOption debuggerEvalOption_; JSPrincipals * principals_; State state_; @@ -1572,13 +1575,14 @@ class FrameIter AsmJSFrameIterator asmJSFrames_; Data(JSContext* cx, SavedOption savedOption, ContextOption contextOption, - JSPrincipals* principals); + DebuggerEvalOption debuggerEvalOption, JSPrincipals* principals); Data(const Data& other); }; MOZ_IMPLICIT FrameIter(JSContext* cx, SavedOption = STOP_AT_SAVED); - FrameIter(JSContext* cx, ContextOption, SavedOption); - FrameIter(JSContext* cx, ContextOption, SavedOption, JSPrincipals*); + FrameIter(JSContext* cx, ContextOption, SavedOption, + DebuggerEvalOption = FOLLOW_DEBUGGER_EVAL_PREV_LINK); + FrameIter(JSContext* cx, ContextOption, SavedOption, DebuggerEvalOption, JSPrincipals*); FrameIter(const FrameIter& iter); MOZ_IMPLICIT FrameIter(const Data& data); MOZ_IMPLICIT FrameIter(AbstractFramePtr frame); @@ -1719,8 +1723,9 @@ class ScriptFrameIter : public FrameIter ScriptFrameIter(JSContext* cx, ContextOption cxOption, - SavedOption savedOption) - : FrameIter(cx, cxOption, savedOption) + SavedOption savedOption, + DebuggerEvalOption debuggerEvalOption = FOLLOW_DEBUGGER_EVAL_PREV_LINK) + : FrameIter(cx, cxOption, savedOption, debuggerEvalOption) { settle(); } @@ -1728,8 +1733,9 @@ class ScriptFrameIter : public FrameIter ScriptFrameIter(JSContext* cx, ContextOption cxOption, SavedOption savedOption, + DebuggerEvalOption debuggerEvalOption, JSPrincipals* prin) - : FrameIter(cx, cxOption, savedOption, prin) + : FrameIter(cx, cxOption, savedOption, debuggerEvalOption, prin) { settle(); } @@ -1770,8 +1776,10 @@ class NonBuiltinFrameIter : public FrameIter NonBuiltinFrameIter(JSContext* cx, FrameIter::ContextOption contextOption, - FrameIter::SavedOption savedOption) - : FrameIter(cx, contextOption, savedOption) + FrameIter::SavedOption savedOption, + FrameIter::DebuggerEvalOption debuggerEvalOption = + FrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK) + : FrameIter(cx, contextOption, savedOption, debuggerEvalOption) { settle(); } @@ -1779,8 +1787,16 @@ class NonBuiltinFrameIter : public FrameIter NonBuiltinFrameIter(JSContext* cx, FrameIter::ContextOption contextOption, FrameIter::SavedOption savedOption, + FrameIter::DebuggerEvalOption debuggerEvalOption, JSPrincipals* principals) - : FrameIter(cx, contextOption, savedOption, principals) + : FrameIter(cx, contextOption, savedOption, debuggerEvalOption, principals) + { + settle(); + } + + NonBuiltinFrameIter(JSContext* cx, JSPrincipals* principals) + : FrameIter(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED, + FrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK, principals) { settle(); } @@ -1812,8 +1828,10 @@ class NonBuiltinScriptFrameIter : public ScriptFrameIter NonBuiltinScriptFrameIter(JSContext* cx, ScriptFrameIter::ContextOption contextOption, - ScriptFrameIter::SavedOption savedOption) - : ScriptFrameIter(cx, contextOption, savedOption) + ScriptFrameIter::SavedOption savedOption, + ScriptFrameIter::DebuggerEvalOption debuggerEvalOption = + ScriptFrameIter::FOLLOW_DEBUGGER_EVAL_PREV_LINK) + : ScriptFrameIter(cx, contextOption, savedOption, debuggerEvalOption) { settle(); } @@ -1821,8 +1839,9 @@ class NonBuiltinScriptFrameIter : public ScriptFrameIter NonBuiltinScriptFrameIter(JSContext* cx, ScriptFrameIter::ContextOption contextOption, ScriptFrameIter::SavedOption savedOption, + ScriptFrameIter::DebuggerEvalOption debuggerEvalOption, JSPrincipals* principals) - : ScriptFrameIter(cx, contextOption, savedOption, principals) + : ScriptFrameIter(cx, contextOption, savedOption, debuggerEvalOption, principals) { settle(); } @@ -1846,7 +1865,8 @@ class AllFramesIter : public ScriptFrameIter { public: explicit AllFramesIter(JSContext* cx) - : ScriptFrameIter(cx, ScriptFrameIter::ALL_CONTEXTS, ScriptFrameIter::GO_THROUGH_SAVED) + : ScriptFrameIter(cx, ScriptFrameIter::ALL_CONTEXTS, ScriptFrameIter::GO_THROUGH_SAVED, + ScriptFrameIter::IGNORE_DEBUGGER_EVAL_PREV_LINK) {} }; diff --git a/js/xpconnect/tests/chrome/test_xrayToJS.xul b/js/xpconnect/tests/chrome/test_xrayToJS.xul index 2241a2204e..dda7df3830 100644 --- a/js/xpconnect/tests/chrome/test_xrayToJS.xul +++ b/js/xpconnect/tests/chrome/test_xrayToJS.xul @@ -565,13 +565,13 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=933681 is(e[name], undefined, name + " property censored after suspicious replacement"); } testProperty('message', x => x == 'some message', 'some other message', 42); - testProperty('fileName', x => /xul/.test(x), 'otherFilename.html', new iwin.Object()); + testProperty('fileName', x => x == '', 'otherFilename.html', new iwin.Object()); // Note - an Exception newed via Xrays is going to have an empty stack given the // current semantics and implementation. This tests the current behavior, but that // may change in bug 1036527 or similar. testProperty('stack', x => /^\s*$/.test(x), 'some other stack', new iwin.WeakMap()); - testProperty('columnNumber', x => x > 5 && x < 100, 99, 99.5); - testProperty('lineNumber', x => x > 100 && x < 10000, 50, 'foo'); + testProperty('columnNumber', x => x == 1, 99, 99.5); + testProperty('lineNumber', x => x == 0, 50, 'foo'); } }