Closed
Bug 1451295
Opened 6 years ago
Closed 6 years ago
IPC: crash with PWyciwygChannel::Msg_Init [@mozilla::net::WyciwygChannelParent::RecvAppData]
Categories
(Core :: Networking, defect, P2)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: posidron, Assigned: valentin)
References
(Blocks 1 open bug)
Details
(Keywords: crash, Whiteboard: [necko-triaged])
Crash Data
Attachments
(3 files, 1 obsolete file)
The following message was identified to be responsible for this crash and got blacklisted from fuzzing until fixed. Message: PWyciwygChannel::Msg_Init PWyciwygChannel::Msg_WriteToCacheEntry $ hexdiff message.2080.3577.{o,m} $ hexdiff message.2080.3332.{o,m}
Reporter | ||
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
This looks like what would happen if you reordered RecvAppData to be called before RecvInit. Should be straightforward to make RecvAppData handle mChannel not being initialized.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8965248 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•6 years ago
|
Attachment #8965249 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → valentin.gosu
Priority: -- → P2
Whiteboard: [necko-triaged]
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8965248 [details] Bug 1451295 - Add a null check for mChannel https://reviewboard.mozilla.org/r/233952/#review239678 ::: netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:165 (Diff revision 1) > > if (!SetupAppData(loadContext, parent)) > return IPC_FAIL_NO_REASON(this); > > + if (!mChannel) { > + return IPC_OK(); We will fail later in WyciwygChannelParent::RecvWriteToCacheEntry, so why not to fail here?
Attachment #8965248 -
Flags: review?(michal.novotny) → review-
Assignee | ||
Comment 6•6 years ago
|
||
I was trying to follow the same pattern as in RecvAsyncOpen: if (!mChannel) return IPC_OK(); If you think returning IPC_FAIL is better, I'll submit a patch shortly.
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #6) > I was trying to follow the same pattern as in RecvAsyncOpen: > if (!mChannel) > return IPC_OK(); > > If you think returning IPC_FAIL is better, I'll submit a patch shortly. IIUC the report, this can happen only when the messages are reordered by fuzzing, i.e. this shouldn't happen in a real world, so error is more appropriate here.
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8965248 [details] Bug 1451295 - Add a null check for mChannel https://reviewboard.mozilla.org/r/233952/#review239686
Attachment #8965248 -
Flags: review?(michal.novotny) → review+
Comment 10•6 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/798d83c02cc1 Add a null check for mChannel r=michal
Reporter | ||
Comment 11•6 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #8) > IIUC the report, this can happen only when the messages are reordered by > fuzzing, i.e. this shouldn't happen in a real world, so error is more > appropriate here. Jus to make clear, the messages are not re-ordered in the fuzzing process. Only the message content gets mutated.
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/798d83c02cc1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 13•6 years ago
|
||
Looks like this crash does hit in the wild with low frequency. Worth a backport to 60 since it's just an added null check?
Crash Signature: [@ mozilla::net::WyciwygChannelParent::RecvAppData]
status-firefox59:
--- → wontfix
status-firefox60:
--- → fix-optional
status-firefox-esr52:
--- → wontfix
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > Looks like this crash does hit in the wild with low frequency. Worth a > backport to 60 since it's just an added null check? I think it there isn't any risk to do so. I'll request an uplift.
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #14) > (In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > > Looks like this crash does hit in the wild with low frequency. Worth a > > backport to 60 since it's just an added null check? > > I think it there isn't any risk to do so. I'll request an uplift. Actually, Ryan, most of the crashes I've seen are on 52.*.esr + 2 crashes on 57 + 3 crashes on 58. Since this was triggered with fuzzing, it might be an overkill to uplift, since I don't see any crashes on 60.
Flags: needinfo?(ryanvm)
You need to log in
before you can comment on or make changes to this bug.
Description
•