809609 - allow the full range of IPv4 address formats in isLegalIPv4Address(), e.g. those without 3 dots.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #80855 comment 74 +++

[e-mail] 2012-07-08 17:48:58 CEST

*Rather like IPv6 not requiring all components (::0:: assumed), IPv4 does not require all components either. In this case, the last component can be a larger integer to make up the missing bytes. Decimal, octal and hex are all legal. Some examples in hex: 0xC0.0xA8.0x90.0x78 can be written 0xC0.0xA8.0x9078 or 0xC0.0xA89078 or 0xC0A89078 and they are all the same address.

Comment 1

[:aceman]

From bug 810763:
[e-mail] 2013-04-07 19:25:11 CEST

Comment on attachment 734291 [details] [diff] [review] [diff] [review]
test for 80855

Seems reasonable, but do you want to add things like 127.1 and 10.100.1000 as valid extended IP addresses?

Comment 2

[:aceman]

Let's not forget to add those new formats to the test in mailnews/base/test/unit/test_hostnameUtils.js .

Comment 3

[:aceman]

Attachment: patch

Apply on top of patch in bug 810763.

Comment 4

[[e-mail]]

Comment on attachment 734766 [details] [diff] [review]
patch

>+  if ((componentCount != 4 && !aAllowExtendedIPFormats) || componentCount > 4)
>+    return null;
Not 100% sure about this. Perhaps if ((componentCount < 4 && !aAllowExtendedIPFormats) || componentCount > 4) or if (aAllowExtendedIPFormats ? componentCount > 4 : componentCount != 4)

>+    let component = 0;
Nit: = 0 is unnecessary.

>+    // Make sure the component in not larger then the expected maximum.
>+    if (component > Math.pow(256, aWidth) - 1)
if ((component >>> ((aWidth - 1) * 8)) >= 256) perhaps?

Comment 5

[[e-mail]]

(mostly to avoid Math.pow)

Comment 6

[:aceman]

OK, if I am allowed to put a comment there what that crazy thing does ;)

Comment 7

[:aceman]

(In reply to [e-mail] from comment #4)
> >+    // Make sure the component in not larger then the expected maximum.
> >+    if (component > Math.pow(256, aWidth) - 1)
> if ((component >>> ((aWidth - 1) * 8)) >= 256) perhaps?

The page at 
http://stackoverflow.com/questions/2373791/bitshift-in-javascript mentions shift arguments are cut down to 32bits. May this expression lie to us if component is above max_uint32?

Comment 8

[[e-mail]]

(In reply to aceman from comment #7)
> (In reply to [e-mail] from comment #4)
> > >+    // Make sure the component in not larger then the expected maximum.
> > >+    if (component > Math.pow(256, aWidth) - 1)
> > if ((component >>> ((aWidth - 1) * 8)) >= 256) perhaps?
> shift arguments are cut down to 32bits. May this expression lie to us if
> component is above max_uint32?
Ah, that's a point. In that case, I guess
if (component >= Math.pow(256, aWidth))
(to avoid the unnecessary -1)

Comment 9

[:aceman]

But we can always have a precomputed table for the lookup:
const kPowersOf256 = [ 256, 65535, 16777216, 4294967296]

if (component >= kPowersOf256[aWidth]) ...

Comment 10

[:aceman]

Attachment: patch v2

Comment 11

[:aceman]

Neil, ping :)

Comment 12

[[e-mail]]

Comment on attachment 736383 [details] [diff] [review]
patch v2

>+  if (!((aAllowExtendedIPFormats && componentCount <= 4) || componentCount == 4))
>+    return null;
I found this confusing. A possible alternative approach:

if (componentCount > 4)
  return null;
if (componentCount < 4 && !aAllowExtendedIPFormats)
  return null;

(Or you could use || if you don't think that makes it confusing again).

>+  const kPowersOf256 = [ 1, 256, 65535, 16777216, 4294967296 ];
65536. r=me with that fixed.

Comment 13

[:aceman]

Attachment: patch v3

Thanks, done.

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/07484e2a5190