886120 - Make ICE respond before receiving peer credentials

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 2

[Eric Rescorla (:ekr)]

This is as required by S 7.2

Comment 3

[Eric Rescorla (:ekr)]

https://tbpl.mozilla.org/?tree=Try&rev=2ab2f8809dbc

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 5

[Eric Rescorla (:ekr)]

Attached new patch that fixes errors discovered on try.

Windows error was a pre-C99/C99 issue.
Linux was a defect introduced by too aggressive cleanup.

https://tbpl.mozilla.org/?tree=Try&rev=c96073fd7da9

Comment 6

[Adam Roach [:abr]]

Comment on attachment 767188 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

r- for assert()s that aren't backed by exception handling code for non-debug builds. There's also one minor bug in nr_ice_peer_ctx_start_checks2(). Everything else is nits.

ice_component.c:286: tab character
ice_component.c:287: tab character

::: media/mtransport/test/ice_unittest.cpp
@@ +588,5 @@
> +  ConnectP2();
> +  PR_Sleep(1000);
> +  ConnectP1(TRICKLE_DEFERRED);
> +  DoTrickleP1(0);
> +  std::cerr << "Sleeping between trickle streams" << std::endl;

Is there any rationale about std::cerr versus std::cout in this test driver? Output seems to be pretty evenly split between the two.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +48,5 @@
>  
> +static int nr_ice_component_stun_server_default_cb(void *cb_arg,nr_stun_server_ctx *stun_ctx,nr_socket *sock, nr_stun_server_request *req, int *dont_free, int *error);
> +static int nr_ice_pre_answer_request_destroy(nr_ice_pre_answer_request **parp);
> +
> +static int nr_ice_pre_answer_request_create(nr_socket *sock, nr_stun_server_request *req, nr_ice_pre_answer_request **parp)

Please add a comment above this function warning that it assumes ownership of req->request and req->response upon success.

Also, consider setting req->request and req->response to 0 to make this code more resilient to someone botching this ownership transfer in the future.

@@ +285,5 @@
> +      /* Add the default STUN credentials so that we can respond before
> +	 we hear about the peer. Note: we need to recompute these because
> +	 we have not yet computed the values in the peer media stream.*/
> +      lufrag=component->stream->ufrag ? component->stream->ufrag : ctx->ufrag;
> +      assert(lufrag);

if (!lufrag) { ABORT(R_INTERNAL); }

@@ +287,5 @@
> +	 we have not yet computed the values in the peer media stream.*/
> +      lufrag=component->stream->ufrag ? component->stream->ufrag : ctx->ufrag;
> +      assert(lufrag);
> +      lpwd=component->stream->pwd ? component->stream->pwd :ctx->pwd;
> +      assert(lpwd);

if (!lpwd) { ABORT(R_INTERNAL); }

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +289,5 @@
>    abort:
>      return(_status);
>    }
>  
> +int nr_ice_media_stream_service_pre_answer_requests(nr_ice_peer_ctx *pctx,nr_ice_media_stream *lstream,nr_ice_media_stream *pstream, int *serviced)

The predominant (although not uniform) style in this file appears to be spaces after commas in function declarations.

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +399,1 @@
>          ABORT(R_FAILED);

I don't think this abort is quite right any more, since it's going to skip over our check for a pre-answer stream. You probably want to turn it into "stream = NULL; break;".

@@ +425,5 @@
> +      stream=STAILQ_NEXT(stream, entry);
> +    }
> +
> +    if (!started)
> +      ABORT(R_NOT_FOUND);

I think we want logging here -- something to indicate that we couldn't find a pre-answer stream either.

::: media/mtransport/third_party/nICEr/src/stun/stun_build.c
@@ +244,5 @@
> +       if ((r=nr_stun_message_add_ice_controlling_attribute(req, params->tiebreaker)))
> +           ABORT(r);
> +       break;
> +   case NR_ICE_CONTROLLED:
> +       assert(0);  /* Can't happen */

ABORT(R_INTERNAL);

Also, this should probably be clearer about why it can't happen; something like "Forbidden by RFC 5245, section 7.1.2.1"

@@ +247,5 @@
> +   case NR_ICE_CONTROLLED:
> +       assert(0);  /* Can't happen */
> +       break;
> +   default:
> +       assert(0);

ABORT(R_INTERNAL);

@@ +286,5 @@
>         if ((r=nr_stun_message_add_ice_controlled_attribute(req, params->tiebreaker)))
>             ABORT(r);
>         break;
> +   default:
> +       assert(0);

ABORT(R_INTERNAL);

::: media/mtransport/third_party/nICEr/src/stun/stun_build.h
@@ +80,5 @@
>      Data password;
>      UINT4 priority;
> +    int control;
> +#define NR_ICE_CONTROLLING  1
> +#define NR_ICE_CONTROLLED   2

