1229447 - Odin: Handle OOM when writing in the bytecode array

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: oom.patch

I couldn't make a failing test case out of this, but in theory this could happen: when we resize the IR vector, if it fails, it's going to return the index -1; then if the opcode at this position needs patching, we try to write at bytecode_[size_t(-1)], which sounds bad.

My guess is that I couldn't trigger a test case because the engine is always failing somewhere else in the meanwhile, but let's ensure this doesn't ever happen.

Just to be sure, marking it as hidden.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8694319 [details] [diff] [review]
oom.patch

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

I'd really like to avoid more OOM flags.  I think it ends up being much cleaner and easier to reason about to just propagate OOM via bool return value.  In this case, I'd return the offset (in the success case) as an outparam.  Alternatively, if we switched to postorder for expressions (which we may end up doing in the wasm spec), then I think that'd avoid most or even all of this backpatching (that needs the return value)?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

I've quickly started the approach of the outparam and return bool, but it really needs to change all the places in the code. Since the probability of triggering OOM and write arbitrary data seems fairly low, I'll just consider implementing the switch to postorder.

Comment 3

[Luke Wagner [:luke]]

I wouldn't switch to post-order just yet; some new considerations (that compute-line-numbers-during-decode idea which suggest that, if we want a pre-order-looking text format (with "foo(a,b)") then we want a pre-order bytecode) make it unclear which way we should go at this point.

Comment 4

[Andrew McCreight [:mccr8]]

I'll just mark this sec-other for now because you think it would be very hard to trigger in practice. Adjust as desired.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Superseded by the work in bug 1229399. For the record, we landed the patch there without sec-approval: it should be *really* hard to trigger (tried for several hours to make a failing test case out of it but I couldn't), so we decided to let the fix ride the trains.