Bug 175693

Summary: WebAssembly: const in unreachable code decoded incorrectly, erroneously rejects binary as invalid
Product: WebKit Reporter: Alon Zakai <alonzakai>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Severity: Normal CC: bfulgham, buildbot, commit-queue, jfbastien, keith_miller, mark.lam, msaboff, product-security, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Linux   
Description Flags
js script
wasm file
saam: review+, saam: commit-queue-
commit-queue: commit-queue-
patch none

Description Alon Zakai 2017-08-17 16:08:53 PDT
Created attachment 318433 [details]
js script


jsc b.js -- b.wasm

it reports

Exception: Error: WebAssembly.Module doesn't parse at byte 287 / 321: can't get immediate for I64Const in unreachable context, in function at index 0 (evaluating 'new WebAssembly.Module(binary)')
Module@[native code]
global code@b.js:22:63

However the wasm binary is valid (accepted by v8, sm, binaryen, wabt).
Comment 1 Radar WebKit Bug Importer 2017-08-17 16:09:14 PDT
Comment 2 Alon Zakai 2017-08-17 16:09:15 PDT
Created attachment 318434 [details]
wasm file
Comment 3 JF Bastien 2017-08-17 16:31:17 PDT
I believe this is a benign bug because unreachable code shouldn't occur anyways. It's still a bug we should fix, though.
Comment 4 JF Bastien 2017-08-17 16:32:17 PDT
It looks like the i64.const causing the issue is:

    42 80 80 80 80 80 80 80 80 80 |       i64.const -9223372036854775808
Comment 5 JF Bastien 2017-08-17 16:33:38 PDT
Well this is our bug:

    // one immediate cases
    case I32Const:
    case I64Const: // <------ derp
    case SetLocal:
    case GetLocal:
    case TeeLocal:
    case GetGlobal:
    case SetGlobal:
    case Br:
    case BrIf:
    case Call: {
        uint32_t unused;
        WASM_PARSER_FAIL_IF(!parseVarUInt32(unused), "can't get immediate for ", m_currentOpcode, " in unreachable context");
        return { };

This is benign, not a security issue.
Comment 6 JF Bastien 2017-08-17 16:43:33 PDT
Created attachment 318440 [details]
Comment 7 Saam Barati 2017-08-17 16:44:39 PDT
Comment on attachment 318440 [details]

please add test
Comment 8 JF Bastien 2017-08-17 16:58:50 PDT
Created attachment 318443 [details]

Add test, and also handle i32.const / i64.const as signed.
Comment 9 WebKit Commit Bot 2017-08-17 16:59:52 PDT
Comment on attachment 318443 [details]

Rejecting attachment 318443 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 318443, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4333352
Comment 10 JF Bastien 2017-08-17 17:08:59 PDT
Created attachment 318444 [details]

Comment 11 WebKit Commit Bot 2017-08-17 18:03:28 PDT
The commit-queue encountered the following flaky tests while processing attachment 318444 [details]:

svg/animations/smil-leak-list-property-instances.svg bug 175701 (author: sabouhallawa@apple.com)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2017-08-17 18:04:03 PDT
Comment on attachment 318444 [details]

Clearing flags on attachment: 318444

Committed r220894: <http://trac.webkit.org/changeset/220894>
Comment 13 WebKit Commit Bot 2017-08-17 18:04:06 PDT
All reviewed patches have been landed.  Closing bug.