976530 - Crash on Heap through [@ js::SizedTypeRepresentation::initInstance] with TypedObject

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision e3daaa4c73dd (run with --fuzzing-safe --ion-eager):


var Vec3u16Type = TypedObject.uint16.array((1073741823));
var PairVec3u16Type = new TypedObject.StructType({ fst: Vec3u16Type, snd: Vec3u16Type });
new PairVec3u16Type();

Comment 1

[Christian Holler (:decoder)]

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

Comment 2

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
0xf7df26e7 in ?? () from /lib/i386-linux-gnu/libc.so.6
#0  0xf7df26e7 in ?? () from /lib/i386-linux-gnu/libc.so.6
#1  0x080c870e in MemoryInitVisitor (rt=0x9455fc0, this=0xffffbf6c) at /usr/include/bits/string3.h:85
#2  js::SizedTypeRepresentation::initInstance (this=0x9507560, rt=0x9455fc0, mem=0x94e6e38 "", length=1) at  js/src/builtin/TypeRepresentation.cpp:743
#3  0x080d726b in js::TypedObject::createZeroed (cx=0x9462270, descr=..., length=0) at  js/src/builtin/TypedObject.cpp:1498
#4  0x080fea55 in js::TypedObject::constructSized (cx=0x9462270, argc=0, vp=0xffffc214) at  js/src/builtin/TypedObject.cpp:2329
#5  0x085c1aec in js::CallJSNative (cx=0x9462270, native=0x80fe870 <js::TypedObject::constructSized(JSContext*, unsigned int, JS::Value*)>, args=...) at  js/src/jscntxtinlines.h:230
#6  0x085c240b in js::CallJSNativeConstructor (cx=0x9462270, native=0x80fe870 <js::TypedObject::constructSized(JSContext*, unsigned int, JS::Value*)>, args=...) at  js/src/jscntxtinlines.h:263
#7  0x085b79f1 in js::InvokeConstructor (cx=0x9462270, args=...) at  js/src/vm/Interpreter.cpp:573
edx     0x954bfc0       156549056
=> 0xf7df26e7:  movdqa %xmm0,0x40(%edx)

Comment 3

[Christian Holler (:decoder)]

The number in comment 0 is 2^30 - 1. Subtracting one dword or increasing the value leads to either an OOM message or "Type is too large to allocate" so I suspect this is an improper bounds check. Marking sec-high for now.

Comment 4

[Niko Matsakis [:nmatsakis]]

decoder -- sounds right. We try to guard against this sort of thing...

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140220102129" and the hash "7a5cbe4dadf8".
The "bad" changeset has the timestamp "20140220102430" and the hash "cc73b1f7a47d".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7a5cbe4dadf8&tochange=cc73b1f7a47d

Comment 6

[Niko Matsakis [:nmatsakis]]

This bug is caused by using a uint32_t instead of an int32_t in the StructTypeRepresentation::create() routine. It is fixed by the (final two) patches in bug 966575.

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: Bug976530.diff

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: bug976530.js

Include test case

Comment 9

[Niko Matsakis [:nmatsakis]]

That said, here is a fix, it also adds some useful assertions to array buffer and a test.

Comment 10

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

Can you put that in the form of a patch? Right now, the attachment is a 3-line test file. You mentioned assertions, so I'm guessing this isn't what you meant to upload.

Comment 11

[Niko Matsakis [:nmatsakis]]

Argh. My mistake.

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug976530.diff

Here is the patch. It doesn't include the test case for some reason, but you already saw that ;)

Comment 13

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

Comment on attachment 8385330 [details] [diff] [review]
Bug976530.diff

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

I suspect typed objects should be using mfbt/CheckedInt.h all over, but r=me anyway.

Has Waldo or somebody written up anything about how to use it? I just looked at the header file, and it's a rough read.

::: js/src/builtin/TypeRepresentation.cpp
@@ +308,5 @@
>      fieldCount_ = names.length();
>  
>      // We compute alignment into the field `align_` directly in the
>      // loop below, but not `size_` because we have to very careful
> +    // about overflow. For now, we always use a int32_t for

*an int32_t

Comment 14

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc60397b3a1

Comment 15

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=08f69ec35b3b

Comment 16

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2645fa20fa25 for being in a push that broke jsreftests: https://tbpl.mozilla.org/php/getParsedLog.php?id=36020939&tree=Mozilla-Inbound

Comment 17

[Niko Matsakis [:nmatsakis]]

Try run (green, as far as I can tell): https://tbpl.mozilla.org/?tree=Try&rev=a6d2715798c8

Comment 18

[Niko Matsakis [:nmatsakis]]

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

Comment 19

[Phil Ringnalda (:philor)]

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

Comment 20

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 21

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx30