1324995 - Crash in jemalloc_crash | je_free | r_free | stun_get_win32_addrs

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-6d9ebcab-8de4-49c6-8ed9-43b272161221.
=============================================================

#8 crash in 20161219004024 aurora. Total 258 crashes since 2016-12-14
Only 52 was affected.

Stack looks like:
 jemalloc_crash
 je_free
 r_free
 stun_get_win32_addrs
 nr_stun_get_addrs
 nr_stun_find_local_addresses

Byron, this seems webrtc related, can you take a look?

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Bug 1324995: only free AdapterAddresses if needed.

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

Comment 2

[Nils Ohlmeier [:drno]]

I causes this with the new code from bug 1309585. The attached patch should fix it.

Comment 3

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

Comment on attachment 8820581 [details]
Bug 1324995: only free AdapterAddresses if needed.

https://reviewboard.mozilla.org/r/100062/#review100764

drive-by r+

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8820581 [details]
Bug 1324995: only free AdapterAddresses if needed.

https://reviewboard.mozilla.org/r/100062/#review101910

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc2c3de0b1ff
only free AdapterAddresses if needed. r=bwc,jesup

Comment 6

[Nils Ohlmeier [:drno]]

Thanks Ryan :-)

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8820581 [details]
Bug 1324995: only free AdapterAddresses if needed.

Approval Request Comment
[Feature/Bug causing the regression]: The code from bug 1309585 caused this.

[User impact if declined]: Browser can crash on Windows if WebRTC is being used.
 
[Is this code covered by automated tests?]: The code in question should be executed by every single WebRTC mochitest executed on Windows. Most likely our tests did not encounter this crash is that appears to be almost exclusively on Win7 Service Pack 1, plus that regular network interface gathering fails on these machines.

[Has the fix been verified in Nightly?]: No. We don't know exactly how to reproduce it.

[Needs manual test from QE? If yes, steps to reproduce]: See above. Probably only reproducible on a specific version of Win, plus some unknown other factors.

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It only guards a call for free() with an if condition to cover for an early error scenario.

[String changes made/needed]: N/A

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Julien Cristau [:jcristau]]

I don't really understand this patch.  AFAICT RFREE(ptr) is a no-op if ptr is NULL, if https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_memory.h#90 is to be believed.  I wonder if the issue couldn't be when you reach the ABORT at https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#184 you've already freed AdapterAddresses as part of the loop, and then end up double-freeing it at https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#247.  Hmm?  What am I missing?

Comment 10

[Nils Ohlmeier [:drno]]

Thanks Julien. I think you are right.

My theory how this crash happens in a double free (which apparently has undefined behavior) here https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#247
But since the free() operation leaves the pointer untouched the right solution is to reset the pointer after freeing it to prevent the double free.

What I find strange is that this crash apparently only ever happens on 52 and never happened on 53. I have no real explanation for that. Does Nightly have different compiler setting then Aurora which could cause this?

Comment 11

[Nils Ohlmeier [:drno]]

Attachment: Bug 1324995: null pointer after freeing.

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

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8823903 [details]
Bug 1324995: null pointer after freeing.

https://reviewboard.mozilla.org/r/102366/#review102944

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this good to land?

Comment 14

[Nils Ohlmeier [:drno]]

Yes it is. Autoland is currently closed. I'll check back tomorrow to see if it actually landed.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7af5ba471c7
null pointer after freeing. r=bwc

Comment 16

[Nils Ohlmeier [:drno]]

Speaking of the devil... it landed on autoland :-)

Comment 17

[Nils Ohlmeier [:drno]]

Comment on attachment 8823903 [details]
Bug 1324995: null pointer after freeing.

[Feature/Bug causing the regression]: The code from bug 1309585 caused this.

[User impact if declined]: Browser can crash on Windows if WebRTC is being used.
 
[Is this code covered by automated tests?]: The code in question should be executed by every single WebRTC mochitest executed on Windows. Most likely our tests did not encounter this crash because it appears to be almost exclusively on Win7 Service Pack 1, plus that regular network interface gathering fails on these machines.

[Has the fix been verified in Nightly?]: No. We don't know exactly how to reproduce it.

[Needs manual test from QE? If yes, steps to reproduce]: See above. Probably only reproducible on a specific version of Win, plus some unknown other factors.

[List of other uplifts needed for the feature/fix]: N/A

[Is the change risky?]: No.

[Why is the change risky/not risky?]: It only set a pointer to Null after freeing it in an error scenario to not free the same pointer twice later.

[String changes made/needed]: N/A

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8823903 [details]
Bug 1324995: null pointer after freeing.

avoid double free in webrtc code, aurora52+

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/248982f8164b