371409 - canonicalNum_ fails with 7 hex digits

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Brian Ryner (not reading)]

canonicalNum_ fails to parse a number like 0x1234567, because it takes the last 8 characters so the "x" is the first character.  parseInt() isn't able to parse that.  It should probably substitute \1 from the IS_HEX regexp, which trims off the leading 0x.

Comment 1

[Tony Chang (Google)]

Attachment: fixes to parseIPAddress

This rolls together fixes for this bug, bug 371412, and bug 371414.

Comment 4

[Tony Chang (Google)]

From bug 371412:
When parsing an address like "1.2.3.00x0", parseIPAddress ignores the failure
return from canonicalNum for the last octet, and returns a bogus IP address. 
It should fail immediately so that the hostname remains unchanged.

From bug 371414:
When parsing an IP address like "12.34.56.78 xy", we should ignore the space
and everything after it.  This is to match what Windows does when resolving the
host.

The exact test should be something like this:

if (host.length <= 15 && (host matches "^(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})
") {
  host = (match);
}

before testing against POSSIBLE_IP.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 256435 [details] [diff] [review]
fixes to parseIPAddress

>Index: tests/test_bug371409.xhtml

It seems like it would be clearer to have all of the IP address parsing tests together, rather than creating a new test file for each bug.  But if this is generally the convention for unit tests, I guess it's ok.

Looks good.

Comment 6

[Tony Chang (Google)]

On trunk.  I moved the tests into test_bug356355.xhtml file to be close to the other tests as you mentioned.  There's no real convention here.

Checking in content/enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.11; previous revision: 1.10
done
Checking in tests/test_bug356355.xhtml;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/test_bug356355.xhtml,v  <--  test_bug356355.xhtml
new revision: 1.4; previous revision: 1.3
done

Comment 7

[Tony Chang (Google)]

Attachment: branch patch

same as above except for branch (no mochitests)

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 256508 [details] [diff] [review]
branch patch

This is missing the new files.

Comment 9

[Tony Chang (Google)]

Which new files?

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 256508 [details] [diff] [review]
branch patch

Oops, sorry I was thinking of your other patch.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 256508 [details] [diff] [review]
branch patch

approved for 1.8 branch, a=dveditz for drivers

Comment 12

[Tony Chang (Google)]

on branch

Checking in content/enchash-decrypter.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/enchash-decrypter.js,v  <--  enchash-decrypter.js
new revision: 1.1.2.6; previous revision: 1.1.2.5
done