838169 - WebRTC use-after-free crash [@mozilla::NrIceCtx::Release]

Status: RESOLVED FIXED

Description

[Christoph Diehl [:posidron]]

Created attachment 710199 [details]
callstack

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

Comment 1

[Eric Rescorla (:ekr)]

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?

Comment 2

[Christoph Diehl [:posidron]]

ekr, please run the testcase in bug 837324 with an ASan -O1 build.

Comment 3

[Eric Rescorla (:ekr)]

I ran it with an ASan and --disable-optimize and no joy.

Please provide the set of ASan flags you want me to use.

Comment 4

[Christoph Diehl [:posidron]]

As said with -O1 ;)

ac_add_options --enable-optimize="-O1 -g"

Comment 5

[Eric Rescorla (:ekr)]

Created attachment 712332 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx

Comment 6

[Eric Rescorla (:ekr)]

Created attachment 712333 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx

Comment 7

[Eric Rescorla (:ekr)]

cdiehl:

Can you test with the above patch? It removes the crash on my system with the above repro case.

Comment 8

[Christoph Diehl [:posidron]]

Looks fixed, can't reproduce it anymore.

Comment 9

[Eric Rescorla (:ekr)]

Created attachment 712346 [details] [diff] [review]
Fix FMR and multiple release in NrIceCtx

Comment 10

[Eric Rescorla (:ekr)]

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

[Adam Roach [:abr]]

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

[Randell Jesup [:jesup]]

(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.

Comment 13

[Eric Rescorla (:ekr)]

(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.

Comment 14

[Eric Rescorla (:ekr)]

Created attachment 712932 [details] [diff] [review]
Fix nricectx cleanup

Comment 15

[Adam Roach [:abr]]

Comment on attachment 712932 [details] [diff] [review]
Fix nricectx cleanup

Review of attachment 712932 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.

Comment 16

[Eric Rescorla (:ekr)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e870235b44ce

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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