Closed Bug 976530 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox29 --- disabled
firefox30 --- verified
firefox-esr24 --- unaffected

People

(Reporter: decoder, Assigned: nmatsakis)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(3 files, 1 obsolete file)

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();
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)
Whiteboard: [jsbugmon:update,bisect]
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.
decoder -- sounds right. We try to guard against this sort of thing...
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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
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.
Attached patch Bug976530.diff (obsolete) — Splinter Review
Attachment #8381834 - Flags: review?(sphink)
Attached file bug976530.js
Include test case
Attachment #8381834 - Attachment is obsolete: true
Attachment #8381834 - Flags: review?(sphink)
Attachment #8381835 - Flags: review?(sphink)
That said, here is a fix, it also adds some useful assertions to array buffer and a test.
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.
Argh. My mistake.
Attached patch Bug976530.diffSplinter Review
Here is the patch. It doesn't include the test case for some reason, but you already saw that ;)
Attachment #8385330 - Flags: review?(sphink)
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
Attachment #8385330 - Flags: review?(sphink) → review+
Attachment #8381835 - Flags: review?(sphink) → review+
Group: javascript-core-security
Assignee: nobody → nmatsakis
Try run (green, as far as I can tell): https://tbpl.mozilla.org/?tree=Try&rev=a6d2715798c8
https://hg.mozilla.org/mozilla-central/rev/bb5bd88acaeb
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security
JSBugMon: This bug has been automatically verified fixed on Fx30
Group: core-security
You need to log in before you can comment on or make changes to this bug.