859892 - WebGL data-view-test.html crash [@ js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

https://www.khronos.org/registry/webgl/sdk/tests/conformance/typedarrays/data-view-test.html

crashes on load in the 20130408 windows 32-bit nightly.  All crashes are at:

https://crash-stats.mozilla.com/report/index/bp-f1a443a7-070a-4211-bed2-8690c2130409

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Might be a dup of bug 706438

Comment 2

[Benjamin Smedberg]

Near-null but in the type system so still potentially scary. See prior art bug 736830 (marked DUP of a fixed bug) and bug 706438.

Comment 3

[Andrew McCreight [:mccr8]]

Looks like this involves BaselineCompiler and DataViews.

Comment 4

[Daniel Veditz [:dveditz]]

Unlikely to be related to bug 706438, that's a pretty common function and there must be zillions of ways to screw up the code it's checking. Similarly not all crashes in js_Interpret are the same thing.

Comment 5

[Andrew McCreight [:mccr8]]

Vlad, can you still reproduce this?  Works for me in OSX.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yup -- just crashed on 20130612 nightly.  https://crash-stats.mozilla.com/report/index/bp-200d3d86-9f26-49b3-8efd-f8fc22130613

Comment 7

[Jan de Mooij [:jandem]]

Vlad, can you still reproduce this?

It doesn't crash on OS X, I also tried a 20130612 nightly. I wonder if this is Windows only somehow.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Very much still crashes.  Win32, 20131011 nightly: https://crash-stats.mozilla.com/report/index/7952f90d-8544-4109-aa64-d98bf2131015

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Different signature now though -- matches bug 901333 now.

Comment 10

[Jan de Mooij [:jandem]]

I can reproduce the crash on Windows 7. Investigating.

Comment 11

[Jan de Mooij [:jandem]]

I expected this to be a JIT bug, but it seems to be a DataView bug. Minimal testcase:

<script>
function f() {
    var arrayBuffer = (new Uint8Array([255, 255, 255, 127])).buffer;
    var view = new DataView(arrayBuffer, 0, arrayBuffer.byteLength);
    document.write(view.getFloat32(0));
}
f();
</script>

Report: https://crash-stats.mozilla.com/report/index/af128a85-ae66-4b1b-9c39-9fb2a2131015

It looks like we're not canonicalizing the NaN value correctly and interpreting it as a Value. DataViewObject::getFloat32Impl correctly calls JS_CANONICALIZE_NAN though, so maybe it's an MSVC bug? 

I also can't get this to crash in a Windows shell I downloaded. Will try a debug build now.

Comment 12

[Jeff Walden [:Waldo]]

static inline double
JS_CANONICALIZE_NAN(double d)
{
    if (MOZ_UNLIKELY(d != d)) {
        jsval_layout l;
        l.asBits = 0x7FF8000000000000LL;
        return l.asDouble;
    }
    return d;
}

It's entirely possible |d != d| should be |mozilla::DoubleIsNaN(d)|.  We've had problems with multiple compilers in the past checking for NaN-ness in pretty much any way but using that method, no matter what *should* happen for that expression.  There's a reasonable chance that'll fix this, sadly.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm. PGO bug maybe? I don't think the shell gets PGO'd..

Comment 14

[Andrew McCreight [:mccr8]]

You could try a nightly debug build, or we may even have opt non-PGO builds sitting around somewhere.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> Hmm. PGO bug maybe? I don't think the shell gets PGO'd..

Yeah I just wanted to post that: I downloaded a Win32 opt build with PGO and one without and it seems PGO only... I will do Try builds with some changes and see what works.

This means PGO is introducing random sec-critical bugs, that most of our non-Windows, shell-only fuzzers won't find :(

Comment 16

[Jan de Mooij [:jandem]]

PGO Try build, renames JS_CANONICALIZE_NAN to JS::CanonicalizeNaN and implements it using mozilla::IsNaN and JS::GenericNaN:

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

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> Hmm. PGO bug maybe? I don't think the shell gets PGO'd..

The shell does not get PGOed. (We could do that, but unless we were linking js.exe against mozjs.dll it wouldn't be using the same optimized binary anyway.)

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Jan de Mooij [:jandem] from comment #15)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #13)
> > Hmm. PGO bug maybe? I don't think the shell gets PGO'd..
> 
> Yeah I just wanted to post that: I downloaded a Win32 opt build with PGO and
> one without and it seems PGO only... I will do Try builds with some changes
> and see what works.
> 
> This means PGO is introducing random sec-critical bugs, that most of our
> non-Windows, shell-only fuzzers won't find :(

As a vocal opponent of PGO, I'd like to say that this would be far from the first incident. PGO has introduced both top crashes and sg:crit bugs in many past releases.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to David Anderson [:dvander] from comment #18)
> As a vocal opponent of PGO, I'd like to say that this would be far from the
> first incident. PGO has introduced both top crashes and sg:crit bugs in many
> past releases.

Yeah I wonder if we should start disabling MSVC PGO for most of js/src as a start (with pragmas or something better). I've had to work around multiple MSVC PGO bugs, they're always time consuming to track down and can be introduced by random changes. I know others had the same experience :(

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #16)
> PGO Try build, renames JS_CANONICALIZE_NAN to JS::CanonicalizeNaN and
> implements it using mozilla::IsNaN and JS::GenericNaN:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=acdd62310585

This build no longer crashes. I made some small changes and will put the patch up for review if this one is also stable:

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

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to Jan de Mooij [:jandem] from comment #19)
> Yeah I wonder if we should start disabling MSVC PGO for most of js/src as a
> start (with pragmas or something better). I've had to work around multiple
> MSVC PGO bugs, they're always time consuming to track down and can be
> introduced by random changes. I know others had the same experience :(

Unfortunately, I think js is where most of the PGO performance wins have come from.

Comment 23

[Jeff Walden [:Waldo]]

That it was possible to eyeball the code (given a reduced testcase) and identify the issue, to me, suggests that it's somewhat inapposite to blame PGO.  In cases where it's a random failure of a kind we've not seen before, and had to debug from scratch, okay.  But this was an easily identified pattern that we knew was failure-prone before, and which we *should* be able to catch in reviews and such.

I'd bet comparing any float/double to itself is something we could easily use the static-analysis builds to flag.  I filed bug 927430 to add this to the clang static-analysis plugin.

Comment 24

[Jeff Walden [:Waldo]]

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

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

Could be worth a cover bug for this change, to hide that it's a security issue, even after sec-approval and all that.  Presumably the PGO'd assembly is easy enough to figure out how to subvert here, that extra caution is warranted.

::: js/src/vm/StructuredClone.cpp
@@ +433,5 @@
>          double d;
>      } pun;
>      if (!read(&pun.u))
>          return false;
> +    *p = CanonicalizeNaN(pun.d);

Hm, let's hope this works.

::: js/src/vm/TypedArrayObject.cpp
@@ +53,5 @@
>  
>  using mozilla::IsNaN;
>  using mozilla::PodCopy;
>  using JS::GenericNaN;
> +using JS::CanonicalizeNaN;

C goes before G.

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> Could be worth a cover bug for this change, to hide that it's a security
> issue, even after sec-approval and all that. 

I can make it look like a standard refactoring/renaming, but how do we handle branch patches in that case? Or should the cover bug be for a (top-)crash or something?

Comment 26

[Jeff Walden [:Waldo]]

No good way to handle the branch patch case, other than to coordinate with drivers and land as late in the cycle as possible.  :-\  Or, actually, you *could* fake it with a totally bogus topcrash, yeah.  But it's perhaps a bunch of work to make it look convincing, it's dubiously ethical (even if in pursuit of a good end), and I wouldn't be willing to do it if it were me.

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Fixes the nit. I don't think a cover bug is much help if we have to land on branches anyway, but I will let the security team decide.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not easy but it's possible to guess what's going on for somebody who knows what he's doing.

> 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?
Release, beta, aurora, esr24. Windows-only so does not affect b2g branches.

> If not all supported branches, which bug introduced the flaw?
Unknown.

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

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

Comment 28

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

Comment on attachment 818040 [details] [diff] [review]
Patch v2

sec-approval+.

Please create and nominate a patch for Aurora, Beta, and ESR24.

Comment 29

[(no longer active)]

You can always sneak in the fix in a merge commit -- just land a simple test fix or something on somewhere non-tip, do hg merge, and before hg commit-ing, sneak in your changes.  :-)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Certain sheriffs have been known to be amenable to doing such things too when pinged. Though this is a lot more complicated since it involves branch uplifts as well.

My vote is for just waiting until the latest point in the cycle possible before landing.

Comment 31

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

Ryan, the issue here is that if we're taking it on Beta, we want to get it in a little-b beta or two.

Comment 32

[Jan de Mooij [:jandem]]

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

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 818040 [details] [diff] [review]
Patch v2

sec-approval+ from abillings in comment 28 but forgot to set the flag.

Comment 34

[Jan de Mooij [:jandem]]

Attachment: Patch for Aurora

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown

> User impact if declined: 
Security bugs, crashes on Windows.

> Testing completed (on m-c, etc.):
On m-i.

> Risk to taking this patch (and alternatives if risky): 
Low risk.

> String or IDL/UUID changes made by this patch:
None.

Comment 35

[Jan de Mooij [:jandem]]

Attachment: Patch for Beta

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown.

> User impact if declined: 
Security bugs and crashes on Windows.

> Testing completed (on m-c, etc.):
On m-i.

> Risk to taking this patch (and alternatives if risky): 
Low risk.

> String or IDL/UUID changes made by this patch:
None.

Comment 36

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR24

[Approval Request Comment]
> User impact if declined: 
Security bugs and crashes on Windows.

> Fix Landed on Version:
Will be backported to beta (25).

> Risk to taking this patch (and alternatives if risky): 
Very low risk.

> String or UUID changes made by this patch: 
None.

Comment 37

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #35)
> [Approval Request Comment]
> > Bug caused by (feature/regressing bug #):
> Unknown.

This probably dates back to https://hg.mozilla.org/integration/mozilla-inbound/rev/cb31ec3a3325 which would be bug 584168 which is approximately the dawn of time.  Whether MSVC miscompilation happens at every instant between then and now is, of course, unknown.

Comment 38

[Alex Keybl [:akeybl]]

Comment on attachment 818332 [details] [diff] [review]
Patch for Beta

Well want to land this on beta at the same time as m-c, given this is our second to last today

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0f3b4a99461
https://hg.mozilla.org/releases/mozilla-beta/rev/d5d12f7343d6
https://hg.mozilla.org/releases/mozilla-esr24/rev/8f61daab1964

Comment 40

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/f3f9a19a57b9

Comment 42

[Jeff Walden [:Waldo]]

sunfish, the issue here is one it seems worth making you aware of, given bug 910814 comment 4 and bug 910814 comment 9.

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in FF24esr branch, 2013-10-15. 
Verified fixed in FF25 release.

Comment 45

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed in FF24.1.1esr 2013-11-20.
Verified fixed in FF26, FF27 2013-11-21.