Closed
Bug 838169
Opened 12 years ago
Closed 12 years ago
WebRTC use-after-free crash [@mozilla::NrIceCtx::Release]
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: posidron, Assigned: ekr)
References
Details
(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-])
Attachments
(2 files, 3 obsolete files)
11.53 KB,
text/plain
|
Details | |
12.07 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
This happened while fuzzing the JSEP and might relate to bug 835835.
alloc: nricectx.cpp
RefPtr<NrIceCtx> ctx = new NrIceCtx(name, offerer);
free: nricectx.h:189
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrIceCtx)
stun_client_ctx.c:658
[..]
/* Fire the callback */
if (ctx->finished_cb) {
NR_async_cb finished_cb = ctx->finished_cb;
ctx->finished_cb = 0; /* prevent 2nd call */
/* finished_cb call must be absolutely last thing in function
* because as a side effect this ctx may be operated on in the
* callback */
finished_cb(0,0,ctx->cb_arg);
[..]
re-use: nricectx.h:189
NS_INLINE_DECL_THREADSAFE_REFCOUNTING(NrIceCtx)
Tested with m-i changeset: 120856:66efdc5f9355 -O1
Updated•12 years ago
|
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc+]
Assignee | ||
Comment 1•12 years ago
|
||
Something is seriously wrong with this callstack.
The relevant line of C code where this is allegedly called is:
finished_cb(0,0,ctx->cb_arg);
And this comes from ctx->finished_cb.
In any case, this is C code, so it's pretty implausible that it's calling C++ member
functions from NrIceCtx.
Can you get this in the debugger?
Updated•12 years ago
|
status-b2g18:
--- → disabled
status-firefox19:
--- → disabled
status-firefox20:
--- → disabled
status-firefox21:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-firefox21:
--- → +
Updated•12 years ago
|
Flags: needinfo?(cdiehl)
Reporter | ||
Comment 2•12 years ago
|
||
ekr, please run the testcase in bug 837324 with an ASan -O1 build.
Flags: needinfo?(cdiehl)
Assignee | ||
Comment 3•12 years ago
|
||
I ran it with an ASan and --disable-optimize and no joy.
Please provide the set of ASan flags you want me to use.
Reporter | ||
Comment 4•12 years ago
|
||
As said with -O1 ;)
ac_add_options --enable-optimize="-O1 -g"
Updated•12 years ago
|
Assignee: nobody → ekr
Assignee | ||
Updated•12 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused]
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #712332 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
cdiehl:
Can you test with the above patch? It removes the crash on my system with the above repro case.
Flags: needinfo?(cdiehl)
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #712333 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #712346 -
Flags: review?(adam)
Assignee | ||
Comment 10•12 years ago
|
||
Adam,
There are a number of changes here, some defects, some not:
1. Make sure that PCImpl::mSTSThread is set even if PCMedia::Init fails.
2. Have nr_stun_client_ctx_destroy() zero the pointer it was asked to destroy so that you can call it twice in a row safely (this now can happen).
3. Have nr_ice_candidate_destroy() call nr_stun_client_ctx_destroy)
4. Remove weird refcting hacks in NrIceCtx because they aren't needed with change #3.
Comment 11•12 years ago
|
||
Comment on attachment 712346 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx
Review of attachment 712346 [details] [diff] [review]:
-----------------------------------------------------------------
This looks okay to me, modulo the callback issue we discussed on IRC. I'm leaving it as 'r?' pending the resolution of that issue (detailed below).
::: media/mtransport/nricectx.cpp
@@ -388,2 @@
> int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb,
> this);
Now that we're not using a reference increment to keep the NrIceCtx alive, I think we can run into situations where the underlying nr_ice_ctx still has callbacks pending on it after we've been destroyed. I think we need nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when invoking it) so that any event that happens between nr_ice_ctx_destroy and nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx.
::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +669,5 @@
> if(!ctxp || !*ctxp)
> return(0);
>
> ctx=*ctxp;
> + *ctxp=0;
Is there (expected to be) any consistency with how the nICEr dtors handle the passed-in pointer WRT whether it gets zeroed out? It seems to be somewhat arbitrary at the moment.
Comment 12•12 years ago
|
||
(In reply to Adam Roach [:abr] from comment #11)
> ::: media/mtransport/nricectx.cpp
> @@ -388,2 @@
> > int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb,
> > this);
>
> Now that we're not using a reference increment to keep the NrIceCtx alive, I
> think we can run into situations where the underlying nr_ice_ctx still has
> callbacks pending on it after we've been destroyed. I think we need
> nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when
> invoking it) so that any event that happens between nr_ice_ctx_destroy and
> nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx.
Do these callbacks run on the same thread that would zero out the callback pointer (in ns_ice_ctx_destroy()?
If so, please add asserts for that. If not, then locking will be needed.
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #12)
> (In reply to Adam Roach [:abr] from comment #11)
>
> > ::: media/mtransport/nricectx.cpp
> > @@ -388,2 @@
> > > int r = nr_ice_initialize(ctx_, &NrIceCtx::initialized_cb,
> > > this);
> >
> > Now that we're not using a reference increment to keep the NrIceCtx alive, I
> > think we can run into situations where the underlying nr_ice_ctx still has
> > callbacks pending on it after we've been destroyed. I think we need
> > nr_ice_ctx_destroy to zero out its callback pointer (and check for zero when
> > invoking it) so that any event that happens between nr_ice_ctx_destroy and
> > nr_ice_ctx_destroy_cb doesn't try to call into (the now destroyed) NrIceCtx.
>
> Do these callbacks run on the same thread that would zero out the callback
> pointer (in ns_ice_ctx_destroy()?
Of course.
> If so, please add asserts for that. If not, then locking will be needed.
Not really practical, since this is all in nICEr and it has no real notion
of threads.
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #712932 -
Flags: review?(adam)
Assignee | ||
Updated•12 years ago
|
Attachment #712346 -
Attachment is obsolete: true
Attachment #712346 -
Flags: review?(adam)
Comment 15•12 years ago
|
||
Comment on attachment 712932 [details] [diff] [review]
Fix nricectx cleanup
Review of attachment 712932 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me.
Attachment #712932 -
Flags: review?(adam) → review+
Assignee | ||
Comment 16•12 years ago
|
||
Comment 17•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-]
Updated•12 years ago
|
Whiteboard: [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-] → [WebRTC],[blocking-webrtc+],[webrtc-asan-confused][qa-][adv-main21-]
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•