Bug 175691 – WebAssembly: max with NaN generates incorrect result

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alon Zakai]

Created attachment 318430 [details]
js script

The attached testcase is runnable with

jsc a.js -- a.wasm

The expected output is

calling: func_1
   result: NaN
done.

which is seen on v8 and sm. On jsc it prints

calling: func_1
   result: 4294967296
done.

(Filing as security-sensitive since it's a fuzz bug, but I don't have any information aside from that on the risk here.)

[Alon Zakai]

Created attachment 318431 [details]
wasm file

[Radar WebKit Bug Importer]

<rdar://problem/33952228>

[JF Bastien]

The disassembly is:

 ~/wasm/waterfall/src/work/wabt/bin/wasm-objdump -d ~/Downloads/a.wasm 

a.wasm:	file format wasm 0x1

Code Disassembly:

00002e func[0]:
 000030: 43 00 00 80 4f             | f32.const 0x1p+32
 000035: 10 01                      | call 1
 000037: 43 69 ab ff ff             | f32.const -nan:0x7fab69
 00003c: 97                         | f32.max
 00003d: 10 01                      | call 1
 00003f: 0b                         | end
000040 func[1]:
 000042: 20 00                      | get_local 0
 000044: 43 00 00 00 00             | f32.const 0x0p+0
 000049: 41 01                      | i32.const 1
 00004b: 1b                         | select
 00004c: 0b                         | end


This program does:

  select(f32.max(select(0x1p+32, 0x0p+0, 1), -nan:0x7fab69), 0x0p+0, 1)

We return 0x100000000.

[JF Bastien]

Equivalent wast:

(module
  (type (;0;) (func (result f32)))
  (type (;1;) (func (param f32) (result f32)))
  (func (;0;) (type 0) (result f32)
    f32.const 0x1p+32 (;=4.29497e+09;)
    call 1
    f32.const -nan:0x7fab69 (;=nan;)
    f32.max
    call 1)
  (func (;1;) (type 1) (param f32) (result f32)
    get_local 0
    f32.const 0x0p+0 (;=0;)
    i32.const 1
    select)
  (memory (;0;) 1 1)
  (export "func_1" (func 0)))


select returns the second operand on the stack in both cases because the condition is 1 (on the stack, so that's left-hand-operand in s-expression). The following:

    select(f32.max(select(0x1p+32, 0x0p+0, 1), -nan:0x7fab69), 0x0p+0, 1)

Is therefore equivalent to:

    f32.max(4294967296.0, -nan:0x7fab69)

Max is defined as follows:

    if either operand is NaN, returns NaN

So this is "just" a semantic bug, not security related :-)

[Alon Zakai]

It looks like this is still open (I confirmed by running the testcase on the latest jsc from jsvu).

I'm seeing incorrect behavior in a real-world app that is floating-point heavy, which reminded me about this. It's possible this is the cause of that app failing, although I can't be sure.

[Tadeu Zagallo]

Created attachment 365806 [details]
Patch

I decided to fix just f32.max first, to make it easier to review. Once everyone is happy with the solution and this lands, I'll go back and fix f32.min, f64.min and f64.max

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Tadeu Zagallo]

Created attachment 365807 [details]
Patch

Update wasm.json in JSTests

[Saam Barati]

Comment on attachment 365807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365807&action=review

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2820
> +    append(isNaN, AddFloat, arg0, arg1, result);

Why do we need this add?

> JSTests/wasm/wasm.json:156
> +        "f32.max":             { "category": "arithmetic", "value": 151, "return": ["f32"],      "parameter": ["f32", "f32"],           "immediate": [], "b3op": "Select(Equal(@0, @1), BitAnd(@0, @1), Select(LessThan(@0, @1), @1, Select(GreaterThan(@0, @1), @0, Add(@0, @1))))" },

Why do we have two copies of this?

[Keith Miller]

Comment on attachment 365807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365807&action=review

Well, that's gross... Too bad we don't have ICs for Wasm.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2808
>      append(isEqual, AndFloat, arg0, arg1, result);

Why are we anding them here? Why can't we just emit a move?

>> JSTests/wasm/wasm.json:156
>> +        "f32.max":             { "category": "arithmetic", "value": 151, "return": ["f32"],      "parameter": ["f32", "f32"],           "immediate": [], "b3op": "Select(Equal(@0, @1), BitAnd(@0, @1), Select(LessThan(@0, @1), @1, Select(GreaterThan(@0, @1), @0, Add(@0, @1))))" },
> 
> Why do we have two copies of this?

We have two because we use other parts of the json to generate the wasm binaries.

[Tadeu Zagallo]

Comment on attachment 365807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365807&action=review

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2808
>>      append(isEqual, AndFloat, arg0, arg1, result);
> 
> Why are we anding them here? Why can't we just emit a move?

I believe it's because of the max(0, -0) case, where we have to return the positive 0.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2820
>> +    append(isNaN, AddFloat, arg0, arg1, result);
> 
> Why do we need this add?

Because we don't know which of the args in NaN.

[WebKit Commit Bot]

Comment on attachment 365807 [details]
Patch

Clearing flags on attachment: 365807

Committed r243446: <https://trac.webkit.org/changeset/243446>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.