Closed Bug 1472638 Opened 3 years ago Closed 3 years ago

Crash [@ __memset_sse2] or Hit MOZ_CRASH(Tried to access a protected region!) with OOM


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed
firefox63 --- fixed


(Reporter: decoder, Assigned: nbp)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [jsbugmon:update])

Crash Data


(1 file)

The following testcase crashes on mozilla-central revision 23885c14f025 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

oomTest(function() { eval(`
  var BIG_INDEX = 4294967290;
  var arr = Array(BIG_INDEX);
  arr[BIG_INDEX - 1] = 'a';
  arr.length = BIG_INDEX - 5000;
  status = inSection(1);
`); });


received signal SIGSEGV, Segmentation fault.
__memset_sse2 () at ../sysdeps/x86_64/multiarch/../memset.S:65
#0  __memset_sse2 () at ../sysdeps/x86_64/multiarch/../memset.S:65
#1  0x0000000000536f7b in memset (__len=40, __ch=206, __dest=<optimized out>) at /usr/include/x86_64-linux-gnu/bits/string3.h:90
#2  js::detail::BumpChunk::setBump (this=this@entry=0x7fffec3e2000, newBump=newBump@entry=0x7fffec3e3018 '\315' <repeats 199 times>, <incomplete sequence \315>...) at js/src/ds/LifoAlloc.h:335
#3  0x00000000005591d9 in js::detail::BumpChunk::tryAlloc (n=40, this=0x7fffec3e2000) at js/src/ds/LifoAlloc.h:461
#4  js::LifoAlloc::allocImpl (n=40, this=0x7ffff49fd578) at js/src/ds/LifoAlloc.h:590
#5  js::LifoAlloc::alloc (this=0x7ffff49fd578, n=40) at js/src/ds/LifoAlloc.h:663
#6  0x0000000000d3a6ed in js::LifoAlloc::new_<js::ObjectGroup::Property, jsid&> (this=<optimized out>) at js/src/ds/LifoAlloc.h:887
#7  js::ObjectGroup::getProperty (this=this@entry=0x7ffff2b60a90, sweep=..., cx=cx@entry=0x7ffff5f17000, obj=obj@entry=0x7ffff4d36a30, id=id@entry=...) at js/src/vm/TypeInference-inl.h:1142
#8  0x0000000000d04fd3 in js::ObjectGroup::markPropertyNonData (this=0x7ffff2b60a90, cx=cx@entry=0x7ffff5f17000, obj=obj@entry=0x7ffff4d36a30, id=..., id@entry=...) at js/src/vm/TypeInference.cpp:2885
#9  0x00000000004db4ef in js::MarkTypePropertyNonData (id=..., obj=0x7ffff4d36a30, cx=0x7ffff5f17000) at js/src/vm/TypeInference-inl.h:471
#10 js::DeleteProperty (result=..., id=..., obj=..., cx=0x7ffff5f17000) at js/src/vm/JSObject-inl.h:256
#11 js::DeleteElement (cx=0x7ffff5f17000, obj=..., obj@entry=..., index=index@entry=4294967237, result=...) at js/src/vm/JSObject-inl.h:268
#12 0x00000000004c812e in js::ArraySetLength (cx=<optimized out>, cx@entry=0x7ffff5f17000, arr=arr@entry=..., id=..., id@entry=..., attrs=attrs@entry=4, value=..., value@entry=..., result=...) at js/src/builtin/Array.cpp:834
#13 0x00000000004c906b in array_length_setter (cx=cx@entry=0x7ffff5f17000, obj=..., id=..., v=..., result=...) at js/src/builtin/Array.cpp:645
#14 0x0000000000c168c9 in js::CallJSSetterOp (result=..., v=..., id=..., obj=..., op=<optimized out>, cx=0x7ffff5f17000) at js/src/vm/JSContext-inl.h:301
#15 NativeSetExistingDataProperty (result=..., v=..., shape=..., obj=..., cx=0x7ffff5f17000) at js/src/vm/NativeObject.cpp:2535
#16 SetExistingProperty (cx=<optimized out>, id=..., id@entry=..., v=v@entry=..., receiver=..., receiver@entry=..., pobj=..., pobj@entry=..., prop=..., prop@entry=..., result=...) at js/src/vm/NativeObject.cpp:2745
#17 0x0000000000c3a5d5 in js::NativeSetProperty<(js::QualifiedBool)1> (cx=<optimized out>, cx@entry=0x7ffff5f17000, obj=..., id=id@entry=..., v=..., v@entry=..., receiver=..., receiver@entry=..., result=...) at js/src/vm/NativeObject.cpp:2788
#18 0x00000000005b786c in js::SetProperty (cx=0x7ffff5f17000, obj=..., id=..., v=..., receiver=..., result=...) at js/src/vm/NativeObject.h:1705
#19 0x00000000005a345e in SetPropertyOperation (rval=..., id=..., lval=..., op=<optimized out>, cx=0x7ffff5f17000) at js/src/vm/Interpreter.cpp:268
#20 Interpret (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:2990
#21 0x00000000005ae5f6 in js::RunScript (cx=0x7ffff5f17000, state=...) at js/src/vm/Interpreter.cpp:423
#22 0x00000000005b195d in js::ExecuteKernel (cx=<optimized out>, cx@entry=0x7ffff5f17000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=<optimized out>) at js/src/vm/Interpreter.cpp:771
#23 0x00000000005ea3b0 in EvalKernel (cx=0x7ffff5f17000, v=..., v@entry=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., env=env@entry=..., pc=<optimized out>, vp=...) at js/src/builtin/Eval.cpp:319
#24 0x00000000005eabbe in js::DirectEval (cx=<optimized out>, v=..., vp=...) at js/src/builtin/Eval.cpp:427
#25 0x000000000069a683 in js::jit::DoCallFallback (cx=<optimized out>, frame=0x7fffffffbf28, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffbed8, res=...) at js/src/jit/BaselineIC.cpp:2641
#26 0x00002ade6844028c in ?? ()
#27 0x00007fffffffbea0 in ?? ()
#58 0x0000000000000000 in ?? ()
rax	0x7fffec3e2ff0	140737156886512
rbx	0x7fffec3e2000	140737156882432
rcx	0x7fffec3e2ff0	140737156886512
rdx	0x28	40
rsi	0xce	206
rdi	0x7fffec3e2ff0	140737156886512
rbp	0x7fffffffa240	140737488331328
rsp	0x7fffffffa158	140737488331096
r8	0x5dee488d	1575897229
r9	0x9e3779b8	2654435768
r10	0x0	0
r11	0x7ffff487b400	140737295922176
r12	0x0	0
r13	0x7fffec3e3018	140737156886552
r14	0x28	40
r15	0x7fffec3e3018	140737156886552
rip	0x7ffff6bc026c <__memset_sse2+44>
=> 0x7ffff6bc026c <__memset_sse2+44>:	movdqu %xmm0,-0x10(%rdi,%rdx,1)
   0x7ffff6bc0272 <__memset_sse2+50>:	ja     0x7ffff6bc0280 <__memset_sse2+64>

