987380 - Crash in NrSocket::~NrSocket()

Status: RESOLVED DUPLICATE of bug 891551

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: crash.txt

If NrSocket::Create() fails early, we can be left trying to PR_Close(0),
which causes a crash in dereferencing fd->methods.

No STR, but I can imagine a number of ways this might happen.

Comment 1

[Eric Rescorla (:ekr)]

Note that fd_ is properly initialized to nullptr at:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h?from=~NrSocket#128

So the fix here is just to add an if(fd_) { } around the PR_Close() in :

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h?from=~NrSocket#130


I marked this as security because I am unsure of our policy around NULL ptr deref.

Comment 2

[Eric Rescorla (:ekr)]

Actually, I do have reported STR, but on a wacky branch and on Windows. I have asked
ehugg to repro.

Hi, EKR, 
  I just get a crash,  the case is:  Firefox branch: gmp7,  openh264 branch: plugin2
    1.  start p2p video call,   but after a while, the video freeze. 
    2.  stop and start video again,    then crash,  please check the crash stack at attachment.

Comment 3

[Eric Rescorla (:ekr)]

ehugg points out not win....

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

guaranteed null pointer deref (unless there's an arbitrary offset that's added, like a string length) is not a sec bug.  If it's random-memory-that-happens-to-be-null, that might be a sec bug.

Comment 5

[Byron Campen [:bwc]]

I should point out that the ICE TCP patch already has a fix for this:

https://bug891551.bugzilla.mozilla.org/attachment.cgi?id=8398368

Comment 6

[Byron Campen [:bwc]]

Closing, since the fix for this landed as part of bug 891551.