1248153 - Differential Testing: Different output message involving typed arrays

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    x = [];
    x[4] = 0;
    [, , , , , , , , , , , x, , , 0]
} catch (e) {}
for (v of [0]) {
    var y = new Float32Array(x);
}
y = new Uint32Array(y);
print(uneval(y));


$ ./js-64-dm-linux-d719ac4bcbec --fuzzing-safe --no-threads --ion-eager testcase.js
({0:0, 4:0, 1:0, 2:0, 3:0})

$ ./js-64-dm-linux-d719ac4bcbec --fuzzing-safe --no-threads --ion-eager --gc-zeal=2 testcase.js
({0:2147483648, 4:0, 1:2147483648, 2:2147483648, 3:2147483648})

Tested this on m-c rev d719ac4bcbec.

My configure flags are:

AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-more-deterministic" -r d719ac4bcbec

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4ec207bba727
user:        Lars T Hansen
date:        Wed Dec 16 12:01:57 2015 +0100
summary:     Bug 1229829 - make sameBuffer more discriminating. r=waldo

Lars, is bug 1229829 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I've tested this to reproduce on 64-bit opt deterministic builds on Linux, but does not seem to reproduce on Mac OS X.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Let's lock this down as s-s first, since typed arrays are involved.

Comment 3

[Lars T Hansen [:lth]]

Oh, foo!  :)

Comment 4

[Lars T Hansen [:lth]]

Able to repro locally on Linux 64-bit.  Will look into it further.

Comment 5

[Lars T Hansen [:lth]]

sameBuffer() is not the culprit, per se, but the change to sameBuffer() forces one of its callers to take a different path, and that path has a problem.

Specifically, we are in ElementSpecific<>::setFromTypedArray() in TypedArrayCommon.h, ca line 238, in the case for Float32.  The value is not converted from NaN to 0, but from NaN to INT_MIN.

This is looking a lot like an optimizer issue:  It's only reproducible in optimized builds, and if I pass the src pointer to an extern function inside the copy loop to create the impression of a possible side effect then the bug goes away.  Also, a simpler test case than yours exhibits the same problem with or without the gczeal argument, we just need to make sure we stay in interpreted code so that the C++ code is run.  (I'm guessing when the JIT runs we may get unboxed arrays here.)  Will attach that test case.

Gary, what C++ compiler are you using on Linux?  I appear to be on GCC 5.2.1, last updated from Ubuntu archives a few weeks ago probably.

There should be no need to mark this security-sensitive, so you can unhide it.

(Probably need to uplift any fix to 46 and 45.)

Comment 6

[Lars T Hansen [:lth]]

TC:

var x = [];
x[4] = 0;
for ( var i=0 ; i < x.length ; i++ )
    print(x[i]);
var y = new Float32Array(x);
for ( var i=0 ; i < y.length ; i++ )
    print(y[i]);
var zz = new Uint32Array(y);
for ( var i=0 ; i < zz.length ; i++ )
    print(zz[i]);

This should print (because NaN -> 0 when converted from float32 to int32 or uint32):

undefined
undefined
undefined
undefined
0
NaN
NaN
NaN
NaN
0
0
0
0
0
0

But when compiled as in comment #0 and run simply with --no-ion --no-baseline it prints

undefined
undefined
undefined
undefined
0
NaN
NaN
NaN
NaN
0
2147483648
2147483648
2147483648
2147483648
0

