Last Comment Bug 754146 - Crash [@ js::HeapPtr<js::BaseShape, unsigned int>::operator] with use-after-free and findReferences
: Crash [@ js::HeapPtr<js::BaseShape, unsigned int>::operator] with use-after-f...
js-triage-needed [fuzzblocker][qa-]
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla15
Assigned To: Jim Blandy :jimb
: Jason Orendorff [:jorendorff]
: 710688 (view as bug list)
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2012-05-10 18:37 PDT by Christian Holler (:decoder)
Modified: 2012-08-17 09:36 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test case for shell (see README file inside). (954 bytes, application/x-gtar-compressed)
2012-05-10 18:37 PDT, Christian Holler (:decoder)
no flags Details
Root *all* the objects we encounter while reversing the heap. (3.85 KB, patch)
2012-05-18 15:47 PDT, Jim Blandy :jimb
wmccloskey: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-10 18:37:24 PDT
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

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.
Comment 1 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-11 22:05:38 PDT
Could you take a look at this, Jim? It's crashing with while reading from a GCed value during findReferences.
Comment 2 Jim Blandy :jimb 2012-05-16 12:44:54 PDT
Comment 3 Jim Blandy :jimb 2012-05-18 15:19:50 PDT
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.
Comment 4 Jim Blandy :jimb 2012-05-18 15:28:27 PDT
In this specific case, our reversed heap map found some template objects used by JM, referred to via:


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.
Comment 5 Jim Blandy :jimb 2012-05-18 15:46:48 PDT
Comment 6 Jim Blandy :jimb 2012-05-18 15:47:48 PDT
Created attachment 625290 [details] [diff] [review]
Root *all* the objects we encounter while reversing the heap.
Comment 7 [PTO to Dec5] Bill McCloskey (:billm) 2012-05-18 16:00:25 PDT
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?
Comment 8 Jim Blandy :jimb 2012-05-18 16:39:51 PDT
*** Bug 710688 has been marked as a duplicate of this bug. ***
Comment 9 Christian Holler (:decoder) 2012-05-22 05:47:54 PDT
Can we land this? It is causing quite a few different signatures, making it hard to distinguish this bug from more important ones.
Comment 11 Jim Blandy :jimb 2012-05-22 13:29:50 PDT
(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.
Comment 12 Jim Blandy :jimb 2012-05-22 14:02:05 PDT
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.
Comment 13 Ed Morley [:emorley] 2012-05-23 05:22:29 PDT
Comment 14 Daniel Veditz [:dveditz] 2012-05-24 16:49:55 PDT
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.
Comment 15 Al Billings [:abillings] 2012-06-08 17:18:31 PDT
Christian, can you verify that this is fixed on trunk?
Comment 16 Andrew McCreight [:mccr8] 2012-06-08 17:25:07 PDT
This is JS shell only, if that matters for where it needs to be fixed...
Comment 17 Christian Holler (:decoder) 2012-06-08 17:44:28 PDT
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?
Comment 18 Jim Blandy :jimb 2012-08-09 12:50:23 PDT
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.)
Comment 19 Virgil Dicu [:virgil] [QA] 2012-08-17 08:48:25 PDT
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.
Comment 20 Christian Holler (:decoder) 2012-08-17 09:02:09 PDT
There's no need to verify this test as it's debug-only diagnostic code :)
Comment 21 Virgil Dicu [:virgil] [QA] 2012-08-17 09:05:07 PDT
No verifyme needed here, then. Thanks!
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-17 09:36:48 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.