Closed
Bug 1450231
Opened 6 years ago
Closed 6 years ago
Ignore MOZ_CRASH in FatalError() for --enable-fuzzing builds
Categories
(Core :: IPC, enhancement, P2)
Core
IPC
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)
1.15 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #8963901 -
Flags: superreview?(jld)
Attachment #8963901 -
Flags: review?(jld)
Comment 1•6 years ago
|
||
I think we should also disable the |ProtocolErrorBreakpoint| at the top of |FatalError|.
Assignee | ||
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
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+
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
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
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee7a5b7274d8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Assignee: nobody → cdiehl
You need to log in
before you can comment on or make changes to this bug.
Description
•