Crash [@ js::HeapPtr<js::BaseShape, unsigned int>::operator] with use-after-free and findReferences

RESOLVED FIXED in Firefox 15

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: jimb)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
mozilla15
x86
Linux
crash, testcase
Points:
---

Firefox Tracking Flags

(firefox13 wontfix, firefox14 affected, firefox15 fixed)

Details

(Whiteboard: js-triage-needed [fuzzblocker][qa-], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 623004 [details]
Test case for shell (see README file inside).

The attached test crashes on mozilla-central revision 4c6d01c92dcc (options -m -n -a):


GDB Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0805aad4 in js::HeapPtr<js::BaseShape, unsigned int>::operator js::BaseShape* (this=0xdadadada) at ../../gc/Barrier.h:212
212         operator T*() const { return value; }
(gdb) bt 8
#0  0x0805aad4 in js::HeapPtr<js::BaseShape, unsigned int>::operator js::BaseShape* (this=0xdadadada) at ../../gc/Barrier.h:212
#1  0x0804d24d in js::Shape::base (this=0xdadadada) at ../../jsscope.h:708
#2  0x0804d18f in js::Shape::getObjectClass (this=0xdadadada) at ../../jsscope.h:607
#3  0x0804e269 in js::ObjectImpl::getClass (this=0xf750c8b0) at ../../vm/ObjectImpl-inl.h:245
#4  0x0804e27d in js::ObjectImpl::hasClass (this=0xf750c8b0, c=0x85dacc0) at ../../vm/ObjectImpl-inl.h:257
#5  0x0805fd67 in JSObject::isBlock (this=0xf750c8b0) at ../../jsobjinlines.h:789
#6  0x08060835 in ReferenceFinder::representable (this=0xffffbad0, cell=0xf750c8b0, kind=0) at /srv/repos/mozilla-central/js/src/shell/jsheaptools.cpp:409
#7  0x08060a2c in ReferenceFinder::visit (this=0xffffbad0, cell=0xf750c8b0, path=0xffffb8f0) at /srv/repos/mozilla-central/js/src/shell/jsheaptools.cpp:446
(More stack frames follow...)
(gdb) x /i $pc
=> 0x805aad4 <js::HeapPtr<js::BaseShape, unsigned int>::operator js::BaseShape*() const+6>:     mov    (%eax),%eax
(gdb) info reg eax
eax            0xdadadada       -623191334
(gdb) 


The trace indicates a use-after-free so I assume this could be s-s. The test uses findReferences but I guess could as well be an underlying bug revealed by that debug function.
Assignee: general → wmccloskey
Could you take a look at this, Jim? It's crashing with while reading from a GCed value during findReferences.
Assignee: wmccloskey → general
(Assignee)

Comment 2

5 years ago
Reproduced!
(Assignee)

Comment 3

5 years ago
I've figured out what's going on. findReferences operates in two phases:

- First, we traverse the heap, and build our own map, containing every object and listing its incoming edges. This is careful to allocate no heap objects, and will not cause GC.

- Second, we build the results object, by traversing that map. This involves adding properties (which allocates shapes), and building arrays. GC could certainly happen in this phase.

The bug is that findReferences assumes that, as long as it roots, itself, all things that were referred to by roots in the first phase, then none of the objects it discovers will become garbage during the second phase. This is untrue; the engine is free to rearrange any edges it likes at any time, as long as it won't affect the behavior of the program. (Of course, findReferences can observe these rearrangements, but that's because it's a diagnostic tool.)

findReferences really needs to root *every* object it finds, itself, so that it can hold onto them long enough to put them in the results object.
(Assignee)

Comment 4

5 years ago
In this specific case, our reversed heap map found some template objects used by JM, referred to via:

script->jitHandleNormal->value->chunkDescriptor(0).chunk->rootedTemplates()[0]

The process of constructing the findReferences results object caused two GC's. In the first one, script's jitScripts were disposed of, making the template object unreachable. In the second GC, the template object was swept, and became invalid.
(Assignee)

Comment 5

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=283a066a06b1
(Assignee)

Comment 6

5 years ago
Created attachment 625290 [details] [diff] [review]
Root *all* the objects we encounter while reversing the heap.
Assignee: general → jimb
Status: NEW → ASSIGNED
Attachment #625290 - Flags: review?(wmccloskey)
Comment on attachment 625290 [details] [diff] [review]
Root *all* the objects we encounter while reversing the heap.

Review of attachment 625290 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Jim.

::: js/src/shell/jsheaptools.cpp
@@ +272,3 @@
>  
>  bool
>  HeapReverser::traverseEdge(void *cell, JSGCTraceKind kind) {

Could you fix the brace here to be on its own line?

@@ +308,3 @@
>  
>  bool
>  HeapReverser::reverseHeap() {

And here too?
Attachment #625290 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 710688
(Reporter)

Comment 9

5 years ago
Can we land this? It is causing quite a few different signatures, making it hard to distinguish this bug from more important ones.
Whiteboard: js-triage-needed → js-triage-needed [fuzzblocker]
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea95691cd679
(Assignee)

Comment 11

5 years ago
(In reply to Christian Holler (:decoder) from comment #9)
> Can we land this? It is causing quite a few different signatures, making it
> hard to distinguish this bug from more important ones.

Landed, but I have a question:

In a case like this, where we strongly suspect that the bug is in non-browser code (findReferences), is there some reason you can't simply drop findReferences from the fuzzer's vocabulary, and continue on fuzzing other stuff?

That is, I didn't really prioritize 710688 because it wasn't easy for me to reproduce, nor easy to debug, and it seemed like it shouldn't cause you much trouble. But if there's some aspect of fuzzing that I don't understand that means that you can't just drop findReferences, then that was a mistake on my part.
(Assignee)

Comment 12

5 years ago
For what it's worth, when there's an r+'ed patch, I completely agree that the right thing is to ping the assignee, and not tweak the fuzzer.
https://hg.mozilla.org/mozilla-central/rev/ea95691cd679
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Keywords: sec-critical
Given bug 710688 being a dupe and the problem possibly predating that I'm going to assume we need this patch on the ESR-10 branch. We definitely need it for Firefox 14 so please request aurora approval.
status-firefox-esr10: --- → affected
status-firefox13: --- → wontfix
status-firefox14: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox14: --- → +
Christian, can you verify that this is fixed on trunk?
This is JS shell only, if that matters for where it needs to be fixed...
(Reporter)

Comment 17

5 years ago
According to comment 3 this is a bug in findReferences. If that is the case, then this bug would not be sec-critical and shell-only as Andrew pointed out already.

@jimb: Can you confirm that, or am I wrong?
status-firefox-esr10: affected → ---
tracking-firefox-esr10: ? → ---
tracking-firefox14: + → ---
Keywords: sec-critical
Group: core-security
(Assignee)

Comment 18

5 years ago
This doesn't need to be fixed on ESR or Firefox 14. It's just a bug in some diagnostic code we don't ship. (That wasn't clear initially, but now that we've fixed it, we are sure.)
Keywords: verifyme
Got a Reference error when attempting to run the test case in a js shell build out of both the revision in comment 0 and current beta: part2.js:16: ReferenceError: reportCompare is not defined

I'assuming I'm doing something wrong or can't reproduce. So if anybody else can verify this or provide input on where I'm making mistakes, would appreciate it.
(Reporter)

Comment 20

5 years ago
There's no need to verify this test as it's debug-only diagnostic code :)
No verifyme needed here, then. Thanks!
Keywords: verifyme
(In reply to Virgil Dicu [:virgil] [QA] from comment #21)
> No verifyme needed here, then. Thanks!

Thanks Virgil and Christian. Adding [qa-] so this doesn't show up in my untriaged query.
Whiteboard: js-triage-needed [fuzzblocker] → js-triage-needed [fuzzblocker][qa-]
You need to log in before you can comment on or make changes to this bug.