1412420 - Crash [@ js::TypeSet::GetValueType] with invalid read

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision a80d568a417e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

function f(j) {
    var i = Math.pow(j++, j) | 0;
    var obj = {
        i: i,
    }
}
var max = 150;
for (var j = 0; j <= max; ++j) {
    f(j);
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::TypeSet::GetValueType (val=...) at js/src/vm/TypeInference-inl.h:182
#0  js::TypeSet::GetValueType (val=...) at js/src/vm/TypeInference-inl.h:182
#1  js::AddTypePropertyId (cx=cx@entry=0x7ffff6952000, group=0x7ffff467f8b0, obj=0x0, id=..., value=...) at js/src/vm/TypeInference.cpp:2850
#2  0x000000000062133c in js::jit::DoTypeUpdateFallback (cx=0x7ffff6952000, frame=<optimized out>, stub=0x7ffff432f3c8, objval=..., value=...) at js/src/jit/BaselineIC.cpp:329
#3  0x00000289dd30058e in ?? ()
#4  0x00007fffffffd5e0 in ?? ()
#5  0x00007fffffffd4f8 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x7fffffffffff	140737488355327
rbx	0x7ffff6952000	140737330356224
rcx	0x7ffff4600d60	140737293323616
rdx	0x0	0
rsi	0x7ffff467f8b0	140737293842608
rdi	0x7ffff6952000	140737330356224
rbp	0x7fffffffd420	140737488344096
rsp	0x7fffffffd3f0	140737488344048
r8	0x40ffe8000000	71467853152256
r9	0xfff80000ffffffff	-2251795518717953
r10	0x7fffffffd518	140737488344344
r11	0x7ffff4674bc8	140737293798344
r12	0x7fffffffd440	140737488344128
r13	0x7fffffffd530	140737488344368
r14	0x7ffff432f3c8	140737290367944
r15	0x7fffffffd480	140737488344192
rip	0xc856ed <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+125>
=> 0xc856ed <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+125>:	mov    (%r8),%rax
   0xc856f0 <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+128>:	or     $0x1,%r8


I was only able to reproduce this on the GCOV builds we made for JS shell coverage analysis. The difference to our normal builds is that the GCOV builds use gcc 6. So this could be a compiler issue.

Marking s-s and sec-high because this looks like an invalid read from a bad address.

Comment 1

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Wennie]

Hi Naveed, please reassigned these bugs to developers who can investigate and fix them. Thanks!

Comment 3

[Jan de Mooij [:jandem]]

Interesting. This reminds me of some crashes that have been reported on unsupported ARM configurations.

How can we repro this locally?

Comment 4

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #3)
>
> How can we repro this locally?

I've used this compiler to build, so maybe try that as a start:

gcc-6 (Ubuntu/Linaro 6.3.0-18ubuntu2~16.04) 6.3.0 20170519


Also, the builds we use for coverage are here:

https://build.fuzzing.mozilla.org/builds/jsshell-mc-64-opt-gcov.zip
https://build.fuzzing.mozilla.org/builds/jsshell-mc-64-debug-gcov.zip

I only tried the opt build here because that is what we currently use in testing gcov.

Comment 5

[Jan de Mooij [:jandem]]

I can reproduce this with GCC 6.4 when I add the --coverage flag to CC and CXX.

Our Baseline IC for JSOP_BITOR calls JS::ToInt32(double) and this GCC build does *not* zero the upper half of the return register (it contains garbage). Then we use masm.orPtr and masm.boxValue => we now have a corrupt Value.

A few things to do here:

* We should use masm.or32 et al instead of orPtr.

* We have storeCallWordResult and storeCallBoolResult - we probably need storeCallInt32Result.

Patch next week.

Comment 6

[Jan de Mooij [:jandem]]

Oh and this is very build specific. I doubt it affects our official Tier-1 builds (we would have heard by now).

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Fixes described in comment 5.

