Closed Bug 809609 Opened 8 years ago Closed 7 years ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 25.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

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

neil@parkwaycc.co.uk 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.
From bug 810763:
neil@parkwaycc.co.uk 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?
Depends on: 810763
Let's not forget to add those new formats to the test in mailnews/base/test/unit/test_hostnameUtils.js .
Flags: in-testsuite?
Attached patch patch (obsolete) — Splinter Review
Apply on top of patch in bug 810763.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #734766 - Flags: review?(neil)
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?
(mostly to avoid Math.pow)
OK, if I am allowed to put a comment there what that crazy thing does ;)
(In reply to neil@parkwaycc.co.uk 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?
(In reply to aceman from comment #7)
> (In reply to neil@parkwaycc.co.uk 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)
But we can always have a precomputed table for the lookup:
const kPowersOf256 = [ 256, 65535, 16777216, 4294967296]

if (component >= kPowersOf256[aWidth]) ...
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #734766 - Attachment is obsolete: true
Attachment #734766 - Flags: review?(neil)
Attachment #736383 - Flags: review?(neil)
Neil, ping :)
Flags: needinfo?(neil)
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.
Attachment #736383 - Flags: review?(neil) → review+
Attached patch patch v3Splinter Review
Thanks, done.
Attachment #736383 - Attachment is obsolete: true
Attachment #782296 - Flags: review+
Flags: needinfo?(neil)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/07484e2a5190
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
You need to log in before you can comment on or make changes to this bug.