1659687 - WebAssembly memory growth failure since version 79

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[colin.guyon]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

Trying to instantiate a WebAssembly Memory object of 512MiB with a maximum of 4GiB and then making it grow is not working anymore since Firefox 79

Actual results:

let memory = new WebAssembly.Memory({initial: parseInt(230 / 2 / (64 * 210)), maximum: parseInt(4 * 230 / (64 * 210))})
memory
Uncaught RangeError: failed to grow memory

But when setting 4GiB - 64KiB for the maximum, it works. So the behavior seems to have changed. Is there a new maximum limit?

Expected results:

The WebAssembly.Memory object should be able to grow even if the maximum is 4GiB, or the object creation should have failed

Comment 1

[colin.guyon]

Sorry, it is not properly formatted (and I didn't find how to edit the initial description):

> let memory = new WebAssembly.Memory({initial: parseInt(2**30 / 2 / (64 * 2**10)), maximum: parseInt(4 * 2**30 / (64 * 2**10))})
> memory.grow(1)
Uncaught RangeError: failed to grow memory

But when setting 4GiB - 64KiB for the maximum, it works. So the behavior seems to have changed. Is there a new maximum limit?

Comment 2

[Lars T Hansen [:lth]]

Less convoluted TC:

(new WebAssembly.Memory({initial: 8192, maximum: 65536})).grow(1)

We have a bug where a max size of 4GB (64K pages of 64K each) is interpreted as zero. A uint64_t is required to represent the maximum size of a wasm memory (because we multiply page count by page size early on, and 65536*65536 is not representable in 32 bits) and we correctly return Maybe<uint64_t> from the buffer's wasmMaxSize getter. But this is cast implicitly to Maybe<uint32_t> in grow(), thus we get zero, thus the max size is seen as zero, thus we fail to grow the memory.

As to how badly this bug breaks code, it is not normally going to be a problem for memories since they often don't have max sizes and we distinguish "not present" from zero; nor would most programs request a max size of 4GB. As a functionality bug it's manageable.

The underlying problem is that Maybe<uint64_t> downcasts silently to Maybe<uint32_t> and we had a change recently that introduced such a downcast inadvertently. I marked the bug s-s just to be safe since I think the casting behavior is nasty, but it may be Working As Designed. Nathan?

The code that uses Maybe<uint32_t> in grow() was introduced here:

# HG changeset patch
# User Ryan Hunt <[e-mail]>
# Date 1567198430 0
#      Fri Aug 30 20:53:50 2019 +0000
# Node ID 5bd0f481e985190cf9bf6235d055d97a1378432b
# Parent  47e5adb5ad3137edae7d39b84f74d3d748e734ef
Bug 1518210 - Wasm: Conditionally create huge memory's based on wasm::IsHugeMemoryEnabled. r=lth

but it was correct at the time. The inadvertent downcast was introduced recently, when we upgraded to 64-bit and changed the signature of wasmMaxSize()

# HG changeset patch
# User Ryan Hunt <[e-mail]>
# Date 1593062512 0
#      Thu Jun 25 05:21:52 2020 +0000
# Node ID cad72d98f53b308424453d92e11c8100c0e0f77e
# Parent  09322b9cdd6278b42f22dab5eff3a1de56b4fd02
Bug 1642940 - Widen Limits::{initial, maximum} to uint64_t. r=lth

As the reporter says, it should thus be in FF79 but not in FF78.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1659687 - Use Maybe<uint64_t> for max memory size. r?rhunt

Use Maybe<uint64_t> for the max size in three places where this is required.

Drive-by fix: Use standard printf macros for formatting, and use them correctly.

Depends on D82023

Comment 4

[Ryan Hunt [:rhunt] (on leave until early May)]

We should get a test added for this to limits.js [1].

[1] https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/js/src/jit-test/tests/wasm/limits.js

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Lars T Hansen [:lth] from comment #2)

The underlying problem is that Maybe<uint64_t> downcasts silently to Maybe<uint32_t> and we had a change recently that introduced such a downcast inadvertently. I marked the bug s-s just to be safe since I think the casting behavior is nasty, but it may be Working As Designed. Nathan?

Uh. You're saying that one can do:

  // Picking a readily-available 64-bit number; obviously other numbers could be worse here.
  Maybe<uint64_t> y = Some(UINT64_MAX);
  Maybe<uint32_t> x = y;

and it just silently works? We should fix that; I don't think that behavior is desirable. Can you file an MFBT bug on that, please?

Comment 6

[Lars T Hansen [:lth]]

Agreed on test case, easy enough to add here if we don't think the bug is exploitable.

Will file an MFBT bug tomorrow.

Comment 7

[Lars T Hansen [:lth]]

This is clearly not exploitable and it's not even a DOS possibility, it's just a compatibility issue. So I will add a test case and land the patch. Keeping it s-s is probably right until the mfbt issue has been fixed.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/4b66bbac751a3f7ab25e8b97f4ccb4a03006db39
https://hg.mozilla.org/mozilla-central/rev/4b66bbac751a

Comment 10

[Andrew McCreight [:mccr8]]

sec-other is more appropriate when there's no sec issue here, but it is being hidden because of an issue being tracked in another bug.