(Other than an optimizer feature we could be running into undefined conversion behavior in C++ here - investigating further.  But since adding an unknown call in the loop changes behavior I doubt that that's the full story.)

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Lars T Hansen [:lth] from comment #5)
> Specifically, we are in ElementSpecific<>::setFromTypedArray() in
> TypedArrayCommon.h, ca line 238, in the case for Float32.  The value is not
> converted from NaN to 0, but from NaN to INT_MIN.
> 
> This is looking a lot like an optimizer issue:  It's only reproducible in
> optimized builds

This isn't an optimizer issue, it's us misusing C++.  Per C++11 [conv.fpint]p1, "The behavior is undefined if the truncated value cannot be represented in the destination type."  Casting a NaN float/double to any integral type is UB.  Relying solely on how the compiler implements C++ casting here is Doing It Wrong.

I think we need to do something a bit more special for the float-to-integral conversion cases than just cast and hope.  (Integral-integral casting isn't great, either, but at least that's implementation-defined-land, not UB.)

Comment 8

[Lars T Hansen [:lth]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> 
> I think we need to do something a bit more special for the float-to-integral
> conversion cases than just cast and hope.  (Integral-integral casting isn't
> great, either, but at least that's implementation-defined-land, not UB.)

Funny.  Back in the day I expended a lot of effort to make sure all conversions in Opera went through libraries we controlled, because the C++ compilers and libraries were so buggy and unreliable.

Then... compilers improved and we could all just use the builtins to get "the right thing".

Then... compilers improved even more and now we go back to building our own conversions.

(Thanks for the feedback.)

Comment 9

[Lars T Hansen [:lth]]

Attachment: sketch: introduce well-defined conversions

This is just a sketch, but something like this perhaps?

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Lars T Hansen [:lth] from comment #5)
> Gary, what C++ compiler are you using on Linux?  I appear to be on GCC
> 5.2.1, last updated from Ubuntu archives a few weeks ago probably.

I think I'm also on GCC 5.2.1. (default on Ubuntu 15.10 Wily)

> There should be no need to mark this security-sensitive, so you can unhide
> it.

Is this still applicable after Waldo's analysis?

Comment 11

[Lars T Hansen [:lth]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> (In reply to Lars T Hansen [:lth] from comment #5)
> > There should be no need to mark this security-sensitive, so you can unhide
> > it.
> 
> Is this still applicable after Waldo's analysis?

I think so, the conversion does happen, it just happens to create a garbage value.  I would not expect (I know, I know, UB) anything larger to be the problem.  But if you're nervous, by all means keep it hidden.

Comment 12

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Jeff, how much of a security issue do you think this is? In the worst case, it could be bad but maybe it is just always going to generate some particular safe crash.

Comment 13

[Jeff Walden [:Waldo]]

It's undefined behavior, so there's not a great way to know.  Not without looking at what every compiler generates and working through the logic.  It could be that the compiler implicitly generates a MOZ_ASSUME_UNREACHABLE() in the asm to "handle" the cases that C++ doesn't require be handled, for example.  Or maybe it's just a garbage value, as comment 11 suggested.

But even if we (probably) get a "garbage" value out the other end, we don't know where that garbage value came from.  It's not wholly inconceivable that it might expose a value privately calculated somewhere else, as bits of some operation previously calculated using XMM registers or so.

This might not be sec-critical in practice, but there's not really a way for us to know, so I'd prefer if we kept it that locked down.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8719510 [details] [diff] [review]
sketch: introduce well-defined conversions

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

Something grungy and templaty does seem like approximately what you want, yes.  Not sure this is quite it, tho, because we already have stuff for this, somewhat.  See JS::detail::To{Ui,I}ntWidth in js/public/Conversions.h, except that they need double -> float/double generalization (should be pretty simple, as the algorithms are already nearly floating-point-type-parametrized).  Also JS::ToInt32 has a nice asm-optimized ARM version that of course is preferred to ToIntWidth<int32_t>.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Oops, the wrong bug number was in the commit message.

Comment 16

[Lars T Hansen [:lth]]

I can't reproduce the error in my TC in Developer Edition on Linux even with Ion and Baseline disabled, so I'm not going to worry about 45 and 46.  The problem is almost certainly exposed by a recent GCC, and it seems probable that we're using older GCC still for the production builds.  (Info about that would be welcome.)

Comment 17

[Lars T Hansen [:lth]]

Also reproducible - at least my TC - when configuring without any flags at all.  (Configuring with --enable-more-deterministic breaks regression testing, see bug 1250496.)

Comment 18

[Lars T Hansen [:lth]]

Attachment: proper conversions, not C++ casts

Now using the existing conversion code.  I've elected not to expose any float-input conversion operations, relying instead on float-to-double conversion along that path.  Also, I went over TypedArrayCommon.h to look for other conversions that might be unsafe, but found none.

Comment 19

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I'm going to mark this as sec-other because it seems to not affect anything we ship, but feel free to change it to sec-high or whatever if you think that is appropriate.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8722556 [details] [diff] [review]
proper conversions, not C++ casts

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

::: js/src/vm/TypedArrayCommon.h
@@ +65,5 @@
> +// integer -> floating conversion is well-defined, use a T() cast
> +// floating -> uint8_clamped conversion is well-defined, use a T() cast
> +// floating -> integer conversion has UB corner cases, use the JS::ToT() functions
> +// For float -> integer conversion, it will be float -> double -> integer if
> +//   the JS::ToT() functions don't have float specializations

Explicitly noting "UB" in the comment here has me leery.  Let's go ahead with a patch that *doesn't* have this comment.  Then add this comment in a month or two.

@@ +67,5 @@
> +// floating -> integer conversion has UB corner cases, use the JS::ToT() functions
> +// For float -> integer conversion, it will be float -> double -> integer if
> +//   the JS::ToT() functions don't have float specializations
> +
> +template<typename To, typename From> To convert(From src);

Style nit:

template<typename To, typename From>
inline To
convert(From src);

Additionally, "convert" is 1) rather vague, 2) not properly InterCaps.

@@ +71,5 @@
> +template<typename To, typename From> To convert(From src);
> +
> +template<> inline int8_t convert<int8_t, float>(float src) {
> +    return JS::ToInt8(src);
> +}

And all these should be

template<>
inline int8_t
convert<int8_t, float>(float src)
{
    return JS::ToInt8(src);
}

@@ +119,5 @@
> +}
> +
> +template<typename To, typename From> inline To convert(From src)
> +{
> +    return To(src);

Maybe add

    static_assert(!mozilla::IsFloatingPoint<To>::value ||
                  (mozilla::IsFloatingPoint<To>::value && mozilla::IsFloatingPoint<From>::value),
                  "conversion to floating point should have been handled by "
                  "specializations above");

@@ +260,5 @@
>          switch (source->as<TypedArrayObject>().type()) {
>            case Scalar::Int8: {
>              SharedMem<JS_VOLATILE_ARM int8_t*> src = data.cast<JS_VOLATILE_ARM int8_t*>();
>              for (uint32_t i = 0; i < count; ++i)
> +                Ops::store(dest++, convert<T, int8_t>(Ops::load(src++)));

Get rid of the second template argument to all of these -- it'll be inferred, and specifying it (especially in copy-paste code) seems like an opportunity to get it wrong.

@@ +415,5 @@
>          switch (source->type()) {
>            case Scalar::Int8: {
>              int8_t* src = static_cast<int8_t*>(data);
>              for (uint32_t i = 0; i < len; ++i)
> +                Ops::store(dest++, convert<T, int8_t>(*src++));

Again, remove the second parameter.

Comment 21

[Lars T Hansen [:lth]]

Attachment: do not convert by cast

Comment 22

[Lars T Hansen [:lth]]

Attachment: Do not convert fp to int by cast

As suggested, mostly (static_assert needed tweaking and required the introduction of a case for uint8_clamped, but nothing dramatic).  I kept the comment block but removed the bit about UB - did you want me to remove the entire comment block?

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8726701 [details] [diff] [review]
Do not convert fp to int by cast

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

Yeah, I think I'd remove the entire comment block.

Comment 24

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fda6bf7dcb31a863deb3d035fd9552a80471116
Bug 1248153 - Do not convert fp to int by cast. r=waldo

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/8fda6bf7dcb3

This missed the merge, do we want to uplift to aurora47?

Comment 26

[Lars T Hansen [:lth]]

IMO this problem is related chiefly to using GCC5, which, IIUC, we do not use in production yet.  So no, no need to uplift unless that is expected to change.  (The presumed irrelevance is why I had little urgency in getting this landed in the first place.)

Let's ask Waldo too.

Comment 27

[Jeff Walden [:Waldo]]

I would be inclined to uplift this everywhere.

For starters, not everyone doesn't use gcc5 -- it seems super-likely that Fedora and Ubuntu and others use gcc5, for example.

Past that, we don't *really* know exactly what assumptions all our supported compilers make here.  Local inspection of how they behave (or at least the particular facets we examine) will give only a partial picture, and it's not worth the trouble to get a deep-dive, comprehensive answer (which we might not be able to get for the non-open-source compiler we use, even if we wanted to).

But given we seem to not see anything super-wrong with the compiler versions we use, I certainly don't think we need to rush fixing this.  Somewhere reasonably comfortably out from branch/release date (4-6 weeks, even) seems a fine time to land this, and then to uplift it everywhere, for the most testing.  There's not huge risk of us footgunning ourselves by implicitly revealing this problem, seems to me, so we should use whatever time we can to be certain we've correctly implemented all the conversions.

Comment 28

[Lars T Hansen [:lth]]

Wes, given the fairly complicated proposal from Jeff about the uplift schedule, how do we manage this?  What do you need me to do?

Comment 29

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I would just request sec-approval, mentioning the particular circumstances, and uplift everywhere once you have that. As Jeff says, we're early in the release cycle, so it is a good time to land it.

Comment 30

[Lars T Hansen [:lth]]

Comment on attachment 8726701 [details] [diff] [review]
Do not convert fp to int by cast

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Chance of triggering C++ undefined behavior with some compilers, which could lead to weird bugs and/or exploit possibilities
[Describe test coverage new/current, TreeHerder]: No regression testcase landed at this point, but otherwise test coverage should be good
[Risks and why]: Performance regression risk, mostly, due to more complicated paths in some cases.  Considered minor.
[String/UUID change made/needed]: n/a

Note that the patch that landed is slightly different from this patch; pick the former.

Comment 31

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I got Dan to add a new csectype-undefined, so I'll use that here.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Lars can you request sec-approval?

Comment 33

[Lars T Hansen [:lth]]

Comment on attachment 8726701 [details] [diff] [review]
Do not convert fp to int by cast

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

We have no idea - we're tickling undefined conversion behavior in the language, and an aggressive compiler can exploit that to generate code that is not what we expect.  We don't know for sure which compilers will do this, we've only seen GCC5 so far.  We don't know that it's exploitable.  Pls see recent discussion on this bug.

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

No - there was no test case committed, and the incriminating comment that is in this patch was removed before the patch landed.

Which older supported branches are affected by this flaw?

Probably many, because when I last rewrote the code I think I only copied casts that were already in the code, and those casts are wrong.

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

Don't know - it might have been a long time ago, when TypedArrays came into the engine.

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

The patch will likely apply to the most recent branches, that particular code has been fairly stable since it was rewritten.  (And if it does not it can be adapted.)  Trying to backport beyond the large rewrite is unlikely to be meaningful work.

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

It is not likely to cause regressions.  For most conversions the same code is generated as before.  For the rest we are mostly using previously tested conversion code.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding tracking for 46+, sounds like this may be good to uplift

Comment 35

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #32)
> Lars can you request sec-approval?

This is a sec-other. Why does it need sec-approval?

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Al: I wasn't sure if it did or not, but because the whiteboard mentioned possible sec-high issues I thought it might be a good idea to run it by you.

Comment 37

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

Comment on attachment 8726701 [details] [diff] [review]
Do not convert fp to int by cast

Ah, ok. That makes sense.

sec-approval+. It is a good time for things to go in.

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8726701 [details] [diff] [review]
Do not convert fp to int by cast

This may land in time for the beta 4 build today but if not, beta 5.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1abf0bb9827a

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2fc0bb903817