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)

48 Branch
x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

(Keywords: crash, csectype-nullptr)

Crash Data

Attachments

(1 file)

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 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+
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
https://hg.mozilla.org/mozilla-central/rev/e6e6e6ceb9c9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(docfaraday)
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+
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.