Closed Bug 1402815 Opened 2 years ago Closed 2 years ago

Implement interruptTest function for testing all interrupt return points in the JS engine

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: decoder, Assigned: decoder)

Details

(Keywords: sec-want, Whiteboard: [adv-main58-])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1395240 +++

We could use a technique similar to oomTest and stackTest to simulate interrupts in the JS engine with a callback that returns false (thereby stopping execution and forcing early returns in some functions).


I have written a patch for this and it already found at least one bug in fuzzing. Some concerns I have about the patch that I would like to point reviewers to in particular are:


1) It (ab)uses a lot of the OOM environment, like using the js::oom:: namespace, listening to the same OOM_* environment variables, using cx->runningOOMTest to prevent calling stackTest and oomTest from running at the same time and disabling the function by --disable-oom-functions. From a fuzzing standpoint, a lot of this behavior is desirable and I would like to keep it this way, but this function has nothing to do with "oom" itself so maybe we cant to restructure this at some point? It seems like a lot of duplication though because the function needs the same structures.


2) Does expectExceptionOnFailure make any sense for this function since the handler itself generates and throws the exception? If it isn't the failing caller that is supposed to call some error function to set an exception then I guess checking for this has little value?

3) nbp mentioned that this doesn't cover some JIT cases. Can you elaborate on this here in the bug so we can discuss if and how we can cover that (potentially also in a follow-up patch)?

4) I added code in InterruptTest to clear out my artificial (failing) interrupt handler after completion of the function but I'm unsure if this is the correct approach. It seems to be working though.


Patch coming directly into reviewboard in a few.
Attachment #8911737 - Flags: feedback?(nicolas.b.pierron)
(In reply to Christian Holler (:decoder) from comment #0)
> 3) nbp mentioned that this doesn't cover some JIT cases. Can you elaborate
> on this here in the bug so we can discuss if and how we can cover that
> (potentially also in a follow-up patch)?

The Jit are also checking the interrupt callback, and the problem with the current patch is that ShouldFailWithInterrupt is not called form the JITs, thus this patch will only check for the interruption callback execution when executed within the Interpreter, or maybe the first time we run baseline code.

Baseline [1] checks that the flag is set before calling the interruption callback.

Ion generate codes on the Out-of-line code path [2] but it is not executed unless the execution is being interrupted with a signal and that back-edges are patched [3] to enter the interrupt callback.

Yes this can be done in follow-up patches, but I would prefer if we do not add additional code-path, in order to test the patching part of the code too.

[1] http://searchfox.org/mozilla-central/source/js/src/jit/BaselineCompiler.cpp#700
[2] http://searchfox.org/mozilla-central/source/js/src/jit/CodeGenerator.cpp#12492
[3] http://searchfox.org/mozilla-central/source/js/src/wasm/WasmSignalHandlers.cpp#1433
(In reply to Christian Holler (:decoder) from comment #0)
> 1) It (ab)uses a lot of the OOM environment, like using the js::oom::
> namespace, listening to the same OOM_* environment variables, using
> cx->runningOOMTest to prevent calling stackTest and oomTest from running at
> the same time and disabling the function by --disable-oom-functions. From a
> fuzzing standpoint, a lot of this behavior is desirable and I would like to
> keep it this way, but this function has nothing to do with "oom" itself so
> maybe we cant to restructure this at some point? It seems like a lot of
> duplication though because the function needs the same structures.

I think basing it on the OOM code is okay for these testing functions. The code duplication is a bit unfortunate - it might be nice to abstract this more but since this is testing code and we're not planning to add more of these functions (right?) I don't think it has to block landing.

> 2) Does expectExceptionOnFailure make any sense for this function since the
> handler itself generates and throws the exception? If it isn't the failing
> caller that is supposed to call some error function to set an exception then
> I guess checking for this has little value?

Yeah not sure if that's needed here; unlike OOM and overrecursion, interrupts throw an uncatchable exception (returning false without reporting an exception on the cx).

> 3) nbp mentioned that this doesn't cover some JIT cases. Can you elaborate
> on this here in the bug so we can discuss if and how we can cover that
> (potentially also in a follow-up patch)?

The JITs have their own interrupt checks. It seems the only way to handle this is by calling a C++ function before we do an interrupt check and there we can set cx->interrupt_ if needed. For this to work we probably have to discard all JIT code in the testing function, and then when we generate JIT code we can check if we're interrupt-testing and then emit the call...

> 4) I added code in InterruptTest to clear out my artificial (failing)
> interrupt handler after completion of the function but I'm unsure if this is
> the correct approach. It seems to be working though.

It might be safer to do something like:

  auto& callbacks = cx->interruptCallbacks();
  if (!callbacks.empty() && callbacks.back() == FailingInterruptCallback)
      callbacks.popBack();
(In reply to Jan de Mooij [:jandem] from comment #3)

> 
> It might be safer to do something like:
> 
>   auto& callbacks = cx->interruptCallbacks();
>   if (!callbacks.empty() && callbacks.back() == FailingInterruptCallback)
>       callbacks.popBack();


We discussed on IRC that this can fail due to the fact that the code being tested could theoretically add another callback. We should be able to leave the code as it is because erase has asserts and checks in place that we are not removing anything past the end (plus, JS content should not be able to remove previous interrupt handlers that are already in place).
Comment on attachment 8911737 [details]
Bug 1402815 - Add interruptTest function to JS engine.

https://reviewboard.mozilla.org/r/183148/#review189846

Sorry for the delay, LGTM!
Attachment #8911737 - Flags: review?(jdemooij) → review+
Pushed by choller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/754da8e69897
Add interruptTest function to JS engine. r=jandem
https://hg.mozilla.org/mozilla-central/rev/754da8e69897
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Whiteboard: [adv-main58-]
You need to log in before you can comment on or make changes to this bug.