1479529 - Make oomAfterAllocations/oomAtAllocation more robustly usable with minification

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

gkw had lunch the other day and talked about a fuzz bug (bug 1476383).

The problem: fuzz bugs that hit a bad OOM handling case are tricky to minimize, because they rely on the exact pattern of allocations leading up to the OOM as well as the post-OOM handling code.

oomAfterAllocations makes this a bit easier, since if it is included in the test case and is what triggered the OOM, then most of the preceding code can be eliminated without losing the crash.

But that doesn't hope if you're just running general JS code and hitting an OOM that is handled badly.

The idea: if the fuzzing harness could cause the same allocation to fail in a minimized test case, then it could insert an oomAtAllocation(N) with just the right N to reproduce the failure. Hopefully.

So say we add a function call ImNotDeadYet(). It does absolutely nothing, or as little as possible, that is visible to the rest of the engine. But it updates a "still alive at allocation number K" global variable. When you hit an OOM crash, the fuzzing harness's post-crash gdb script would read out this value as well as the current allocation counter, and subtract the two to produce a delta D.

Then the minimizer could change the last ImNotDeadYet() that was executed in the source to oomAtAllocation(D) or oomAfterAllocations(D), and proceed to minimize normally.

This isn't a straightforward thing to do: which ImNotDeadYet() was the last one executed? What if it's called in a loop, and you need to target the 17th invocation not the 1st? What if the crash depends on two failed allocations followed by some successful ones?

But I'm posting this bug to see if the fuzzers find it to be worthwhile. I have a patch for this (though it uses recordAllocationNumber() in place of ImNotDeadYet()). I suspect we'd want to expand it accept a unique ID, eg

  recordAllocationNumber(37487234);

to make it easier to identify the correct call to update with oomAtAllocation. And I need to think about the threadType garbage. But I guess I wanted to get an opinion from the fuzzers as to whether this would be useful, or whether there's some fatal flaw I'm missing or this would just be too hard to make use of or too unlikely to work. Or whether something similar would work better.

If this *does* work, it might be even more useful to do the same thing with the GC -- instead of an allocation counter, it would need to record the GC heap size and then have a way of lying to the GC about its current heap size.

Comment 1

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

fwiw, I think lithium is able to reduce with varying numbers to be replaced in a testcase:

e.g. if a testcase involves:

oomAfterAllocations(80)

we are able to vary the number passed into the function arbitrarily in a range (e.g. 0 to 100) and have it set to "interesting" as long as the issue (crash/assert) still happens with any number within that range.

e.g. thus if the number generated is 89, the line turns to:

oomAfterAllocations(89)

the testcase is still marked as "interesting", even though "80" no longer reproduces the issue.

I'm not sure if this is still relevant in the context of comment 0, but thought it'll be good to have this noted down.

Comment 2

[Christian Holler (:decoder)]

Is there a reason to still support oomAfterAllocations/oomAtAllocation if we have the more reliable oomTest? We introduced that function for that particular reason, to make things more reliable.

Comment 3

[Steve Fink [:sfink] [:s:]]

I'm going to close this bug because both comment 1 and comment 2 give valid reasons why it wouldn't be that helpful. We may still want *something* for GC triggers. I'll file a separate bug for that... bug 1509157.