The crash looks like a stack space exhaustion but outside of GDB I also get the MOZ_CRASH, so I suspect this might be taking a different control flow route depending on how much stack space is available. Marking s-s until investigated further.
Flags: needinfo?(nicolas.b.pierron)
This definitely looks like what is being reported to us by crash-stat, i-e some write on the bottom of protected buffers during a LifoAlloc allocation. (see also Bug 1263794 comment 34)
Assignee: nobody → nicolas.b.pierron
Flags: needinfo?(nicolas.b.pierron)
This particular problem happens on OOM under getOrCreateChunk, when we cannot allocate a new chunk.  The first step of getOrCreateChunk is to flag the last chunk as reserved, but in case of OOM it will remain in the last chunk position.

In this particular case we attempt to allocate a 64k hash set, fail, and resume under ObjectGroup::markPropertyNonData as if nothing happend.  Thus the last chunk is being protected while remaining the last chunk.

This is an instrumentation bug and might not be the issue we are looking for. I will make a quick fix to handle this false-positive.

Thread 1 hit Breakpoint 3, js::detail::BumpChunk::setRWUntil (this=0xb90ce20,
    loc=js::detail::BumpChunk::Loc::Reserved) at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.cpp:76
76          if (!protect_)
(rr) bt
#0  js::detail::BumpChunk::setRWUntil (this=0xb90ce20, loc=js::detail::BumpChunk::Loc::Reserved)
    at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.cpp:76
