Closed Bug 1457536 Opened 4 years ago Closed 4 years ago
Make Transition() friendlier to fuzzing
59 bytes, text/x-review-board-request
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 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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/07313b4db1d6 refactor Transition in IPC to make it more amenable to fuzing; r=froydnj
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
We have something like 1000 IPC methods , 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 , 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 . > 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.  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.