Closed
Bug 1447996
Opened 6 years ago
Closed 6 years ago
Assertion failure: !inUnsafeRegion ([AutoAssertNoGC] possible GC in GC-unsafe region), at js/src/vm/JSContext.h:591
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | fixed |
People
(Reporter: decoder, Assigned: jandem)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update])
Attachments
(1 file)
3.32 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 7771df14ea18+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2): var global = this; var ReflectApply = global.Reflect.apply; var RegExpPrototypeExec = global.RegExp.prototype.exec; var StringPrototypeIndexOf = global.String.prototype.indexOf; function StringSplit(str, delimiter) { var last = 0; var i = ReflectApply(StringPrototypeIndexOf, str, [delimiter, last]); } function printStatus(msg) { var lines = StringSplit(msg, "\n"); } function reportMatch(expectedRegExp, actual, description) { var matches = ReflectApply(RegExpPrototypeExec, expectedRegExp, [actual]) !== null; } var actual = 'No Crash'; test(); function test() { printStatus(actual); expect = /TypeError: .+ is not a function/; try { test("{ 1; "); } catch (ex) {} reportMatch(expect, actual) } Backtrace: received signal SIGSEGV, Segmentation fault. 0x0000000000ed526a in JSContext::verifyIsSafeToGC (this=0x7ffff5f16000) at js/src/vm/JSContext.h:591 #0 0x0000000000ed526a in JSContext::verifyIsSafeToGC (this=0x7ffff5f16000) at js/src/vm/JSContext.h:591 #1 js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffff5f16000, kind=<optimized out>) at js/src/gc/Allocator.cpp:291 #2 0x0000000000ed5555 in js::AllocateString<JSFatInlineString, (js::AllowGC)1> (cx=0x7ffff5f16000, heap=heap@entry=js::gc::DefaultHeap) at js/src/gc/Allocator.cpp:179 #3 0x0000000000c4bd81 in js::Allocate<JSFatInlineString, (js::AllowGC)1> (heap=js::gc::DefaultHeap, cx=<optimized out>) at js/src/gc/Allocator.h:57 #4 JSFatInlineString::new_<(js::AllowGC)1> (cx=<optimized out>) at js/src/vm/StringType-inl.h:283 #5 js::AllocateInlineString<(js::AllowGC)1, unsigned char> (cx=<optimized out>, len=18, chars=0x7fffffdfd840) at js/src/vm/StringType-inl.h:38 #6 0x0000000000c79ae1 in js::NewInlineString<(js::AllowGC)1, unsigned char> (cx=0x7ffff5f16000, chars=...) at js/src/vm/StringType-inl.h:57 #7 js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char> (cx=0x7ffff5f16000, s=<optimized out>, n=<optimized out>) at js/src/vm/StringType.cpp:1534 #8 0x0000000000c79e8b in js::NewStringCopyUTF8N<(js::AllowGC)1> (cx=cx@entry=0x7ffff5f16000, utf8=...) at js/src/vm/StringType.cpp:1605 #9 0x00000000009c788b in js::NewStringCopyUTF8Z<(js::AllowGC)1> (utf8=..., cx=0x7ffff5f16000) at js/src/vm/StringType.h:1499 #10 JS_NewStringCopyUTF8Z (cx=0x7ffff5f16000, s=...) at js/src/jsapi.cpp:5778 #11 0x00000000009d9103 in js::ErrorToException (cx=0x7ffff5f16000, reportp=0x7fffffdfdb30, callback=<optimized out>, userRef=<optimized out>) at js/src/jsexn.cpp:678 #12 0x0000000000b3ccd3 in js::ReportErrorNumberVA (cx=cx@entry=0x7ffff5f16000, flags=flags@entry=0, callback=callback@entry=0xb26df0 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=116, argumentsType=argumentsType@entry=js::ArgumentsAreASCII, ap=0x7fffffdfdc10) at js/src/vm/JSContext.cpp:903 #13 0x00000000009c10a8 in JS_ReportErrorNumberASCIIVA (cx=0x7ffff5f16000, errorCallback=0xb26df0 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=116, ap=ap@entry=0x7fffffdfdc10) at js/src/jsapi.cpp:6440 #14 0x00000000009c1158 in JS_ReportErrorNumberASCII (cx=cx@entry=0x7ffff5f16000, errorCallback=errorCallback@entry=0xb26df0 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=116) at js/src/jsapi.cpp:6429 #15 0x0000000000b3a876 in js::ReportOverRecursed (maybecx=0x7ffff5f16000, errorNumber=116) at js/src/vm/JSContext.cpp:402 #16 0x00000000004d56b8 in js::CheckRecursionLimit (cx=0x7ffff5f16000) at js/src/jsfriendapi.h:1098 #17 0x0000000000f26a80 in js::irregexp::RegExpCompiler::CheckOverRecursed (this=<optimized out>) at js/src/irregexp/RegExpEngine.cpp:2540 #18 js::irregexp::BoyerMooreLookahead::CheckOverRecursed (this=0x7ffff559a850) at js/src/irregexp/RegExpEngine.cpp:2551 #19 js::irregexp::ActionNode::FillInBMInfo (this=0x7ffff559a668, offset=0, budget=200, bm=0x7ffff559a850, not_at_start=<optimized out>) at js/src/irregexp/RegExpEngine.cpp:633 #20 0x0000000000f4237a in js::irregexp::ChoiceNode::Emit (this=0x7ffff559a6e0, compiler=0x7fffffdfe470, trace=0x7fffffdfe240) at js/src/irregexp/RegExpEngine.cpp:4362 #21 0x0000000000f38f6c in js::irregexp::RegExpCompiler::Assemble (this=this@entry=0x7fffffdfe470, cx=cx@entry=0x7ffff5f16000, assembler=<optimized out>, start=start@entry=0x7ffff559a6e0, capture_count=<optimized out>) at js/src/irregexp/RegExpEngine.cpp:1684 #22 0x0000000000f401bc in js::irregexp::CompilePattern (cx=cx@entry=0x7ffff5f16000, shared=..., shared@entry=..., data=data@entry=0x7fffffdff690, sample=..., sample@entry=..., is_global=is_global@entry=false, ignore_case=<optimized out>, is_latin1=true, match_only=false, force_bytecode=false, sticky=false, unicode=false, tables=...) at js/src/irregexp/RegExpEngine.cpp:1852 #23 0x0000000000be9548 in js::RegExpShared::compile (cx=0x7ffff5f16000, re=re@entry=..., pattern=..., pattern@entry=..., input=..., input@entry=..., mode=mode@entry=js::RegExpShared::Normal, force=force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:1029 #24 0x0000000000bea022 in js::RegExpShared::compile (cx=0x7ffff5f16000, re=..., input=..., mode=js::RegExpShared::Normal, force=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:994 #25 0x0000000000bea1b2 in js::RegExpShared::compileIfNecessary (cx=cx@entry=0x7ffff5f16000, re=..., re@entry=..., input=..., input@entry=..., mode=mode@entry=js::RegExpShared::Normal, force=force@entry=js::RegExpShared::DontForceByteCode) at js/src/vm/RegExpObject.cpp:1062 #26 0x0000000000bf0c0a in js::RegExpShared::execute (cx=cx@entry=0x7ffff5f16000, re=..., re@entry=..., input=..., input@entry=..., start=start@entry=0, matches=matches@entry=0x7fffffe00020, endIndex=endIndex@entry=0x0) at js/src/vm/RegExpObject.cpp:1076 #27 0x00000000004e8539 in ExecuteRegExpImpl (cx=cx@entry=0x7ffff5f16000, res=0x7ffff542db80, re=re@entry=..., input=input@entry=..., searchIndex=0, matches=matches@entry=0x7fffffe00020, endIndex=0x0) at js/src/builtin/RegExp.cpp:125 #28 0x00000000004e89d9 in ExecuteRegExp (cx=cx@entry=0x7ffff5f16000, regexp=regexp@entry=..., string=..., string@entry=..., lastIndex=<optimized out>, matches=matches@entry=0x7fffffe00020, endIndex=endIndex@entry=0x0) at js/src/builtin/RegExp.cpp:963 #29 0x00000000004ea165 in RegExpMatcherImpl (rval=..., lastIndex=<optimized out>, string=..., regexp=..., cx=0x7ffff5f16000) at js/src/builtin/RegExp.cpp:984 #30 js::RegExpMatcher (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/RegExp.cpp:1018 #31 0x0000302fd2893791 in ?? () #32 0x00007fffffe00150 in ?? () #33 0x00007fffffe00100 in ?? () #34 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x7ffff5f16000 140737319624704 rcx 0x7ffff6c282ad 140737333330605 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffdfd7d0 140737486247888 rsp 0x7fffffdfd7b0 140737486247856 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4780 140737354024832 r10 0x58 88 r11 0x7ffff6b9e7a0 140737332766624 r12 0x6 6 r13 0x1 1 r14 0x12 18 r15 0x7fffffdfda50 140737486248528 rip 0xed526a <js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind)+250> => 0xed526a <js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind)+250>: movl $0x0,0x0 0xed5275 <js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1>(JSContext*, js::gc::AllocKind)+261>: ud2 Marking s-s because this assert is about an unsafe GC.
Assignee | ||
Comment 1•6 years ago
|
||
This might be from bug 1447578. Steve, can you help me figure out why the static analysis didn't catch this? :)
Assignee | ||
Comment 2•6 years ago
|
||
Oh we have a Maybe<NativeRegExpMacroAssembler> native_assembler; And that contains our MacroAssembler, so that probably explains it. I can put an explicit JS::AutoCheckCannotGC on top of this, see if that reports the hazard. I'm not sure if it's a real hazard - when we hit overrecursion we call SetRegExpTooBig(). I'll look into it more; I'm not sure if we check that flag also after we Assemble()...
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > I'm not sure if we check that flag also after we Assemble()... Oh we do, we just don't call the getter for it: https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/js/src/irregexp/RegExpEngine.cpp#1695 So I think we're good here security-wise, we just need to check for overrecursion without reporting an exception. I'll check if that's the only thing here that can GC.
Updated•6 years ago
|
Priority: -- → P1
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 4•6 years ago
|
||
JSBugMon: Bisection requested, failed due to error (try manually).
Assignee | ||
Comment 5•6 years ago
|
||
This adds an explicit AutoCheckCannotGC because the static analysis doesn't understand that Maybe<NativeRegExpMacroAssembler> contains one (via StackMacroAssembler). The rest of the patch is dealing with some issues the analysis reported.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8961498 -
Flags: review?(sphink)
Assignee | ||
Comment 6•6 years ago
|
||
. The only places that can GC are calls that will abort regex compilation anyway.
Keywords: sec-high
Comment 7•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) > Oh we have a > > Maybe<NativeRegExpMacroAssembler> native_assembler; > > And that contains our MacroAssembler, so that probably explains it. I can > put an explicit JS::AutoCheckCannotGC on top of this, see if that reports > the hazard. Yeah, the Maybe<> blocks it. And I don't want the analysis to consider Maybe<T> to be a T, because it is often used precisely to limit the region of influence of an RAII guard, eg Maybe<AutoSuppressGC> or whatever. I suppose I could try to get fancy and say that any part of the graph that is reachable by an emplace() should see through Maybe<>, but I don't know if it's worth the effort. (And what if the GC in question always has an intervening reset()?) Filed bug 1448131.
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
tracking-firefox61:
--- → +
Comment 8•6 years ago
|
||
Comment on attachment 8961498 [details] [diff] [review] Patch Review of attachment 8961498 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/irregexp/RegExpEngine.cpp @@ +1693,5 @@ > return RegExpCode(); > > if (reg_exp_too_big_) { > code.destroy(); > + js::gc::AutoSuppressGC suppress(cx); Ugh. This really shouldn't be necessary, since there's nothing held live across it that matters. AutoSuppressGCAnalysis would be better, but that's not going to work either (because it asserts if we GC.) So yeah, this seems like the best of our available options. I mean, you could use struct IgnoreForAnalysis {} JS_HAZ_GC_SUPPRESSED ignore; instead, but I prefer what you have.
Attachment #8961498 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #8) > So yeah, this seems like the best of our available options. I mean, you > could use > > struct IgnoreForAnalysis {} JS_HAZ_GC_SUPPRESSED ignore; > > instead, but I prefer what you have. I don't think this would work, because we're still in an AutoCheckCannotGC scope (two of them in fact) which also does runtime checks, so it's not just the static analysis we're trying to appease.
Comment 11•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a682f7ab2f5 Don't GC when hitting overrecursion in RegExpCompiler; make the static analysis detect this. r=sfink
Comment 12•6 years ago
|
||
FYI: OSS-Fuzz also found this crash (about 3 hours before Christian filed this bug, but for whatever reason it took a day for their automation to report the finding :-)).
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a682f7ab2f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•