856382 - WebRTC TURN use-after-free crash [@nr_turn_client_cancel]

Status: RESOLVED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

alloc: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:309

  if(!(ctx->label=r_strdup(label)))
    ABORT(R_NO_MEMORY);



free: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:355

static int nr_turn_client_failed(nr_turn_client_ctx *ctx)
{
  r_log(NR_LOG_TURN, LOG_ERR, "TURN(%s) failed", ctx->label);
  nr_turn_client_cancel(ctx);

[...]

int nr_turn_client_cancel(nr_turn_client_ctx *ctx)
{
  if (ctx->label)
    r_log(NR_LOG_TURN, LOG_ERR, "TURN(%s): cancelling", ctx->label);

  RFREE(ctx->label);



re-use: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:355

int
nr_turn_client_ctx_destroy(nr_turn_client_ctx **ctxp)
{
  nr_turn_client_ctx *ctx;

  if(!ctxp || !*ctxp)
    return(0);

  ctx=*ctxp;
  *ctxp = 0;

  nr_turn_client_cancel(ctx);

[...]

int nr_turn_client_cancel(nr_turn_client_ctx *ctx)
{
  if (ctx->label)
    r_log(NR_LOG_TURN, LOG_ERR, "TURN(%s): cancelling", ctx->label);

  RFREE(ctx->label);



To reproduce run: media/mtransport/test/turn_unittest


[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from TurnClient
[ RUN      ] TurnClient.Setup
[       OK ] TurnClient.Setup (1 ms)
[ RUN      ] TurnClient.Allocate
/Users/cdiehl/dev/repos/mozilla/mozilla-inbound/media/mtransport/test/turn_unittest.cpp:163: Failure
Value of: res
  Actual: false
Expected: true


Tested with m-i changeset: 126761:c99f1fd792db and the applied patch of bug 786235.

Comment 1

[Eric Rescorla (:ekr)]

This should be fixed in more recent patches.

Comment 2

[Eric Rescorla (:ekr)]

(which have not yet been uploaded).

Comment 3

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

Not blocking yet until we know if TURN is going to release in 22

Comment 4

[Eric Rescorla (:ekr)]

cdiehl: please see if you can repro this with the new patches.

Comment 5

[Christoph Diehl [:posidron]]

Fixed, can not reproduce it anymore.

Comment 6

[Jason Smith [:jsmith]]

(In reply to Christoph Diehl [:cdiehl] from comment #5)
> Fixed, can not reproduce it anymore.

Should we close this bug then? Or is there some other patch within a different bug that needs to land first?

Comment 7

[Christoph Diehl [:posidron]]

I think we can close it, ekr fixed the issue in his updated TURN patch and since then I can't reproduce it anymore.

Comment 8

[Alex Keybl [:akeybl]]

Sounds like FF23 is fixed, but do we need to take any action for FF22?

Comment 9

[Adam Roach [:abr]]

(In reply to Alex Keybl [:akeybl] from comment #8)
> Sounds like FF23 is fixed, but do we need to take any action for FF22?

TURN has not been uplifted into FF22 yet, so it is unaffected.

Comment 10

[Jason Smith [:jsmith]]

This was a bug on a patch that hasn't landed, so this should be unaffected on Fx23 as well.

Comment 11

[Daniel Veditz [:dveditz]]

weeeel, we did apply a patch, and that patch will be part of the code that lands on Fx23. If we were to put a regression test into the tree in Fx23 it would be making sure a fix did not regress

does in-testsuite-minus mean simply there isn't a test checked in, or that we don't want one? When isn't a regression test a decent idea?

Comment 12

[Jason Smith [:jsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #11)
> weeeel, we did apply a patch, and that patch will be part of the code that
> lands on Fx23. If we were to put a regression test into the tree in Fx23 it
> would be making sure a fix did not regress
> 
> does in-testsuite-minus mean simply there isn't a test checked in, or that
> we don't want one? When isn't a regression test a decent idea?

I put the in-testsuite- probably because there isn't an explicit test needed to be added here - this was reusing the TURN unit tests that already exist.