#1  0x000000000148c254 in js::LifoAlloc::getOrCreateChunk (this=0x2fbe8c8, n=65544)
    at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.cpp:210
#2  0x00000000006120d2 in js::LifoAlloc::allocImpl (this=0x2fbe8c8, n=65544)
    at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.h:593
#3  0x000000000061204d in js::LifoAlloc::alloc (this=0x2fbe8c8, n=65544)
    at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.h:663
#4  0x000000000120feb8 in js::LifoAlloc::newArrayUninitialized<js::ObjectGroup::Property*> (this=0x2fbe8c8,
    count=8193) at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.h:763
#5  0x000000000120fabd in js::LifoAlloc::newArray<js::ObjectGroup::Property*> (this=0x2fbe8c8, count=8193)
    at /home/nicolas/mozilla/wksp-9/js/src/ds/LifoAlloc.h:753
#6  0x000000000120fced in js::TypeHashSet::InsertTry<jsid, js::ObjectGroup::Property, js::ObjectGroup::Property> (
    alloc=..., values=@0x7f61a0c68f08: 0xb8fae28, count=@0x7ffd252e571c: 2048, key=...)
    at /home/nicolas/mozilla/wksp-9/js/src/vm/TypeInference-inl.h:778
#7  0x0000000001201db5 in js::TypeHashSet::Insert<jsid, js::ObjectGroup::Property, js::ObjectGroup::Property> (
    alloc=..., values=@0x7f61a0c68f08: 0xb8fae28, count=@0x7ffd252e571c: 2048, key=...)
    at /home/nicolas/mozilla/wksp-9/js/src/vm/TypeInference-inl.h:852
#8  0x00000000011ff04a in js::ObjectGroup::getProperty (this=0x7f61a0c68ee0, sweep=..., cx=0x2f8a160,
    obj=0x7f61a0e38930, id=...) at /home/nicolas/mozilla/wksp-9/js/src/vm/TypeInference-inl.h:1149
#9  0x00000000011bad48 in js::ObjectGroup::markPropertyNonData (this=0x7f61a0c68ee0, cx=0x2f8a160,
    obj=0x7f61a0e38930, id=...) at /home/nicolas/mozilla/wksp-9/js/src/vm/TypeInference.cpp:2885
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Nicolas B. Pierron
date:        Wed Feb 14 17:12:00 2018 +0000
summary:     Bug 1437600 - Use mprotect to prevent mutations of inaccessible regions. r=luke

This iteration took 318.517 seconds to run.
The problem is that we were protecting the last chunk, even in case of some allocation failures that we recover from. Which should explain the cases reported on TI.

This patch adds a lambda to protect the last chunk. I did not used a ScopeExit because last() returns an iterator, and even if it would be safe in the current implementation, it would be unwise to alias it and use it after doing 
some append() to the same container.
Attachment #8989156 - Flags: review?(tcampbell)
Calling it sec-other because it sounds like a false-positive (comment 2). If I'm interpreting that correctly please unhide the bug and remove sec-other. Otherwise please rate appropriately
Blocks: 1437600
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-other
Comment on attachment 8989156 [details] [diff] [review]
Only protect the last chunk when we append a new chunk.

Review of attachment 8989156 [details] [diff] [review]:

Seems reasonable. Other approach would be to add your anonymous function to the BumpChunk class instead, but what you have may be simpler for now.
Attachment #8989156 - Flags: review?(tcampbell) → review+
Group: javascript-core-security
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-other
Pushed by
Only protect the last chunk when we append a new chunk. r=tcampbell
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8989156 [details] [diff] [review]
Only protect the last chunk when we append a new chunk.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1437600
[User impact if declined]: false-positive crash reports
[Is this code covered by automated tests?]: tested by fuzzers.
[Has the fix been verified in Nightly?]: yes. (removed ~1 crash per day from nightly)
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It removes wrong & extra memory protection from pages in case of OOM during a large allocation.
[String changes made/needed]: N/A
Attachment #8989156 - Flags: approval-mozilla-beta?
This is marked disabled for 62 but looks like it would still affect dev edition.
Comment on attachment 8989156 [details] [diff] [review]
Only protect the last chunk when we append a new chunk.

Fix for false positive asserts, had good results in nightly, so let's uplift for beta 8.
Attachment #8989156 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.