Closed
Bug 1338696
Opened 7 years ago
Closed 7 years ago
Crash in nr_stun_server_process_request (probably caused by failure in nr_stun_message_create2)
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: bwc, Assigned: bwc)
Details
(Keywords: crash, csectype-nullptr)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
drno
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details |
This bug was filed from the Socorro interface and is report bp-04649129-e6af-476d-aea3-72ed32170206. ============================================================= This is happening because |req| is null (0x808 is exactly the offset of |header| within |nr_stun_message| on 32-bit platforms). Failure of nr_stun_message_create2 will cause this, an I can see no other possible cause. We probably just need a null check.
Comment 1•7 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #0) > This is happening because |req| is null (0x808 is exactly the offset of > |header| within |nr_stun_message| on 32-bit platforms). Failure of > nr_stun_message_create2 will cause this, an I can see no other possible > cause. We probably just need a null check. Agreed. Looks like there are two possibilities for nr_stun_messages_create2 to fail: - OOM (probably more likely on 32bit) - received a UDP packet > 2048 bytes
Updated•7 years ago
|
Rank: 17
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8836902 [details] Bug 1338696: Don't deref null when nr_stun_message_create2 fails. https://reviewboard.mozilla.org/r/112212/#review113516 ::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c:342 (Diff revision 1) > - if (NR_STUN_GET_TYPE_CLASS(req->header.type) == NR_CLASS_INDICATION) > + if (!req || NR_STUN_GET_TYPE_CLASS(req->header.type) == NR_CLASS_INDICATION) > goto skip_response; > > if (!res) > goto skip_response; At this point I think it makes sense to combined all of these into one if() check.
Attachment #8836902 -
Flags: review?(drno) → review+
Comment hidden (mozreview-request) |
Pushed by bcampen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e6e6e6ceb9c9 Don't deref null when nr_stun_message_create2 fails. r=drno
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e6e6e6ceb9c9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Comment 7•7 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8836902 [details] Bug 1338696: Don't deref null when nr_stun_message_create2 fails. Approval Request Comment [Feature/Bug causing the regression]: Initial webrtc landing. [User impact if declined]: Rare nullptr crashes. [Is this code covered by automated tests?]: Not in a way that could trigger this bug. [Has the fix been verified in Nightly?]: It has been landed, and this crash has not been observed again, but it is a rare enough crash that we would not expect to have seen one. [Needs manual test from QE? If yes, steps to reproduce]: No. [Is the change risky?]: No. It is a very simple change. [String changes made/needed]: None.
Flags: needinfo?(docfaraday)
Attachment #8836902 -
Flags: approval-mozilla-beta?
Attachment #8836902 -
Flags: approval-mozilla-aurora?
Comment 9•7 years ago
|
||
Comment on attachment 8836902 [details] Bug 1338696: Don't deref null when nr_stun_message_create2 fails. null deref crash fix in webrtc for aurora53 and beta52
Attachment #8836902 -
Flags: approval-mozilla-beta?
Attachment #8836902 -
Flags: approval-mozilla-beta+
Attachment #8836902 -
Flags: approval-mozilla-aurora?
Attachment #8836902 -
Flags: approval-mozilla-aurora+
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/faa3b0ea0123
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/970c3992a4f1
Comment 12•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/970c3992a4f1
status-firefox-esr52:
--- → fixed
Comment 13•7 years ago
|
||
Setting qe-verify- based on Byron's assessment on manual testing needs (see Comment 8) and the rare occurrence rate of this issue.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•