Closed Bug 972592 Opened 8 years ago Closed 8 years ago

Improve debugabilty of ICE error responses

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(1 file, 1 obsolete file)

The nICEr code returns the identical "400 Bad Request" response for a few different errors.
To help with debugging network problems we should make the response more verbose.
Assignee: nobody → docfaraday
Assignee: docfaraday → drno
Attachment #8375821 - Flags: review?(docfaraday)
Comment on attachment 8375821 [details] [diff] [review]
improve_ice_400_error_strings.patch

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

r-, since some of the response strings aren't accurate. Also some nits.

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ +249,5 @@
>      if ((r=nr_stun_message_create(&res)))
>          ABORT(r);
>  
>      if ((r=nr_stun_decode_message(req, nr_stun_server_get_password, ctx))) {
> +        /* RFC5389 S 7,3 says "If any errors are detected, the message is 

Generally, folks are sensitive to trailing whitespace. When in Rome...

Also, your sneaky european substitution of ',' for '.' in the section number almost got by me. :)

@@ +255,4 @@
>  #ifndef USE_STUN_PEDANTIC
>          /* ... but that seems like a bad idea, at least return a 400 so
>           * that the server isn't a black hole to the client */
> +        nr_stun_form_error_response(req, res, 400, "Bad Request - Bad Password");

I don't think this has anything to do with password; looking at the impl of nr_stun_decode_message, this is all basic well-formedness checks.

@@ +262,5 @@
>      }
>  
>      if ((r=nr_stun_receive_message(0, req))) {
> +        /* RFC5389 S 7,3 says "If any errors are detected, the message is 
> +         * silently discarded."  */

WS here.

@@ +267,4 @@
>  #ifndef USE_STUN_PEDANTIC
>          /* ... but that seems like a bad idea, at least return a 400 so
>           * that the server isn't a black hole to the client */
> +        nr_stun_form_error_response(req, res, 400, "Bad Request - Missing Request");

Looking at nr_stun_receive_message, there are lots of things that can go wrong that are not accurately described by this. Sadly, we cannot get any of that info out yet.

@@ +275,5 @@
>  
>      if (NR_STUN_GET_TYPE_CLASS(req->header.type) != NR_CLASS_REQUEST
>       && NR_STUN_GET_TYPE_CLASS(req->header.type) != NR_CLASS_INDICATION) {
>           r_log(NR_LOG_STUN,LOG_WARNING,"STUN-SERVER(%s): Illegal message type: %04x",ctx->label,req->header.type);
> +        /* RFC5389 S 7,3 says "If any errors are detected, the message is 

WS here.
Attachment #8375821 - Flags: review?(docfaraday) → review-
Status: NEW → ASSIGNED
Attachment #8375821 - Attachment is obsolete: true
Addressed nits, and tried to improve error strings.
Attachment #8375975 - Flags: review?(docfaraday)
Comment on attachment 8375975 [details] [diff] [review]
improve_ice_400_error_strings.patch

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

Looks good to me.
Attachment #8375975 - Flags: review?(docfaraday) → review+
Even though the patch changes strings only to be safe a try run: https://tbpl.mozilla.org/?tree=Try&rev=c00d85e04c1e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31806aaa5ab2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.