928060 - Parse ?transport=[udp|tcp] in TURN uri

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Parse query-string in TURN URI by hand

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Parse query-string in TURN URI by hand (2)

Wrong query separator.

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

This is generally the kind of thing I expected, but I don't understand all
the subtleties of URL parsing. Can you get whoever reviewed the initial
code to review?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +601,4 @@
>        int32_t questionmark = path.FindChar('?');
>        if (questionmark >= 0) {
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);

sizeof(char) == 1, so you can just do sizeof(match) -1

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

(In reply to Eric Rescorla (:ekr) from comment #2)
> This is generally the kind of thing I expected, but I don't understand all
> the subtleties of URL parsing. Can you get whoever reviewed the initial
> code to review?

That was bz (Bug 825703, Comment 30) though he's on vacation. Someone on IRC suggested annevk, and jesup has worked on nsURI AFAIK. Trying them.

FWIW, I looked at nsSimpleURI uses, and it looks like most code uses GetQuery() and does their own parsing anyways.

Reviewers: Bug 825703 Comment 43 has good info on spec.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +601,4 @@
> >        int32_t questionmark = path.FindChar('?');
> >        if (questionmark >= 0) {
> > +        const char match[] = "transport=";
> > +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> 
> sizeof(char) == 1, so you can just do sizeof(match) -1

Sure (I'm just used to having available a #define lengthof(a) (sizeof(a)/sizeof(*a))

Comment 4

[Anne (:annevk)]

Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

I write http://url.spec.whatwg.org/ No C++.

However, if you simply search for "?", what happens for "...#?transport=udp"? Also, do you want to support "?tr%61nsport=udp"? If you want to follow HTML conventions here you really ought to use http://url.spec.whatwg.org/#application/x-www-form-urlencoded

Comment 5

[(no longer active)]

Comment on attachment 818643 [details] [diff] [review]
Parse query-string in TURN URI by hand (2)

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +585,5 @@
>      nsAutoCString spec;
>      rv = url->GetSpec(spec);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    // TODO([e-mail]): Revisit once nsURI has STUN/TURN support (Bug 833509)

Please document why this is a simple URI and not a standard URI explicitly. It's a bit weird to have a simple URI and then try to parse out its query string.

@@ +601,4 @@
>        int32_t questionmark = path.FindChar('?');
>        if (questionmark >= 0) {
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);

Or, use mozilla::ArrayLength.

@@ +603,5 @@
> +        const char match[] = "transport=";
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);

Let's not try to handle ';'.

@@ +604,5 @@
> +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);

Please assign the return value to an nsDependentCString.  You're creating a reference to a temporary returned by Substring.  I don't even know why this compiles. :(

@@ +605,5 @@
> +
> +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {

Hmm, why not use StringBeginsWith?

@@ +607,5 @@
> +          endPos = path.FindCharInSet("&;", i + 1);
> +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {
> +            transport = Substring(fieldvaluepair, matchlen);
> +            ToLowerCase (transport);

Nit: whitespace before parentheses.

@@ +636,5 @@
>        if (!aDst->addTurnServer(host.get(), port,
>                                 username.get(),
> +                               credential.get(),
> +                               (transport.IsEmpty() ?
> +                                kNrIceTransportUdp : transport.get()))) {

This is a matter of style, but I'd have the function return kNrIceTransportUdp in its transport argument if it cannot find the transport in the query part of the URI, but it's your call which way you want to go.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Parse query-string in TURN URI by hand (3)

Sorry, got side-tracked today.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > +    // TODO([e-mail]): Revisit once nsURI has STUN/TURN support (Bug 833509)
> 
> Please document why this is a simple URI and not a standard URI explicitly.
> It's a bit weird to have a simple URI and then try to parse out its query
> string.

Will do.

> >        int32_t questionmark = path.FindChar('?');
> >        if (questionmark >= 0) {
> > +        const char match[] = "transport=";
> > +        const size_t matchlen = (sizeof(match)-1)/sizeof(*match);
> 
> Or, use mozilla::ArrayLength.

Ah, that's what it's called, thanks!

> > +          endPos = path.FindCharInSet("&;", i + 1);
> 
> Let's not try to handle ';'.

Agreed.

> > +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> > +          endPos = path.FindCharInSet("&;", i + 1);
> > +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> 
> Please assign the return value to an nsDependentCString.  You're creating a
> reference to a temporary returned by Substring.  I don't even know why this
> compiles. :(

That is right off the wiki! https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide#Substrings_.28string_fragments.29 :-)

> > +        for (int32_t i = questionmark, endPos; i >= 0; i = endPos) {
> > +          endPos = path.FindCharInSet("&;", i + 1);
> > +          const nsCSubstring &fieldvaluepair = Substring(path, i + 1, endPos);
> > +          if (StringHead(fieldvaluepair, matchlen).EqualsLiteral(match)) {
> 
> Hmm, why not use StringBeginsWith?

Nice!

> >        if (!aDst->addTurnServer(host.get(), port,
> >                                 username.get(),
> > +                               credential.get(),
> > +                               (transport.IsEmpty() ?
> > +                                kNrIceTransportUdp : transport.get()))) {
> 
> This is a matter of style, but I'd have the function return
> kNrIceTransportUdp in its transport argument if it cannot find the transport
> in the query part of the URI, but it's your call which way you want to go.

I would too, except here kNrIceTransportUdp is actually a std::string from a deeper include file, so this way turned out nicer (less copying back and forth between std::string and nsString).

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Parse query-string in TURN URI by hand (4). r=ehsan

Rebased to land before Bug 928060. Carrying forward r+ from ehsan.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/50ad84fae401

Comment 9

[Mike Hommey [:glandium]]

This added 9 static initializers. Putting global variables in a header is not the best idea ever. Using a string to select between tcp and udp is not the best idea ever either.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

My eagerness to land backfired. I'll wait for reviews on bug 906968 to resolve nricectx.h

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/50ad84fae401