These two constants end up being multiply defined (since they're defined down on lines 96 and 97 as well). Please don't do that.

It's also not clear why we need to add this field here, since USE-CANDIDATE only makes sense for CONTROLLING. If there's some rationale for adding the "control" field, please add it as a comment.

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ +442,5 @@
>      }
> +
> +    if (!clnt && ctx->default_client) {
> +      /* If we can't find a specific client see if this matches the default,
> +         which means that the username is our ufrag.

"...starts with our ufrag..."

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.h
@@ +50,5 @@
>  struct nr_stun_server_client_ {
>    char *label;
>    char *username;
>    Data password;
> +  int (*stun_server_cb)(void *cb_arg, nr_stun_server_ctx *ctx,nr_socket *sock, nr_stun_server_request *req, int *dont_free, int *error);

This should use the nr_stun_server_cb typedef -- it makes it both easier to read and less trouble to change if you need to update the signature in the future.

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 767188 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

::: media/mtransport/test/ice_unittest.cpp
@@ +588,5 @@
> +  ConnectP2();
> +  PR_Sleep(1000);
> +  ConnectP1(TRICKLE_DEFERRED);
> +  DoTrickleP1(0);
> +  std::cerr << "Sleeping between trickle streams" << std::endl;

I am not aware of any. Maybe a pass with sed is in order?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +285,5 @@
> +      /* Add the default STUN credentials so that we can respond before
> +	 we hear about the peer. Note: we need to recompute these because
> +	 we have not yet computed the values in the peer media stream.*/
> +      lufrag=component->stream->ufrag ? component->stream->ufrag : ctx->ufrag;
> +      assert(lufrag);

I can add these, but there's supposedly no way for this to happen
which is why they have asserts.

::: media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c
@@ +399,1 @@
>          ABORT(R_FAILED);

Here's my reasoning.

This test applies if:

1. allow_non_first is 0 (i.e., non-trickle ICE)
2. the first stream has an empty check list.

But in the non-trickle ICE case, the other side should have provided some
candidates or ICE is pretty much not going to work and we're just going
to fail. Hence R_FAILED as opposed to R_NOT_FOUND.

@@ +425,5 @@
> +      stream=STAILQ_NEXT(stream, entry);
> +    }
> +
> +    if (!started)
> +      ABORT(R_NOT_FOUND);

OK.

::: media/mtransport/third_party/nICEr/src/stun/stun_build.h
@@ +80,5 @@
>      Data password;
>      UINT4 priority;
> +    int control;
> +#define NR_ICE_CONTROLLING  1
> +#define NR_ICE_CONTROLLED   2

I think it was just laziness. I will fix.

Comment 8

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 773042 [details] [diff] [review]
Make ICE respond before receiving peer credentials

WIP. There is a problem with unit tests,

Comment 10

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 773456 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

Adam

This passes my local unit tests. Will push a try shortly once I beat hg into
submission,

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 773456 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

Adam

This passes my local unit tests. Will push a try shortly once I beat hg into
submission,

https://tbpl.mozilla.org/?tree=Try&rev=6d933c333927

Comment 13

[Adam Roach [:abr]]

Comment on attachment 773456 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

r+ with tiny nits in the unit test.

signaling_unittests.cpp:2293: tab character

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +2282,5 @@
>    // Need to create an offer, since that's currently required by our
>    // FSM. This may change in the future.
>    a1_.CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);
>    a1_.SetLocal(TestObserver::OFFER, offer, true);
> +  // Really we should detect failure at the SetRemove point,

s/SetRemove/SetRemote/

@@ +2284,5 @@
>    a1_.CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);
>    a1_.SetLocal(TestObserver::OFFER, offer, true);
> +  // Really we should detect failure at the SetRemove point,
> +  // since without a ufrag, we aren't going to be successful.
> +  // But for now ,this isn't detected till SIPCC tries to impose

s/ ,/, /

@@ +2285,5 @@
>    a1_.SetLocal(TestObserver::OFFER, offer, true);
> +  // Really we should detect failure at the SetRemove point,
> +  // since without a ufrag, we aren't going to be successful.
> +  // But for now ,this isn't detected till SIPCC tries to impose
> +  // the parameters on the ICE stack in SetLocal.

Bug #?

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 15

[Eric Rescorla (:ekr)]

Attachment: Make ICE respond before receiving peer credentials

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 773960 [details] [diff] [review]
Make ICE respond before receiving peer credentials

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

abr: if you are happy with htis, please re-r+ and checkin.

Comment 17

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00cf54e2dc61

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00cf54e2dc61