Last Comment Bug 859892 - WebGL data-view-test.html crash [@ js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) ]
: WebGL data-view-test.html crash [@ js::types::TypeMonitorResult(JSContext*, J...
Status: RESOLVED FIXED
[adv-main25+][adv-esr24-1+]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows 8
: -- critical (vote)
: mozilla27
Assigned To: Jan de Mooij [:jandem] (PTO until July 31)
:
Mentors:
: 889937 928028 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-09 10:16 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2015-02-25 20:16 PST (History)
21 users (show)
cbook: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
unaffected
25+
verified
unaffected
unaffected
fixed


Attachments
Patch (7.88 KB, patch)
2013-10-16 06:59 PDT, Jan de Mooij [:jandem] (PTO until July 31)
jwalden+bmo: review+
Details | Diff | Splinter Review
Patch v2 (7.88 KB, patch)
2013-10-16 13:36 PDT, Jan de Mooij [:jandem] (PTO until July 31)
jdemooij: review+
jdemooij: sec‑approval+
Details | Diff | Splinter Review
Patch for Aurora (8.05 KB, patch)
2013-10-17 01:38 PDT, Jan de Mooij [:jandem] (PTO until July 31)
jdemooij: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Patch for Beta (7.29 KB, patch)
2013-10-17 02:09 PDT, Jan de Mooij [:jandem] (PTO until July 31)
jdemooij: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Patch for ESR24 (7.36 KB, patch)
2013-10-17 02:50 PDT, Jan de Mooij [:jandem] (PTO until July 31)
jdemooij: review+
akeybl: approval‑mozilla‑esr24+
Details | Diff | Splinter Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2013-04-09 10:16:28 PDT
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] 2013-04-09 10:18:15 PDT
Might be a dup of bug 706438
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2013-04-09 17:08:41 PDT
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] 2013-04-17 10:08:48 PDT
Looks like this involves BaselineCompiler and DataViews.
Comment 4 Daniel Veditz [:dveditz] 2013-04-17 10:10:37 PDT
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] 2013-06-13 13:25:18 PDT
Vlad, can you still reproduce this?  Works for me in OSX.
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2013-06-13 16:18:23 PDT
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] (PTO until July 31) 2013-10-14 08:40:03 PDT
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] 2013-10-14 17:37:06 PDT
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] 2013-10-14 17:37:32 PDT
Different signature now though -- matches bug 901333 now.
Comment 10 Jan de Mooij [:jandem] (PTO until July 31) 2013-10-15 06:16:36 PDT
I can reproduce the crash on Windows 7. Investigating.
Comment 11 Jan de Mooij [:jandem] (PTO until July 31) 2013-10-15 08:07:51 PDT
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] (remove +bmo to email) 2013-10-15 08:40:04 PDT
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] 2013-10-15 08:59:55 PDT
Hmm. PGO bug maybe? I don't think the shell gets PGO'd..
Comment 14 Andrew McCreight [:mccr8] 2013-10-15 09:42:33 PDT
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] (PTO until July 31) 2013-10-15 09:45:12 PDT
(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] (PTO until July 31) 2013-10-15 10:45:20 PDT
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 Ted Mielczarek [:ted.mielczarek] 2013-10-15 11:32:33 PDT
(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] 2013-10-15 14:00:24 PDT
(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] (PTO until July 31) 2013-10-16 02:11:39 PDT
(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] (PTO until July 31) 2013-10-16 03:17:50 PDT
(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] (PTO until July 31) 2013-10-16 06:59:31 PDT
Created attachment 817824 [details] [diff] [review]
Patch
Comment 22 Vladimir Vukicevic [:vlad] [:vladv] 2013-10-16 07:49:41 PDT
(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] (remove +bmo to email) 2013-10-16 08:24:08 PDT
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] (remove +bmo to email) 2013-10-16 08:50:09 PDT
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] (PTO until July 31) 2013-10-16 10:26:42 PDT
(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] (remove +bmo to email) 2013-10-16 12:50:28 PDT
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] (PTO until July 31) 2013-10-16 13:36:05 PDT
Created attachment 818040 [details] [diff] [review]
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] 2013-10-16 13:47:27 PDT
Comment on attachment 818040 [details] [diff] [review]
Patch v2

sec-approval+.

Please create and nominate a patch for Aurora, Beta, and ESR24.
Comment 29 :Ehsan Akhgari (away Aug 1-5) 2013-10-16 14:02:51 PDT
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] 2013-10-16 14:07:02 PDT
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] 2013-10-16 15:29:26 PDT
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] (PTO until July 31) 2013-10-17 01:20:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f9a19a57b9
Comment 33 Jan de Mooij [:jandem] (PTO until July 31) 2013-10-17 01:36:28 PDT
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] (PTO until July 31) 2013-10-17 01:38:17 PDT
Created attachment 818322 [details] [diff] [review]
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] (PTO until July 31) 2013-10-17 02:09:52 PDT
Created attachment 818332 [details] [diff] [review]
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] (PTO until July 31) 2013-10-17 02:50:38 PDT
Created attachment 818354 [details] [diff] [review]
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] (remove +bmo to email) 2013-10-17 03:34:04 PDT
(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] 2013-10-17 06:48:27 PDT
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 40 Carsten Book [:Tomcat] 2013-10-18 03:39:18 PDT
landed on central https://hg.mozilla.org/mozilla-central/rev/f3f9a19a57b9
Comment 41 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-22 06:57:20 PDT
*** Bug 889937 has been marked as a duplicate of this bug. ***
Comment 42 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-22 09:00:48 PDT
sunfish, the issue here is one it seems worth making you aware of, given bug 910814 comment 4 and bug 910814 comment 9.
Comment 43 Jan de Mooij [:jandem] (PTO until July 31) 2013-10-23 11:14:28 PDT
*** Bug 928028 has been marked as a duplicate of this bug. ***
Comment 44 Matt Wobensmith [:mwobensmith][:matt:] 2013-11-21 17:35:59 PST
Confirmed crash in FF24esr branch, 2013-10-15. 
Verified fixed in FF25 release.
Comment 45 Matt Wobensmith [:mwobensmith][:matt:] 2013-11-22 13:22:02 PST
Verified fixed in FF24.1.1esr 2013-11-20.
Verified fixed in FF26, FF27 2013-11-21.

Note You need to log in before you can comment on or make changes to this bug.