The following testcase crashes on mozilla-central revision 32c6c1848f14 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe): let m = parseModule(` function f(x,y,z) { delete arguments[2]; import.meta[2] } f(1,2,3) `); m.declarationInstantiation(); m.evaluation(); Backtrace: received signal SIGSEGV, Segmentation fault. #0 JS::Value::setObject (obj=..., this=<optimized out>) at js/Value.h:490 #1 JS::ObjectValue (obj=...) at js/Value.h:1074 #2 js::jit::IonBuilder::jsop_importmeta (this=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:13183 #3 0x0000000000754996 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffffffc460, op=op@entry=JSOP_IMPORTMETA) at js/src/jit/IonBuilder.cpp:2393 #4 0x00000000007559d7 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7fffffffc460, cfgblock=cfgblock@entry=0x7ffff4af9070, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1572 #5 0x0000000000755cc2 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:1489 #6 0x0000000000756450 in js::jit::IonBuilder::build (this=this@entry=0x7fffffffc460) at js/src/jit/IonBuilder.cpp:864 #7 0x000000000075ed5f in js::jit::AnalyzeArgumentsUsage (cx=0x7ffff5f15000, scriptArg=<optimized out>) at js/src/jit/IonAnalysis.cpp:4496 #8 0x0000000000592b5c in JSScript::ensureHasAnalyzedArgsUsage (cx=<optimized out>, this=<optimized out>) at js/src/vm/JSScript-inl.h:203 #9 Interpret (cx=0x7ffff5f15000, state=...) at js/src/vm/Interpreter.cpp:3554 [...] #28 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9669 rax 0x1ce8020 30310432 rbx 0x7fffffffc460 140737488340064 rcx 0xf07ab8 15760056 rdx 0x0 0 rsi 0x5 5 rdi 0x7ffff4d7c1a0 140737301168544 rbp 0x7fffffffc460 140737488340064 rsp 0x7fffffffc0c0 140737488339136 r8 0xfff8800000000002 -2111062325329918 r9 0x0 0 r10 0x1 1 r11 0x100 256 r12 0xf67920 16152864 r13 0xe8 232 r14 0x7ffff4af8020 140737298530336 r15 0x7ffff5f15000 140737319620608 rip 0x7222e1 <js::jit::IonBuilder::jsop_importmeta()+145> => 0x7222e1 <js::jit::IonBuilder::jsop_importmeta()+145>: movl $0x0,0x0 0x7222ec <js::jit::IonBuilder::jsop_importmeta()+156>: ud2

This is an automated crash issue comment: Summary: Crash [@ js::gc::detail::GetCellLocation] Build version: mozilla-central revision 32c6c1848f14 Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing Runtime options: --fuzzing-safe Testcase can be provided if necessary. Backtrace: ==21844==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000fffe8 (pc 0x00000103c837 bp 0x7ffd16911b50 sp 0x7ffd16911b40 T0) ==21844==The signal is caused by a READ memory access. #1 0x103c836 in js::gc::IsInsideNursery(js::gc::Cell const*) dist/include/js/HeapAPI.h:486 #2 0x103c836 in js::jit::IonBuilder::checkNurseryObject(JSObject*) js/src/jit/IonBuilder.cpp:13657 #3 0x103c836 in js::jit::IonBuilder::constant(JS::Value const&) js/src/jit/IonBuilder.cpp:13676 #4 0x1045538 in js::jit::IonBuilder::pushConstant(JS::Value const&) js/src/jit/IonBuilder.cpp:3255:19 #5 0x1045538 in js::jit::IonBuilder::jsop_importmeta() js/src/jit/IonBuilder.cpp:13183 #6 0x1045538 in js::jit::IonBuilder::inspectOpcode(JSOp) js/src/jit/IonBuilder.cpp:2393 #7 0x103e3df in js::jit::IonBuilder::visitBlock(js::jit::CFGBlock const*, js::jit::MBasicBlock*) js/src/jit/IonBuilder.cpp:1572:9 #8 0x1034352 in js::jit::IonBuilder::traverseBytecode() js/src/jit/IonBuilder.cpp:1489:9 #9 0x102314e in js::jit::IonBuilder::build() js/src/jit/IonBuilder.cpp:864:5 #10 0x1024825 in js::jit::AnalyzeArgumentsUsage(JSContext*, JSScript*) js/src/jit/IonAnalysis.cpp:4496:45 #11 0x9da1fd in JSScript::ensureHasAnalyzedArgsUsage(JSContext*) js/src/vm/JSScript-inl.h:203:12 #12 0x9da1fd in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3554 #13 0x9ca8b8 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:429:12 #14 0xa02da8 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:777:15 #15 0xa03542 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:809:12 #16 0xb0a638 in js::ModuleObject::execute(JSContext*, JS::Handle<js::ModuleObject*>, JS::MutableHandle<JS::Value>) js/src/builtin/ModuleObject.cpp:1105:12 #17 0x1cb8517 in intrinsic_ExecuteModule(JSContext*, unsigned int, JS::Value*) js/src/vm/SelfHosting.cpp:2221:12 #18 0x9fd0d6 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/vm/Interpreter.cpp:449:15 #19 0x9fd0d6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:537 #20 0x9e5fbf in js::CallFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:594:12 #21 0x9e5fbf in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3243 #22 0x9ca8b8 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:429:12 #23 0x9fdbee in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:561:15 #24 0xda1bfa in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:2582:14 #25 0x35ceb30ec8e0 (<unknown module>) Marking s-s because this can also crash in GC with non-null crash address.

