Closed Bug 1450231 Opened 2 years ago Closed 2 years ago

Ignore MOZ_CRASH in FatalError() for --enable-fuzzing builds

Categories

(Core :: IPC, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: posidron, Assigned: posidron)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fatalerror.diff (obsolete) — Splinter Review
No description provided.
Attachment #8963901 - Flags: superreview?(jld)
Attachment #8963901 - Flags: review?(jld)
I think we should also disable the |ProtocolErrorBreakpoint| at the top of |FatalError|.
Attached patch fatalerror.diffSplinter Review
Attachment #8963901 - Attachment is obsolete: true
Attachment #8963901 - Flags: superreview?(jld)
Attachment #8963901 - Flags: review?(jld)
Attachment #8964489 - Flags: superreview?(jld)
Attachment #8964489 - Flags: review?(jld)
Comment on attachment 8964489 [details] [diff] [review]
fatalerror.diff

Review of attachment 8964489 [details] [diff] [review]:
-----------------------------------------------------------------

Technically this isn't a problem, since it won't affect non-FUZZING builds.  I was concerned that it might cause problems, at least in the form of findings that don't make sense and don't apply to real builds, because it might get into state space that normally can't happen, and in particular because code that uses IPC tends to depend on errors being immediately fatal on the child side and doesn't handle errors.  So I looked through IPC error handling, and if I'm following all the method calls correctly that's not a problem — this patch applies only to incoming messages that fail deserialization, and quietly dropping those should be fine, because an attacker could just not send them.

But I'm a little confused, because I'd expect the MsgValueError to end up in the toplevel actor's ProcessingError method, which also kills the child process, and I'd expect that to cause the same problems for fuzzing as these crashes.  (We do, I think, want to keep that behavior for the MsgProcessingError case when a handler returns false, but that's handled through IPCResult::Fail.)

In any case, r=me, and sorry for taking so long with the review.

::: ipc/glue/ProtocolUtils.cpp
@@ +280,1 @@
>    ProtocolErrorBreakpoint(aMsg);

This shouldn't cause problems, and it could be helpful: it just logs a message that there was an error.  (It's named “breakpoint”, I assume, because it's marked noinline so it can be set as a debugger breakpoint.)
Attachment #8964489 - Flags: superreview?(jld)
Attachment #8964489 - Flags: review?(jld)
Attachment #8964489 - Flags: review+
Priority: -- → P2
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7a5b7274d8
Ignore MOZ_CRASH in FatalError() for --enable-fuzzing builds. r=jld
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ee7a5b7274d8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee: nobody → cdiehl
You need to log in before you can comment on or make changes to this bug.