Closed Bug 344809 Opened 13 years ago Closed 10 years ago

numerous platform inconsistencies of PR_StringToNetAddr for IPv4 addresses

Categories

(NSPR :: NSPR, defect, P2, major)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: dwitte)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 25127, bug 66784 and bug 344700 all define various inconsistencies of
PR_StringToNetAddr on various platforms, especially for IPv4 addresses. 

The inconsistencies include differing answers returned on different platforms for
a) empty string input
b) input that is equivalent of 255.255.255.255 (that is, -1)
c) input that contains octal or hexadecimal components

PR_StringToNetAddr is designed to overcome one of the pricipal flaws of the
BSD function inet_addr, namely that with inet_addr, one cannot determine from
the returned value whether the input was invalid or was a valid representation
of the IPv4 broadcast address: 255.255.255.255.  BSD solved that problem by
creating a new function, inet_aton, that returns the success/failure result separately from the resultant address value, and PR_StringToNetAddr's signature
was clearly designed to use that same approach to avoid the ambiguity of 
output.  So, it's especially ironic that PR_StringToNetAddr has the same 
result ambiguity as inet_addr on some platforms.  For the definition of 
inet_ntoa, see http://www.gnu-darwin.org/shims/inet_addr.c

On some platforms, where PR_StringToNetAddr calls inet_pton, it only accepts
IPv4 addresses in the "dotted decimal" notation.  The IP address must have
exactly 4 values (separated by 3 "dots"), and each one must be a decimal 
value, not hex nor octal.  But on other platforms, the input IPv4 address 
may take any of the BSD dotted forms, where the address may have 1-4 parts
(separated by dots if the number is greater than 1) and each part may be 
decimal, octal or hex.  

Finally, the platforms differ on the result of empty input strings.

I propose to fix all these problems by reimplementing PR_StringToNetAddr 
to detect empty input right up front, and to no longer use either inet_pton() 
nor net_addr() for IPv4 addresses.  I propose to change it to use a new
function (for IPv4 addresses only) that I will write, which will functionally
match BSD's inet_aton.  Let's call it PL_inet_ntoa.  PR_StringToNetAddr will
use PL_inet_ntoa for IPv4 addresses on all platforms.  

Comments?  Objections?
Sorry, I copied the wrong bug numbers.  The 3 bugs were:

bug 158830  PR_StringToNetAddr("", *ptr) behaviour is inconsistent 
bug 335546  PR_StringToNetAddr("255.255.255.255",ptr) fails on some platforms 
bug 344700  PR_StringToNetAddr output varies by platform for octal inputs
Blocks: 158830, 335546
No longer blocks: 25127, 66784
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → 4.6.3
This is fine.  The PL_ prefix can only be used in
the PLDS and PLC libraries, whose source code is in
the mozilla/nsprpub/lib/ds and mozilla/nsprpub/lib/libc
directories.  The NSPR library (mozilla/nsprpub/pr)
uses the PR_ prefix.  An internal global function usually
uses the _PR_ prefix, and a static function usually uses
the _pr_, pr_, or no prefix.  This new function could be
a static function.

