1457536 - Make Transition() friendlier to fuzzing

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

In working on IPC fuzzing, one of the challenges I'm hitting is that when the fuzzer generates an invalid state transition the IPC code aborts the whole process: OnMessageReceived() -> Transition() -> LogicError() -> MOZ_CRASH_UNSAFE_OOL().

Killing the whole process is disruptive to the fuzzers :-)

A more fuzzer-friendly way of structuring this code would be to have Transition return true/false depending on whether it was valid, and then returning an error from OnMessageReceived() if Transition had returned invalid.

In release builds errors from OnMessageReceived() result in killing the child (and possibly also the parent, I can't remember off hand), so that should be equally safe.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

This will be much easier to implement with bug 1323532.

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9da4653a74d7e0d23cd8f9cb9df93cba6f936077&group_state=expanded

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1457536 - refactor Transition in IPC to make it more amenable to fuzing;

Instead of crashing the process inside Transition on a bad state transition,
propagate an error up the stack (and crash higher up).

Review commit: https://reviewboard.mozilla.org/r/241530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241530/

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8972974 [details]
Bug 1457536 - refactor Transition in IPC to make it more amenable to fuzing;

https://reviewboard.mozilla.org/r/241530/#review247382

I am not 100% sure the below is needed, but we have pulled out `FatalError` et al for codesize reasons.

::: ipc/ipdl/ipdl/lower.py:413
(Diff revision 1)
> +def errfnUnreachable(msg):
> +    return [
> +        StmtExpr(ExprCall(ExprVar('MOZ_ASSERT_UNREACHABLE'),
> +                                   args=[ExprLiteral.String(msg)]))
> +    ]

Is it possible that we could make this an out-of-line function call to a `MOZ_ASSERT_UNREACHABLE` somehow?  `MOZ_ASSERT*` winds up pulling in quite a lot of stuff per callsite, and there can be a lot of callsites of `Transition`.  Surely we already have some out-of-line helper function we can call here?

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8972974 [details]
Bug 1457536 - refactor Transition in IPC to make it more amenable to fuzing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241530/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07313b4db1d6
refactor Transition in IPC to make it more amenable to fuzing; r=froydnj

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/07313b4db1d6

Comment 8

[Eric Rahm [:erahm]]

It looks like this regressed libxul size by 94k.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=autoland,1338582,1,2&series=mozilla-inbound,1299711,0,2&zoom=1525405011407.6763,1525442632363.0706,130343283.35434644,130679104.24986883&selected=autoland,1338582,333709,462920191,2

Comment 9

[Alex Gaynor [:Alex_Gaynor]]

We have something like 1000 IPC methods [citation needed], for each we generate a recv and a send function. For each one of those we're adding a branch and a call. 94kb / 2,000 => 47 bytes per. 47 bytes is slightly higher than I'd expect for the branch and call, but not totally ridiculous.

Hopefully this regression is offset somewhat by the gains in bug 1323532. Is it required to take action here?

Comment 10

[Eric Rahm [:erahm]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #9)
> We have something like 1000 IPC methods [citation needed], for each we
> generate a recv and a send function. For each one of those we're adding a
> branch and a call. 94kb / 2,000 => 47 bytes per. 47 bytes is slightly higher
> than I'd expect for the branch and call, but not totally ridiculous.
> 
> Hopefully this regression is offset somewhat by the gains in bug 1323532.

It looks like that improved by 47k [1].

> Is it required to take action here?

This is mostly me going through and annotating notable regressions in the past few months. It looks like this is just .text, so from a content process overhead perspective I'm not concerned. Given the improvement I'd say it's not worth worrying too much about it.

[1] https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=autoland,1299714,0,2&series=autoland,1338582,1,2&highlightedRevisions=e8097237f4be&zoom=1525210990077.3542,1525211201161.83,130446562.01627263,130536887.12914197&selected=autoland,1338582,332642,461187620,2

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Sounds good, thanks!