898221 - Wasted work in punycode_decode()

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[pchang9]

Attachment: Suggested patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130116073211

Steps to reproduce:

The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it.

In method punycode_decode() in netwerk/dns/punycode.c, the loop in line 218 keeps overriding "b" with "j" when "delim(input[j])" is true. Therefore, only the last written value is visible out of the loop and all the other writes and iterations are not necessary. The patch iterates from the end of "j" and breaks the first time when "b" is set.

Comment 1

[Patrick McManus [:mcmanus]]

pchang9 - thanks for the contribution.

Nick, can you shepherd this?

Comment 2

[u408661]

(In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #1)
> pchang9 - thanks for the contribution.
> 
> Nick, can you shepherd this?

Absolutely.

pchang9, thanks! I'll take a look at the patch, and run it through our test suite just to be sure. Assuming everything checks out (I don't see why it wouldn't), we'll get this landed in Firefox.

Comment 3

[u408661]

Comment on attachment 781379 [details] [diff] [review]
Suggested patch

Adding myself as reviewer per Patrick's email

Comment 4

[u408661]

Running through try right now: https://tbpl.mozilla.org/?tree=Try&rev=c11ecf46a7b4

Comment 5

[u408661]

Attachment: buildable patch

pchang9 - the original patch didn't compile due to comparing an unsigned value with a signed value (that is one of the many warnings that cause compile errors in mozilla code). I took the liberty of fixing that in the patch, and have pushed the new version to try https://tbpl.mozilla.org/?tree=Try&rev=92bed275dfae The updated version has been uploaded for you to take a look at.

Once this version passes try, we can go ahead and get this patch landed in mozilla-central.

Comment 6

[u408661]

pchang9 - as you can see if you look at the link in comment 5, the patch has passed our tests, and I have landed it on mozilla-inbound for you (the link is below). This should be merged to mozilla-central in a day or so, depending on the sheriff schedule.

Thanks again for the patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/515d75612e53

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/515d75612e53