1725027 - Module instantiation does not handle OOM failures correctly

Status: RESOLVED WONTFIX

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

Description

[Ryan Hunt [:rhunt]]

Found while writing an OOM test for bug 1721718. When an OOM happens, we should 'return false' from the JS-native wrapper that instantiate is called from, and also call ReportOutOfMemory. We're currently returning false correctly, but not reporting the OOM in all cases.

The root cause is that Module::instantiate is confused about who must report an OOM error. For example, instantiateMemory may report a link error and then return false. Or it may encounter an OOM and return false, but not report it [2].

The instantiate path should be audited for this issue somehow. Using some richer type than bool as the return type may help too.

[1] https://searchfox.org/mozilla-central/rev/bb5549df90f9b0f5b453f9d8e872a94e503c64a6/js/src/jit-test/tests/wasm/oom/jsapi-prototype.js#27
[2] https://searchfox.org/mozilla-central/rev/bb5549df90f9b0f5b453f9d8e872a94e503c64a6/js/src/wasm/WasmModule.cpp#740

Comment 1

[Lars T Hansen [:lth]]

I agree that the current system is a mess, but I'm not seeing the problem with instantiateMemory. Having attempted to audit it, I found one case where no sensible error was reported, but this was only for a refcount overflow on shared memory, something that will not happen in normal use.

I will submit a patch for that, and I will audit the rest of the instantiate tree to look for errors, but so far I don't see any clear problems.

Comment 2

[Lars T Hansen [:lth]]

The underlying problem is that we use a single-bit value (a bool) for status return but status return is really a two-bit / tri-state value: no error, error that has been reported, and oom that has not been reported. (And sometimes there's a fourth value, non-oom error that has not been reported, but in that context there's usually other information alongside the error return to disambiguate, eg, an error message or error code).

In addition, there's no sensible reason why every non-void returning function should not implicitly be [[nodiscard]] so that the risk of dropping an error code on the floor essentially disappears. I don't know yet how to enforce that. But if we had a distinguishable error type it might be possible to run an analysis to ensure that it always appears with [[nodiscard]].

On top of that, as the Result class demonstrates, there may be a need to return a value or an error, and so there's the possibility of having to either do some tricky packing or (easier, IMO) to take a by-result argument for the value, and assume the value is dead unless the result is Ok. For some very performance-sensitive code this may be problematic but I doubt we have a lot of that in the wasm engine.

Comment 3

[Lars T Hansen [:lth]]

Attachment: status.patch

A sketch for how to do richer status values. This was a useful exercise; there are plenty of subtleties that show up. I don't think bugs were uncovered in this file but there are bugs higher up in the stack where OOM may or may not be reported properly. The main value of having an explicit status value is that there is never any doubt about what's going on, whether a subroutine did report an error or not - it's explicit.

Comment 4

[Lars T Hansen [:lth]]

Attachment: WIP: Bug 1725027 - Try to clean up OOM reporting in wasm instantiate

Comment 5

[Ryan Hunt [:rhunt]]

The situation here is not great, but I've not found any obvious errors and using a more explicit status is a lot of work. Closing this for now.