Crash in nr_stun_server_process_request (probably caused by failure in nr_stun_message_create2)

RESOLVED FIXED in Firefox 52

Status

()

P1
normal
Rank:
17
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({crash, csectype-nullptr})

48 Branch
mozilla54
x86
Windows 10
crash, csectype-nullptr
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(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
Rank: 17
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 3

2 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)

Comment 5

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6e6e6ceb9c9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
status-firefox52: --- → affected
status-firefox53: --- → affected
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(docfaraday)
(Assignee)

Comment 8

2 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 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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/faa3b0ea0123
status-firefox53: affected → fixed

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/970c3992a4f1
status-firefox52: affected → fixed

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/970c3992a4f1
status-firefox-esr52: --- → fixed
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.