1439898 - Wasm: Incorrect combination of error encoding and assertion in wasm decoding and binary->text

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Lars T Hansen [:lth]]

In DecodeMemoryLimits() in WasmValidate.cpp we use UINT32_MAX as a sentinel:

    if (memory.maximum) {
        if (*memory.maximum > MaxMemoryMaximumPages)
            return d.fail("maximum memory size too big");

        CheckedInt<uint32_t> maximumBytes = *memory.maximum;
        maximumBytes *= PageSize;

        // Clamp the maximum memory value to UINT32_MAX; it's not semantically
        // visible since growing will fail for values greater than INT32_MAX.
        memory.maximum = Some(maximumBytes.isValid() ? maximumBytes.value() : UINT32_MAX);
    }

In RenderResizableMemory() in WasmBinaryToText.cpp this invalid value becomes observable:

    if (resizedMemory.maximum) {
        MOZ_ASSERT(*resizedMemory.maximum % PageSize == 0);
        *resizedMemory.maximum /= PageSize;
    }

If the memory size is given as (0 65536) then the assertion above will fail and we crash.

The sentinel logic came in with bug 1342045:

    changeset:   344605:3453a55b3053
    user:        Benjamin Bouvier <[e-mail]>
    date:        Thu Feb 23 16:11:00 2017 +0100
    summary:     Bug 1342045: Allow wasm::Memory of size 2**32; r=luke

In comments on that bug it is asserted that UINT32_MAX is an OK compromise and that it's not observable if we're off by one byte, but that sentinel is incorrect.  If anything it needs to be 2^32-PageSize, or the assert will fail.  But at that point we might as well just set the limit back to 65535.

I found this by adding a call to wasmBinaryToText to jit-test/tests/wasm/spec/harness/index.js in the function module():

    let module;
    try {
	print(wasmBinaryToText(new Uint8Array(buffer)));    // THIS ONE
        module = new WebAssembly.Module(buffer);
    } catch(e) {

and then trying to run spec/memory.wast.js.

Comment 1

[Lars T Hansen [:lth]]

That line of debugging code also causes crashes in several other spec tests, with an assertion in Vector code:

  Assertion failure: !empty(), at /home/lhansen/m-i/js/src/build-arm64/dist/include/mozilla/Vector.h:559

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Nice find. There's still a test in the spec tests that a memory of size 65536 is allowed [1].

We have a few ways forward here:
- accept that we do not pass this test
- use 2**32 - PageSize here, which would be unfortunate, since it's confusing with a memory of 65535 pages.
- special-case RenderResizableMemory symmetrically.

I'd go with the last option since 1. it allows us to pass the test, 2. binaryToText is only used in the shell, not the browser these days (which use the internal wasm->text conversion from debugger.html, iirc), so it's not worth investigating too deep in my opinion, 3. it parallels the special case in DecodeMemoryLimits, which kinda makes sense (special-cases both ways).

[1] https://github.com/WebAssembly/spec/blob/master/test/core/memory.wast#L6

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: patch

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8953109 [details] [diff] [review]
patch

I agree.

If you didn't already do this: Do you mind running through the wasm tests with that extra line of debug code inserted as mentioned in comment 0, to see if there are other failures?  See comment 1 re "other failures", which I have not diagnosed.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.unreachable.patch

These are just unreachable items, like in:

(func
  unreachable
  select
)

This is valid code, but the ast decoder tries to read three elements in the value stack. Just don't print anything in this case (this is dead code). We might sprinkle a few more in other AstDecode functions, but this is enough to have the spec tests passing.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8953178 [details] [diff] [review]
2.unreachable.patch

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

OK, makes sense.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b7f9b83e53
Special-case RenderResizableMemory for memories of UINT32_MAX bytes; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4bcef778b3
wasm: Don't render more unreachable items; r=lth

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5174f56c8c
Consider unreachable state before br_table in wasmBinaryToText; r=bustage on a CLOSED TREE

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Had to push this fix because brTable unconditionally sets polymorphic base for the current block, after it; we actually have to check *before* the br_table decoding to know if we're already in unreachable code.

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c2b7f9b83e53
https://hg.mozilla.org/mozilla-central/rev/9e4bcef778b3
https://hg.mozilla.org/mozilla-central/rev/db5174f56c8c