Closed
Bug 344809
Opened 18 years ago
Closed 15 years ago
numerous platform inconsistencies of PR_StringToNetAddr for IPv4 addresses
Categories
(NSPR :: NSPR, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
4.8.4
People
(Reporter: nelson, Assigned: dwitte)
References
Details
Attachments
(2 files, 2 obsolete files)
14.88 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
21.60 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•18 years ago
|
||
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
Comment 2•18 years ago
|
||
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.
Updated•18 years ago
|
Target Milestone: 4.6.3 → 4.6.5
Updated•18 years ago
|
QA Contact: wtchang → nspr
Updated•17 years ago
|
Target Milestone: 4.6.5 → 4.6.6
Reporter | ||
Comment 3•17 years ago
|
||
Julien, Sun wants the fixes to these bugs.
Assignee: nelson → julien.pierre.boogz
Priority: P1 → P2
Target Milestone: 4.6.6 → ---
Assignee | ||
Comment 5•15 years ago
|
||
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?
Comment 6•15 years ago
|
||
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.
Reporter | ||
Comment 7•15 years ago
|
||
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
Assignee | ||
Comment 8•15 years ago
|
||
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)
Reporter | ||
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
Reporter | ||
Comment 11•15 years ago
|
||
Dan, Yes, Please put the BSD code into a new separate file, and I'll be happy to review it.
Assignee | ||
Comment 12•15 years ago
|
||
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)
Assignee | ||
Comment 13•15 years ago
|
||
I should also note that I modified the BSD inet_aton to handle a trailing dot, per bug 380543.
Reporter | ||
Comment 14•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
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)
Reporter | ||
Comment 16•15 years ago
|
||
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+
Reporter | ||
Comment 17•15 years ago
|
||
Oh, and Dan, Merci' beaucoup! Bolshoia Spaseebo! Vielen Danke!
Assignee | ||
Comment 18•15 years ago
|
||
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 | ||
Updated•15 years ago
|
Assignee: nobody → dwitte
Reporter | ||
Comment 19•15 years ago
|
||
> 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.
Assignee | ||
Comment 20•15 years ago
|
||
I tried landing, but it's a closed partition, so no go. If you could land it for me, that'd be great.
Comment 21•15 years ago
|
||
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.
Comment 22•15 years ago
|
||
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.
Assignee | ||
Comment 23•15 years ago
|
||
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?
Comment 24•15 years ago
|
||
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.
Assignee | ||
Comment 25•15 years ago
|
||
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.
Comment 26•15 years ago
|
||
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.
Assignee | ||
Comment 27•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #418463 -
Flags: review?(wtc) → review+
Comment 28•15 years ago
|
||
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.
Assignee | ||
Comment 29•15 years ago
|
||
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: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Target Milestone: --- → 4.8.4
Comment 30•2 years ago
|
||
It appears this has been fixed on modern macos. trio currently relies on getaddrinfo AI_NUMERICHOST being approximately as efficient as inet_pton
See https://github.com/python/cpython/issues/90980 for benchmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•