1681684 - Cranelift API: max_memory_length returns bytes but should return pages

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

In memory_at() in bindings/mod.rs we construct a wasmparser::MemoryType::M32 which carries a wasmparser::ResizableLimits { min, max } structure. For memories, Alex says (https://github.com/bytecodealliance/wasm-tools/issues/168) that these values are supposed to be in units of pages, not bytes.

(I'm sort of annoyed since there are neither comments nor checks nor names to ensure that the units used when initializing this structure are the right ones; that said, the wasm spec operates on pages everywhere so it's not unreasonable to assume that.)

I have no idea (a) whether Alex is right here, though I suspect he is, and (b) what the implication of this bug is for us. It does not look like cranelift uses this information except in wasmparser's validator, and then only to ensure that max >= min. Our runtime gets the bounds check limit through our normal paths and should be safe.

Marking as s-s out of an abundance of caution (because, a limit) but not sure it's necessary. Marking P3 because not urgent to fix, but taking it since it's blocking other work.

It looks like the problems here are entirely in our interface code, no cranelift or wasmparser code should need to be updated.

Comment 1

[Lars T Hansen [:lth]]

The values in WasmValidate.h (ModuleEnvironment) for min and max memory length will remain uint64_t btw, and will be byte lengths. So the translation happens in WasmCraneliftCompile.cpp or further along the path to cranelift.

Comment 2

[Julian Seward [:jseward]]

Attachment: maybe-fix-1681684.diff-1

Is there any end-to-end way to verify the behaviour of min/max_memory_len
that doesn't involve sprinkling the compiler with printfs? I mean, some JS or
wasm we can run/verify/observe-failing-verification etc?

Anyways: attached is a possible fix, for consideration. Strictly WIP. Playing around with
js/src/jit-test/tests/wasm/big-resize.js, it seems to now print page-sized numbers,
per previous para, but I haven't tested it any further.

Comment 3

[Lars T Hansen [:lth]]

If ResizableLimits had a constructor / factory method instead of direct initialization, one could validate the values on entry (they should be at most 65536). As it is, a separate verification pass must be run. (One of the things I've really come to appreciate about SpiderMonkey is the liberal use we make of assertions.)

Alternatively, we'd put these asserts in the code that constructs the ResizableLimits.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 9192581 [details] [diff] [review]
maybe-fix-1681684.diff-1

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

Seems pretty reasonable.

::: js/src/wasm/WasmCraneliftCompile.cpp
@@ +394,5 @@
> +    : env(&env) {
> +  // env.minMemoryLength is in bytes.  Convert it to wasm pages.
> +  static_assert(sizeof(env.minMemoryLength) == 8);
> +  MOZ_RELEASE_ASSERT((env.minMemoryLength & 0xFFFF) == 0);
> +  min_memory_length = (uint32_t)(env.minMemoryLength >> 16);

I think we want to be a little more careful here and use wasm::PageSize, because it's not a given that it will remain a constant for all time, and it will serve as a useful marker for when we need to reconsider this code.

@@ +416,5 @@
>  
>  uint32_t env_max_memory(const CraneliftModuleEnvironment* env) {
> +  // env.maxMemoryLength is in bytes.  Convert it to wasm pages.
> +  if (env->env->maxMemoryLength.isSome()) {
> +    auto inBytes = *(env->env->maxMemoryLength);

House style is to avoid auto in cases like this: uint64_t.

Comment 5

[Julian Seward [:jseward]]

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

Ok. I'll nice it up a bit and turn it into a proper Phabricator patch.

auto inBytes = *(env->env->maxMemoryLength);

House style is to avoid auto in cases like this: uint64_t.

The point of the auto was so as to avoid telling the compiler what
type I thought this was, so that the next line ..

static_assert(sizeof(inBytes) == 8);

actually tests something meaningful.
Changing it to uint64_t would make the assertion pointless.

Comment 6

[Julian Seward [:jseward]]

Attachment: Bug 1681684 - Cranelift API: max_memory_length returns bytes but should return pages. r=lth.

CraneliftModuleEnvironment::min_memory_length and wasm::env_max_memory()
provide minimum and maximum memory sizes, given a ModuleEnvironment (not the
same as a CraneliftModuleEnvironment). Unfortunately a ModuleEnvironment
stores these sizes in bytes whereas CraneliftModuleEnvironment stores them
as number-of-wasm-pages. However, the existing code did not perform the
conversion, hence causing the returned values to be the number of bytes rather
than the number of pages.

This small patch does the required scaling. Given that this has to do with
memory bounds, it is hedged around with assertions for safety, to ensure:

• the incoming byte sizes really are 64-bit numbers, so that 2^32 can be
  represented.

• the incoming byte sizes do not exceed 2^32, since we currently do not
  implement 64-bit linear memories and hence do not expect validation to
  accept a memory of size greater than 2^32 bytes.

• the incoming byte size is a multiple of the wasm page size, so that the
  scaling can be done without loss of precision.

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/2e4f2dc6d455710ac5c553cb87f6c590b9227628
https://hg.mozilla.org/mozilla-central/rev/2e4f2dc6d455

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this something which needs backporting or can this ride the trains?

Comment 9

[Lars T Hansen [:lth]]

It can ride the trains.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:lth, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.