We now use masm.or32/xor32/and32 instead of orPtr/xorPtr/andPtr and I split storeCallWordResult into storeCallInt32Result and storeCallPointerResult.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8925532 [details] [diff] [review]
Patch

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

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ +878,5 @@
>          if (!unicode) {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareStrings;
>              masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, fun));
>          } else {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareUCStrings;

follow-up nit: change the return type to int32_t.

::: js/src/jit/CodeGenerator.cpp
@@ +10682,5 @@
>      masm.passABIArg(obj);
>      masm.movePtr(ImmPtr(gen->runtime), output);
>      masm.passABIArg(output);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, jit::TypeOfObject));
> +    masm.storeCallPointerResult(output);

nit: static_assert(sizeof(JSType) == sizeof(void*));

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +726,5 @@
>  
>      inline void generate(CodeGeneratorShared* codegen) const {
> +        // It's okay to use storePointerResultTo here - the VMFunction wrapper
> +        // ensures the upper bytes are zero for bool/int32 return values.
> +        codegen->storePointerResultTo(out_);

Thanks for this comment.

Sean, can you double check if this comment is accurate on arm64?
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/js/src/jit/arm64/Trampoline-arm64.cpp#719,724

Comment 9

[Christian Holler (:decoder)]

There's a considerable amount of crashes with this signature in Crash Stats. Could this be triggered on Windows somehow or is this strictly an issue with GCC?

Comment 10

[Sean Stangl [:sstangl]]

The comment does hold on ARM64: LDRB zero-extends, and LDR (really, any instruction) with a W-reg always zeroes the upper 32 bits of the X-reg so as to enable better micro-optimization.

Comment 11

[Mike Taylor [:miketaylr]]

Jan, is this something we can land in 59 and uplift to 58? Unsure of the risk of this patch.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #9)
> There's a considerable amount of crashes with this signature in Crash Stats.
> Could this be triggered on Windows somehow or is this strictly an issue with
> GCC?

In theory we could trigger this on Windows/MSVC, but the crashes with this signature look unrelated (ObjectValue with null payload or something).

Not sure what to do here since we only see this with GCOV builds. I'll just request sec-approval.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, jit::TypeOfObject));
> > +    masm.storeCallPointerResult(output);
> 
> nit: static_assert(sizeof(JSType) == sizeof(void*));

It's confusing but js::TypeOfObject returns JSType, jit::TypeOfObject returns JSString*. We call the latter here so this should be okay.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Official builds are most likely not affected. Requesting sec-approval just in  case... I think we should land this on beta and Nightly.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All, in theory (see above).

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be pretty easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

I'm confused... 99% of the crashes with this signature are Windows (mostly with wildptr addresses).  How would this be a GCOV issue?

Comment 16

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ to land on trunk.

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Randell Jesup [:jesup] from comment #15)
> I'm confused... 99% of the crashes with this signature are Windows (mostly
> with wildptr addresses).  How would this be a GCOV issue?

JS crashes often have a very generic signature. Note that this bug is 64-bit only and a lot of these crashes are on 32-bit, so they're definitely unrelated. That said, I thought the 64-bit crashes were unrelated because of the location where we crash in the function, but after thinking about it more, this bug could trigger similar crashes.

Still okay to land this, Al?

Comment 18

[Mike Taylor [:miketaylr]]

Naveed, can you ask someone to look at the Windows crashes here?

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jan de Mooij [:jandem] from comment #17)

> Still okay to land this, Al?

Yes.

Comment 20

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26d3df5b58fb60facb9057dc1e86576127c9e1f5

Comment 21

[Jan de Mooij [:jandem]]

Ehm this landed:

https://hg.mozilla.org/mozilla-central/rev/26d3df5b58fb60facb9057dc1e86576127c9e1f5

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8933331 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Potential crashes or security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Very mechanical/simple changes.
[String changes made/needed]: None.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8933331 [details] [diff] [review]
Patch

Sec-high, Beta58+

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Jan de Mooij [:jandem] from comment #21)
> Ehm this landed:
> 
> https://hg.mozilla.org/mozilla-central/rev/
> 26d3df5b58fb60facb9057dc1e86576127c9e1f5
Also: https://hg.mozilla.org/mozilla-central/rev/0b42ea549d67

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b4d18da6b10

Comment 26

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for ESR52.

Comment 27

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/c2945f1249eb768d14675e813f325907d46aa73f