Closed Bug 1457536 Opened Last year Closed Last year

Make Transition() friendlier to fuzzing

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
This will be much easier to implement with bug 1323532.
Depends on: 1323532
Assignee: nobody → agaynor
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?
Attachment #8972974 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07313b4db1d6
refactor Transition in IPC to make it more amenable to fuzing; r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/07313b4db1d6
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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?
(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
Sounds good, thanks!
You need to log in before you can comment on or make changes to this bug.