1469879 - UDPSocketParent::RecvJoinMulticast and UDPSocketParent::RecvLeaveMulticast do not handle mSocket not being set

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[Alex Gaynor [:Alex_Gaynor]]

https://searchfox.org/mozilla-central/source/dom/network/UDPSocketParent.cpp#449-473

These two functions will die on a null-ptr-deref if |mSocket| hasn't been sent. The other IPC methods do handle this.

The IPC fuzzer has discovered this :-)

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1469879 - Add check for closed socket on Multicast IPC parent funcs;

Review commit: https://reviewboard.mozilla.org/r/252938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252938/

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Taking 'cause I was doing triage anyways and this looks straightforward. (Famous last words...)

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8987699 [details]
Bug 1469879 - Add check for closed socket on Multicast IPC parent funcs;

https://reviewboard.mozilla.org/r/252938/#review259534

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca98b6f47b4e
Add check for closed socket on Multicast IPC parent funcs; r=baku

Comment 5

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ca98b6f47b4e

Comment 6

[Pascal Chevrel:pascalc]

(In reply to Andrea Marchesini [:baku] from comment #3)
> Comment on attachment 8987699 [details]
> Bug 1469879 - Add check for closed socket on Multicast IPC parent funcs;
> 
> https://reviewboard.mozilla.org/r/252938/#review259534

Andrea, do you feel this is safe to uplift to Beta or should we just let it ride the trains? Thanks

Comment 7

[Andrea Marchesini [:baku]]

Yes, this patch is safe to be uplifted. It introduces a null-check in a couple of IPC methods.
qdot, do you mind to ask for the uplift?

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8987699 [details]
Bug 1469879 - Add check for closed socket on Multicast IPC parent funcs;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 745283
[User impact if declined]: Since the fuzzer caught it, could cause a bad memory access.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not sure if fuzzer has been rerun after this
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: N/A
[String changes made/needed]: None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8987699 [details]
Bug 1469879 - Add check for closed socket on Multicast IPC parent funcs;

Fix for a crash, looks low risk, let's uplift for beta 6.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a1992e3234f4