989299 - GenerationalGC: Crash [@ js::gc::BarrieredCell<js::ObjectImpl>::zone] with TypedObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision 6fa163ff81a3 (run with --fuzzing-safe):


var {StructType, uint32, Object, Any, storage, objectType} = TypedObject;
var Uints = new StructType(({ "" : Object, } ));
var anArray = new Uint32Array(2);
anArray[0] = 22;
var uints = new Uints(anArray.buffer);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Marked s-s and affected for 31 because ggc is landed already on mozilla-inbound.

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::gc::BarrieredCell<js::ObjectImpl>::zone (this=0x16) at js/src/vm/ObjectImpl.h:918
918         JS::Zone *zone = obj->shape_->zone();
#0  js::gc::BarrieredCell<js::ObjectImpl>::zone (this=0x16) at js/src/vm/ObjectImpl.h:918
#1  0x08155a2b in CheckMarkedThing<JSObject> (thing=<optimized out>, trc=0xffffcac0) at js/src/gc/Marking.cpp:147
#2  MarkInternal<JSObject> (trc=0xffffcac0, thingp=0xf653e770) at js/src/gc/Marking.cpp:192
#3  0x080c56a9 in visitReference (mem=0xf653e770 "\026", repr=<optimized out>, this=0xffffc9ac) at js/src/builtin/TypeRepresentation.cpp:791
#4  visitReferences<js::MemoryTracingVisitor> (repr=<optimized out>, mem=0xf653e770 "\026", visitor=...) at js/src/builtin/TypeRepresentation.cpp:656
#5  0x080c5726 in visitReferences<js::MemoryTracingVisitor> (repr=0x9530688, mem=0xf653e770 "\026", visitor=...) at js/src/builtin/TypeRepresentation.cpp:680
#6  0x080e0dea in traceInstance (length=1, mem=0xf653e770 "\026", trace=0xffffcac0, this=0x9530688) at js/src/builtin/TypeRepresentation.cpp:816
#7  js::TypedObject::obj_trace (trace=0xffffcac0, object=(JSObject *) 0xf653e830 [object Handle]) at js/src/builtin/TypedObject.cpp:1608
eax     0x16    22
eip     0x80b47ad <js::gc::BarrieredCell<js::ObjectImpl>::zone() const+29>
=> 0x80b47ad <js::gc::BarrieredCell<js::ObjectImpl>::zone() const+29>:  mov    (%eax),%esi

Comment 3

[Jon Coppeard (:jonco)]

It looks like the testcase in the description creates a typed object from the raw memory in an arraybuffer, resulting in a JSObject pointer with the value 22.  Is this meant to be possible?  It looks totally unsafe as is.

Comment 4

[Christian Holler (:decoder)]

I didn't even spot that at first sight. That means the JSObject pointer is controllable and this is sec-crit. Thanks for pointing that out.

Comment 5

[Niko Matsakis [:nmatsakis]]

Yeah, that's not supposed to be possible because the StructType is opaque. Crazy oversight on my part, but easy to fix -- note that typed objects are (I believe) nightly only.

Comment 6

[Niko Matsakis [:nmatsakis]]

Attachment: Bug989299.diff

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8398589 [details] [diff] [review]
Bug989299.diff

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

Ouch.

Can a TypedObject change between unsized and sized, or be otherwise mutated? I don't see a way for it to cause trouble here, but if you were to switch to using ToInt32 or something on the args in the future, I was wondering if the callee might end up getting modified.

Comment 8

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #7)
> Can a TypedObject change between unsized and sized, or be otherwise mutated?
> I don't see a way for it to cause trouble here, but if you were to switch to
> using ToInt32 or something on the args in the future, I was wondering if the
> callee might end up getting modified.

No, the only thing that could happen is neutering. In particular, if a typed object is opaque, it can never be made non-opaque. (Given a non-opaque typed object, you can create a new, equivalent typed object that IS opaque, but you can never (a) modify the opacity of an existing typed object nor (b) create a non-opaque from an opaque.) I do plan to refactor that code to use ToInt32 and at that time i'll be careful to make that conversion happen *before* the safety checks.

Comment 9

[Niko Matsakis [:nmatsakis]]

https://tbpl.mozilla.org/?tree=Try&rev=a82e651444cf

Comment 10

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c55c3d39c82c

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c55c3d39c82c

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx31