1500016 - Caching tests can't catch embedded static addresses issues

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

If I understand correctly, the testing mechanism we have in the shell serializes and deserializes Modules within the same process. So if the Module's code uses a static address, the testing mechanism can't catch it (since it will re-run the deserialized module in the same process). AsmJS caching tests were spawning a new process, and could have caught this.

Of course, it means that if we wanted to have more rigorous testing, we'd need both to spawn a new process within which we'd run the deserialized module's code the second time, *and* we'd need to have caching tests for every single WebAssembly opcode.

That's a lot of work, so an alternative could be to audit the existing code, add assertions here and there (ImmWord could be more restricted and have a wasm specific ctor, just to make sure we opt in into using it).

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: testcase.patch

Me trying to write a test case that should have crashed (before the "see also" bug was fixed) if we were spawning a different process.

Comment 2

[Lars T Hansen [:lth]]

By sheer luck I didn't write the code on bug 1499198 but I could have, because from time to time I do write code like that.  As I observed to Benjamin, that code was correct until we started caching compiled code, and only earlier this week I was thinking about the blessings of not having to worry about generating static binaries.  Sigh.

A good first step would be to audit ImmWord and AbsoluteAddress.  AbsoluteAddress is pretty restrictive, which is why we resort to ImmWord for this kind of code in the first place.  A big fat comment at ImmWord that says that trying to use it to work around problems with AbsoluteAddress is going to be wrong in the presence of cached code is another thing, at least it'll make us stop and think.  Generalizing the assembler so that we can place constants at meaningful relative addresses in the code (a la ARM) would also make the problem less acute.

Comment 3

[Luke Wagner [:luke]]

So AbsoluteAddress() and ImmPtr() both assert that !IsCompilingWasm().  Moreover, to catch this sort of bug, I added the nestedShell() primitive to the shell which we use to test asm.js serialization and jit-tests/asm.js/testBullet.js tests all of bullet with this.  So this was definitely a consideration in the initial asm.js serialization design.

The reason this one slipped through the cracks is that this is not an op hit by asm.js / bullet.  There is currently no ability to test cross-process caching with streamCacheEntry(), so it makes sense to add that.

Comment 4

[Daniel Veditz [:dveditz]]

The sec team thinks this sounds like an audit process rather than a specific vulnerability found, but we're unsure. please remove sec-audit if this is a specific vulnerability and we're re-triage it.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Definitely an audit process to protect against the kind of issues which bug 1499198 exemplifies.

Comment 6

[Luke Wagner [:luke]]

The new shell testing function (wasmCompileInSeparateProcess()) added in bug 1500203 includes the testcase in comment 1 and indeed fails when https://hg.mozilla.org/mozilla-central/rev/01368a04b48a is reverted.  wasmCompileInSeparateProcess() is fuzzing-safe and so it seems like fuzzing (esp differential) should give us the coverage we need to feel confident with wasm caching.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Great! Then I think we can close this bug when bug 1500203 lands.

Just a random idea here: we could reuse the wasmFullPass architecture here to run each test normally, then run a cached version in a separate process. I don't think we should do this in general (i.e. in CI), but maybe as a one-off: this should catch most issues there could be during code generation, if we did this for every architecture.

Comment 8

[Luke Wagner [:luke]]

That's a great idea; I'll do that locally.

Comment 9

[Luke Wagner [:luke]]

Trying comment 7 locally for x86/x64/arm32 (arm64 can't cache), there are no other failures and reverting the patch in bug 1499198 caused a pre-existing test to fail (wasm/conversion.js).