Watch out for apps that may depend on the current
behavior of PR_StringToNetAddr.
Target Milestone: 4.6.3 → 4.6.5
QA Contact: wtchang → nspr
Target Milestone: 4.6.5 → 4.6.6
Julien, Sun wants the fixes to these bugs.
Assignee: nelson → julien.pierre.boogz
Priority: P1 → P2
Target Milestone: 4.6.6 → ---
Blocks: 380543
Blocks: 317491
Taking.
Assignee: julien.pierre.boogz → nelson
Blocks: 321624
We could really use this in bug 321624, and in general, reimplementing this such that it does not involve an OS call would be great for performance - on Linux at least, OSX almost certainly, and perhaps Windows, this is a very expensive function. (I'm assuming that reimplementing it would involve some string parsing and comparison against a few cases, which would presumably be quite cheap.) Also, a common use case in Firefox is that we simply want to know if the conversion succeeded, without constructing a PRNetAddr result. If determining this is cheaper, perhaps we can expose a second API function, |PRBool PR_IsNetAddr(const char*)|.

Having our own implementation for IPv6 as well would probably offer the same benefits, but I'm not sure how technically difficult this is.

Nelson, do you have any plans to patch this soon? Can I help?
Dan, you are welcome to take this bug from Nelson.
The reason we changed PR_StringToNetAddr to call
getnameinfo is to support IPv6 address literals with
scope/zone IDs.  We have code for Mac that uses our
own code to convert an IP address literal if it doesn't
contain '%', and only call getnameinfo if it does.

There is no reason why getnameinfo with NI_NUMERICHOST
can't be as fast as our own code though.
This was assigned to Julien.  When he quit, I took it, but I have no 
immediate plans to work on it.  

Patches that build and work on all NSPR's supported platforms, and respect
the boundaries between machine dependent ("MD") code and independent code
(meaning they don't put lots of machine dependent ifdefs in the independent
code files) are generally welcome, I believe.
Assignee: nelson → nobody
Attached patch patch v1 (obsolete) — Splinter Review
This does what Nelson proposed in comment 0, and goes further. Specifically, it adds pr_inet_aton for IPv4 addresses, taken directly from current BSD libc sources (http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/src/lib/libc/inet/inet_addr.c?rev=1.4.10.1.2.1;content-type=text%2Fplain) and modified slightly to be NSPR-compatible. I included the full BSD license headers and commented in the source that they apply to that function only. (Luis Villa recommended this approach, in reference to http://www.softwarefreedom.org/resources/2007/gpl-non-gpl-collaboration.html.) Note that this function does differ from the one linked in comment 0, in that it contains additional error checking for edge cases.

Secondly, it makes pr_StringToNetAddrFB use this new function in all cases. For consistency, I also modified pr_StringToNetAddrFB to use StringToV6Addr in all cases, rather than use inet_pton(AF_INET6, ...). I think this is sensible for consistency in the IPv6 case as well, but I can revert this if you'd prefer. I also reordered things so we attempt IPv4 conversion before IPv6, to improve performance in the common case. (This of course assumes that IPv4 and IPv6 syntax is mutually exclusive, which is true as far as I know.)

Finally, it makes PR_StringToNetAddr use pr_StringToNetAddrFB in all cases, except where a scope id may be present. This is the important part for performance. By using pr_StringToNetAddrFB instead of getaddrinfo, the time for 3000 calls to PR_StringToNetAddr drops from 55ms to 0.27ms on Linux. That's a 200x speedup, and completely solves the performance problem we have in bug 321624. I'm not sure why getaddrinfo is so slow - perhaps it does a bunch of allocation - but in the common case, if we can avoid this cost, so much the better.
Attachment #415277 - Flags: review?(wtc)
Comment on attachment 415277 [details] [diff] [review]
patch v1

I suspect that having competing copyright statements in a single 
source file is a bad idea.  Probably better to put the new code 
with the different copyright statement into a separate source file
to keep the copyright statements and their respective code well
separated.  But IANAL.
Nelson, would you like to review this? I can provide a new patch with the BSD function split out into a separate file, if you'd like.
Dan, 
Yes, Please put the BSD code into a new separate file, and I'll be happy to
review it.
Attached patch patch v2 (obsolete) — Splinter Review
Splits the new function into a separate file, pr/src/misc/aton.c.
Attachment #415277 - Attachment is obsolete: true
Attachment #417600 - Flags: review?(nelson)
Attachment #415277 - Flags: review?(wtc)
I should also note that I modified the BSD inet_aton to handle a trailing dot, per bug 380543.
IMO, bug 380543 is invalid.  I'd have preferred if you didn't try to make 
that change in this patch.  Let's fix this bug first, then decide separately
on the central issue of bug 380543, which is whether IP addresses are DNS names.
Attached patch patch v3Splinter Review
Without handling for trailing '.' case. (Which means pr_inet_aton is inet_aton verbatim, apart from superficial changes to make it compile with NSPR.)
Attachment #417600 - Attachment is obsolete: true
Attachment #417975 - Flags: review?(nelson)
Attachment #417600 - Flags: review?(nelson)
Comment on attachment 417975 [details] [diff] [review]
patch v3

>-    rv = inet_pton(AF_INET6, string, &addr->ipv6.ip);
>+    rv = pr_inet_aton(string, &addr->inet.ip);
>+    if (1 == rv)
>+    {
>+        addr->raw.family = AF_INET;

I think this should be PR_AF_INET, but I concede that the old code being
replaced also uses AF_INET, so this patch leaves the code no worse than
before.  

r+= nelson
Attachment #417975 - Flags: review?(nelson) → review+
Oh, and Dan, Merci' beaucoup!  Bolshoia Spaseebo!  Vielen Danke!
Thanks, no problem!

Can I land this myself? I'm guessing not, in which case, could you please land it for me?

Also, I'm not sure if there are plans to tag NSPR soon. If not, would it be easy for you to make a tag so we can pull it into mozilla-central?
Assignee: nobody → dwitte
> Can I land this myself? 
I'm not sure.  But, I think so.  Please try.  
If you cannot, then let us know and I'll take care of it for you.

Wan-Teh JUST did an NSPR release tag, so it's way too soon to do another.
But Wan-Teh may have some other ideas about how to do that.
I tried landing, but it's a closed partition, so no go. If you could land it for me, that'd be great.
Comment on attachment 417975 [details] [diff] [review]
patch v3

>-     * On Mac OS X, getaddrinfo with AI_NUMERICHOST is slow.
>+     * On several platforms, getaddrinfo is much slower than pr_inet_pton.
>      * So we only use it to convert literal IP addresses that
>-     * contain IPv6 scope IDs, which pr_StringToNetAddrFB
>-     * cannot convert.  (See bug 404399.)
>+     * contain IPv6 scope IDs, which pr_inet_pton cannot convert.

Dan, in this comment please be specific what those platforms
(and the versions) are.  One problem with this kind of code is
that once added, it's almost impossible to remove because of
the fear of introducing a performance regression on some
unknown platform.  I'd like to submit bug reports to the
vendors of those OS's so that the performance problem of
getaddrinfo with AI_NUMERICHOST will be fixed eventually.
I(In reply to comment #20)
> I tried landing, but it's a closed partition, so no go. If you could land it
> for me, that'd be great.

I just gave you temporary commit access to mozilla/nsprpub
in CVS.  Please try again.
Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.56; previous revision: 1.55
done
Checking in pr/src/misc/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/misc/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/nsprpub/pr/src/misc/aton.c,v
done
Checking in pr/src/misc/aton.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/aton.c,v  <--  aton.c
initial revision: 3.1
done
Checking in pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.60; previous revision: 3.59
done

Thanks - done. I updated the comment to reference the old Mac OS X bug (bug 404399), and this bug for Linux glibc 2.10, and indicated that it most likely applies to other platforms as well since it's hard to beat a simple string parsing routine that doesn't allocate.

Could you make a (non-release) static tag for something like this, or should we just wait til the next release tag to pull it into mozilla-central?
I didn't realize mozilla-central is OPEN now :-)

Right now we have no plans for NSPR releases.  If you need
a new NSPR CVS tag for mozilla-central, our convention is
to create one in the form NSPR_HEAD_yyyymmdd (e.g.,
NSPR_HEAD_20091218).

Then, follow the steps at
https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central
to push a new NSPR tag to mozilla-central.

For mozilla-1.9.2 and mozilla-1.9.1, we should only push
an NSPR release tag.
Can I create a NSPR_HEAD_20091218 tag myself, or is that something only you should do? (I wasn't sure if you're asking me to do it, or just suggesting that it's possible. :)

We only want this on mozilla-central, so there's no need for a release tag.
You can create a NSPR_HEAD_20091218 tag yourself.  Just
check with one of the NSPR module owners if you need to
create an NSPR tag in the future.
Tagged as NSPR_HEAD_20091218. This updates mozilla-central to the new tag, which pulls in the fixes for this and bug 533966.

wtc, would you like to review?
Attachment #418463 - Flags: review?(wtc)
Attachment #418463 - Flags: review?(wtc) → review+
Comment on attachment 418463 [details] [diff] [review]
update mozilla-central to NSPR_HEAD_20091218

r=wtc.

> 	prnetdb.c  \
>+	aton.c  \
> 	prolock.c  \

This new file should be renamed praton.c.  aton.c
looks out of place among all the prxxx.c files.
http://hg.mozilla.org/mozilla-central/rev/7906f274af56

Renamed aton.c to praton.c, pushed to NSPR, retagged as NSPR_HEAD_20091219, and pushed to mozilla-central.

Thanks for the help and reviews!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.4
You need to log in before you can comment on or make changes to this bug.