1745117 - Assertion failure: ArrayBufferObject::maxBufferByteLength() % PageSize == 0, at wasm/WasmMemory.cpp:266

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

new WebAssembly.Instance(
  new WebAssembly.Module(
    wasmTextToBinary(`
      (memory i64 1)
      (func
        (param $p i64)
        (param $c i32)
        (param $n i64)
        (memory.fill
          (local.get $p)
          (local.get $c)
          (local.get $n)
        )
      )
    `)
  )
);

https://hg.mozilla.org/mozilla-central/rev/d49d8003e35c69debbd83e84632954ee5f38fdcc may be the regressor.

Run with --fuzzing-safe --no-threads --no-baseline --no-ion --no-large-arraybuffers, compile with AR=ar sh ./configure --enable-debug --disable-optimize --with-ccache --enable-gczeal --enable-debug-symbols --disable-bootstrap --disable-tests, tested on m-c rev 740fad344989.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The reason why this happens is:

js/src/vm/ArrayBufferObject.h:193

188	  // The length of an ArrayBuffer or SharedArrayBuffer can be at most
189	  // INT32_MAX. Allow a larger limit on friendly 64-bit platforms if the
190	  // experimental large-buffers flag is used.
191	  static size_t maxBufferByteLength() {
192	#ifdef JS_64BIT
193	    if (supportLargeBuffers) {
194	      return size_t(8) * 1024 * 1024 * 1024;  // 8 GB.
195	    }
196	#endif
197	    return MaxByteLengthForSmallBuffer;
198	  }

Due to --no-large-arraybuffers, MaxByteLengthForSmallBuffer is 2**31 - 1 (2GB - 1) instead of 2**33 (8GB).

This means that:

js/src/wasm/WasmMemory.cpp:265 (non-cranelift code)

263	wasm::Pages wasm::MaxMemoryPages(IndexType t) {
264	  MOZ_ASSERT_IF(t == IndexType::I64, !IsHugeMemoryEnabled(t));
265	  MOZ_ASSERT_IF(t == IndexType::I64,
266	                ArrayBufferObject::maxBufferByteLength() % PageSize == 0);
267	  size_t desired = MaxMemoryLimitField(t);
268	  size_t actual = ArrayBufferObject::maxBufferByteLength() / PageSize;
269	  return wasm::Pages(std::min(desired, actual));
270	}
271	#  endif

The modulus check for ArrayBufferObject::maxBufferByteLength() % PageSize fails as the value is 1 instead of 0. In a release build, actual will then be 32767 which is not a 2**n value.

Assuming the assertion message always has to be true, we could try removing the if (supportLargeBuffers) { guard check to always emit 8GB from maxBufferByteLength() on 64-bit systems. Or, emit MaxByteLengthForSmallBuffer + 1 if on --no-large-arraybuffers for those 64-bit systems instead, as MaxByteLengthForSmallBuffer % PageSize is 1 instead of 0. (Edit: I don't think the fix should be in ArrayBufferObject, it should be in wasm code instead)

Otherwise, the assertion at https://searchfox.org/mozilla-central/rev/1674b86019a96f076e0f98f1d0f5f3ab9d4e9020/js/src/wasm/WasmMemory.h#79 also fails, as 32767 % PageSize is no longer equal to zero.

Comment 2

[Lars T Hansen [:lth]]

Thanks, I'll take a look. (Memory64 is nightly-only code for now.)

Comment 3

[Christian Holler (:decoder)]

The --no-large-arraybuffers flag is not part of fuzz flags. I've spoken to jandem and lth about this and they think the flag should be removed altogether. This configuration is not supposed to be used by the browser right now. The origin of the flag comes from --large-arraybuffers when it was still non-default. We turned this on and the flag automatically got converted.

So this does not affect any current or planned future configuration of the browser.

Comment 4

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

Set release status flags based on info from the regressing bug 1742383

Comment 5

[Lars T Hansen [:lth]]

The irony here is that nothing depends on what's being asserted. Anyhow the assertion is wrong (as the test case shows) and can just be removed.

We deal almost exclusively in pages throughout the wasm system; the two cases (in WasmIonCompile.cpp and WasmBCMemory.cpp) where we call maxBufferByteLength we should arguably be calling MaxMemoryPages instead.

Comment 6

[Christian Holler (:decoder)]

Opening per comment 5.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Bug 1745117 - Adjust use of maxBufferByteLength. r?yury

Remove an incorrect assertion in WasmMemory.cpp -- we don't need to depend on
the max buffer byte length being a multiple of page sizes, as we always round.

Rewrite two uses of maxBufferByteLength to instead use MaxMemoryPages, since
it's the latter quality we care about.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0afa754df085
Adjust use of maxBufferByteLength. r=yury

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/0afa754df085

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.

Comment 11

[Lars T Hansen [:lth]]

This is nightly-only code for one thing and a non-release assert for another thing, so I think we're good.