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.)
Created attachment 318431 [details] wasm file
<rdar://problem/33952228>
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.
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 :-)
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.
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
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".
Created attachment 365807 [details] Patch Update wasm.json in JSTests
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?
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.
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.
Comment on attachment 365807 [details] Patch Clearing flags on attachment: 365807 Committed r243446: <https://trac.webkit.org/changeset/243446>
All reviewed patches have been landed. Closing bug.