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)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: posidron, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [necko-triaged])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file messages.zip
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}
Attached file faulty.txt
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.
Attachment #8965248 - Flags: review?(michal.novotny)
Attachment #8965249 - Attachment is obsolete: true
Assignee: nobody → valentin.gosu
Priority: -- → P2
Whiteboard: [necko-triaged]
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-
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.
(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 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+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/798d83c02cc1
Add a null check for mChannel r=michal
(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.
https://hg.mozilla.org/mozilla-central/rev/798d83c02cc1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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]
Flags: needinfo?(valentin.gosu)
(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)
(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)
Fine with me, you're the expert here :)
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.