1342045 - wasm: Memory maximum limit checks are off by one

Status: RESOLVED FIXED

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

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

So the spec core test that requests a Memory with initial: 0, maximum: 65536 is actually correct. In this case, the max memory size, in number of bytes is precisely 2**32, while we check for UINT32_MAX, which is... 2**32 - 1. So we're off by one here. Fixing this will require having an uint64 maximum limit, only for this edge case, which sounds very unfortunate.

I'm deferring this issue which sounds minor at the moment. Posting a WIP patch with explorations, but this crashes because more places probably want an uint64, and it seems there are more important issues right now.

Comment 1

[Luke Wagner [:luke]]

I think it shouldn't be semantically visible if we clamp maximums to UINT32_MAX internally.  This would allow us to keep everything as uint32_t.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

That's right: the only semantically visible change would be when growing memory from 65535 to 65536 (since we're checking against the max limit), but growing fails if new requested memory byte size > INT32_MAX anyhow, clamping or not.

Comment 3

[Luke Wagner [:luke]]

Right, the meaning of maximum is that the engine is always allowed to fail growing before maximum; it just can't grow *past* maximum.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: memory.patch

This makes writing the patch much easier, but we'll have to be very careful if/when we're allowed to create ArrayBufferObjects with a length bigger than 2**32 in the future.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8840471 [details] [diff] [review]
memory.patch

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

Thanks!  I think we can get rid of all the uint64_t's though:

::: js/src/wasm/WasmBinaryConstants.h
@@ +458,5 @@
>  static const unsigned MaxLocals              =    50000;
>  static const unsigned MaxParams              =     1000;
>  static const unsigned MaxBrTableElems        =  1000000;
>  static const unsigned MaxMemoryInitialBytes  = 1024 * 1024 * 1024;
> +static const uint64_t MaxMemoryMaximumBytes  = 4UL * uint64_t(MaxMemoryInitialBytes);

How about making this MaxMemoryMaximumPages and doing the limit check before the `maximumBytes *= PageSize`?  For symmetry, it'd be good to do likewise with MaxMemoryInitialBytes.  Also, I'd spell out the max value directly instead of in terms of initial.

::: js/src/wasm/WasmValidate.cpp
@@ +836,5 @@
>  
>      memory.initial = initialBytes.value();
>  
>      if (memory.maximum) {
> +        CheckedInt<uint64_t> maximumBytes = *memory.maximum;

Then this can stay a uint32_t.

@@ +849,5 @@
> +        if (maxBytesValue == MaxMemoryMaximumBytes)
> +            maxBytesValue = UINT32_MAX;
> +
> +        MOZ_ASSERT(uint64_t(uint32_t(maxBytesValue)) == maxBytesValue);
> +        memory.maximum = Some(uint32_t(maxBytesValue));

And then I think this logic could be:
  memory.maximum = Some(maximumBytes.isValid() ? maximumBytes.value() : UINT32_MAX);

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3453a55b3053
Allow wasm::Memory of size 2**32; r=luke

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3453a55b3053