I wrongly assumed that when IonBuilder saw bytecode it had already been compiled by Baseline. The safe and easy thing to do to fix this is to check whether the meta object is null and abort. I'm not sure whether the correct thing to do is to try and create the object here or not. Can this analysis can run off-thread?

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/847453f52aab user: Jon Coppeard date: Wed May 23 08:47:28 2018 +0100 summary: Bug 1427610 - Support import.meta in the JITs r=jandem This iteration took 245.065 seconds to run.

How do we reconcile the patch, which is basically a null check, with the non-null ASAN crash in comment 1?

(In reply to Daniel Veditz [:dveditz] from comment #4) > How do we reconcile the patch, which is basically a null check, with the > non-null ASAN crash in comment 1? js::gc::IsInsideNursery() is trying to access the chunk trailer for the chunk containing the pointer. This is at offset 0xfffe8, hence this happens when it's passed a null pointer: ==21844==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000fffe8 (pc 0x00000103c837 bp 0x7ffd16911b50 sp 0x7ffd16911b40 T0) ==21844==The signal is caused by a READ memory access. #1 0x103c836 in js::gc::s(js::gc::Cell const*) dist/include/js/HeapAPI.h:486

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 49b70f7e6817).

Comment on attachment 9003501 [details] [diff] [review] bug1485698-import-meta Review of attachment 9003501 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/IonBuilder.cpp @@ +13180,5 @@ > + > + // The meta object might not have been created yet if we are in an analysis > + // phase. > + if (!metaObject) > + return abort(AbortReason::Error); Instead of aborting (I'm not sure how AbortReason::Error is treated by this analysis) you could do something like this at the start of the function to proceed without the object: https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/jit/IonBuilder.cpp#9458-9465

(In reply to Jon Coppeard (:jonco) from comment #2) > Can this analysis can run off-thread? It always runs on the main thread, but the arguments analysis only cares about how |arguments| is used so anything unrelated to that can just push a dummy value/constant as it will be ignored anyway.

Updated patch with review feedback.

Comment on attachment 9003775 [details] [diff] [review] bug1485698-import-meta v2 [Security approval request comment] How easily could an exploit be constructed based on the patch? This is a null pointer dereference so I'm not sure if it's exploitable, but you can see how to do it fairly easily from the patch. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? As above, you can see from the patch how to get a null pointer dereference. Which older supported branches are affected by this flaw? FF 62. If not all supported branches, which bug introduced the flaw? Bug 1427610. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply cleanly. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions since it's such a localised change.

We're about to build 62 RC1 and I don't think this is the kind of change we should be taking in a late RC (but feel free to disagree if you feel strongly otherwise).

This isn't going to get sec-approval until after 62 ships. It is too late in the cycle since we're building the release candidate and are out of betas. We also need to figure out a security rating.

Note: null-deref's I normally don't consider sec bugs at all, if they're only null-pointer derefs (and the fffe8 offset is a nullptr deref). I would land this in 63 or in 64-and-uplift-to-63, and I'd unhide the bug. IMHO

(In reply to Al Billings [:abillings] from comment #12) Since this is a null dereference and given comment 13 I'd at least like to land this on 63. What do you think?

(In reply to Jon Coppeard (:jonco) from comment #14) > (In reply to Al Billings [:abillings] from comment #12) > Since this is a null dereference and given comment 13 I'd at least like to > land this on 63. What do you think? I think that's fine. I hadn't looked at it in detail before. I'll clear the sec-approval.

JSBugMon: This bug has been automatically verified fixed.