34843 - Use getaddrinfo/getnameinfo

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Wan-Teh Chang]

The getipnodebyname, getipnodebyaddr, inet_pton,
and inetntop functions are being deprecated,
primarily because they do not support scoped
addresses.  We should use getaddrinfo and getnameinfo
instead.

Microsoft's IPv6 Technology Preview for Windows 2000
(http://msdn.microsoft.com/downloads/sdks/platform/tpipv6.asp)
only supports getaddrinfo and getnameinfo, so this
change is necessary to support IPv6 Technolody Preview
for Windows 2000.

The mapping is:
getipnodebyname => getaddrinfo
getipnodebyaddr => getnameinfo
freehostent => freeaddrinfo

For inet_pton, use getaddrinfo with AI_NUMERICHOST
set in the ai_flags field of the hints parameter.

For inet_ntop, use getnameinfo with NI_NUMERICHOST
set in the flags parameter.

For background info on why one might want to use
getxxxinfo instead of inet_pton and inet_ntop, see:

http://www.ietf.org/internet-drafts/draft-ietf-ipngwg-scopedaddr-format-01.txt

Comment 1

[John G. Myers]

Until the IETF settles on a syntax for scoped addresses, I'd be wary.

Comment 2

[Matti Aarnio]

Now a year since jgm's comment, I would say: go ahead.

It depends somewhat on IETF and Austin Group(IEEE/ISO/..) draft of the week, but
better that than getipnodebyname().

Yesterday I entered what essentially is duplicate of this.

Comment 3

[Colin Blake]

Nearly three years have passed. Isn't it about time? The IP people here keep
telling me that the gethostby* routines are depricated and I should move away
from them...

Comment 4

[Wan-Teh Chang]

I can only say, "we accept patches."

Comment 5

[Colin Blake]

Bug 205726 implies that this work is now done?

Comment 6

[Wan-Teh Chang]

Half of this work is done.  NSPR 4.5 and Mozilla 1.6 are using getaddrinfo.
But we are not using getnameinfo yet.

Comment 7

[Wan-Teh Chang]

Darin, could you take a stab at the unfinished work?

Mozilla is not using PR_GetHostByAddr (which is the
only function using getipnodebyaddr), so you just
need to add an implementation of PR_StringToNetAddr
and PR_NetAddrToString using getaddrinfo and getnameinfo
as I outlined in the description of this bug.

Comment 8

[Darin Fisher]

sure, i can do this.  not sure when i will have a chance though.

Comment 9

[Lorenzo Colitti]

Hmm... I might take a crack at this. But I'm worried about how to make it
portable. Can I just assume that if _PR_HAVE_GETADDRINFO is defined then
getnameinfo is available, and load it dynamically in _pr_find_getaddrinfo() like
getaddrinfo is? Or do you think I would have to test for it separately and
define another _PR_HAVE_GETNAMEINFO?

Can we also say that NSPR supports IPv6 only if we have getaddrinfo and
getnameinfo, or do we have to be more flexible?

Comment 10

[Darin Fisher]

>Hmm... I might take a crack at this. But I'm worried about how to make it
>portable. Can I just assume that if _PR_HAVE_GETADDRINFO is defined then
>getnameinfo is available, and load it dynamically in _pr_find_getaddrinfo() 
>like getaddrinfo is? Or do you think I would have to test for it separately and
>define another _PR_HAVE_GETNAMEINFO?

this sounds pretty reasonable to me.  might be nice to come up with a more
generic tag other than GETADDRINFO that would encompass both getaddrinfo and
getnameinfo, but that's just aesthetics.  i defer to wtc.


>Can we also say that NSPR supports IPv6 only if we have getaddrinfo and
>getnameinfo, or do we have to be more flexible?

i'm not sure about this... i wouldn't change the way IPv6 support is detected. 
i believe gethostbyname can return IPv6 addresses on some platforms.

Comment 11

[Lorenzo Colitti]

Clarification: what I really mean was I would like to handle IPv6 addresses in
just one way, with getaddrinfo and getnameinfo, and drop inet_pton and inet_ntop
altogether. But I don't know whether this would stop IPv6 from working on some
platforms. Are there platforms that support IPv6 but not getaddrinfo/getnameinfo?

Comment 12

[Darin Fisher]

Mac OSX 10.2 has getaddrinfo/getnameinfo (but we should not use them -- see bug
222031), but OSX 10.2 also supports IPv6, and it provides inet_pton/inet_ntop.

Comment 13

[Noriko Hosoi]

Attachment: Fix proposal (cvs diff pr/include/prnetdb.h pr/src/misc/prnetdb.c)

We have a software which depends upon NSPR.  Currently, an address in the form of "<IPv6_addr>%<zone_id>" (e.g., "[fe80::204:23ff:fe24:f6c3%eth0]") is not taken care properly on Red Hat Linux.  As suggested in the first comment, using getaddrinfo and getnameinfo solves this problem.

Files:
pr/include/prnetdb.h
pr/src/misc/prnetdb.c

Change description:
1) PR_AI_NUMERICHOST is introduced.
2) PR_SetNetAddr: if PR_IpAddrNull is not set, not just IP address but the associated scope_id is not initialized if the address family is AF_INET6.
3) PR_StringToNetAddr: use PR_GetAddrInfoByName instead of inet_pton.
4) static function pr_IsValidNumericHost is introduced to check if the hostname is a valid numerichost or not when PR_AI_NUMERICHOST is set in flags.  Note that this function is called only from pr_GetAddrInfoByNameFB (used on the system with no getaddrinfo).  The logic used in pr_IsValidNumericHost is almost identical to the one that the original PR_StringToNetAddr was implemented.
5) PR_GetAddrInfoByName: when PR_AI_NUMERICHOST is set in the flags, pass it to getaddrinfo as a hint.
6) pr_GetNameInfo is introduced.  It's a thin wrapper of getnameinfo.  Not like PR_GetAddrInfoByName, the function is not implemented on the platform that does not support getaddrinfo.  It has to be used only in #if defined(_PR_HAVE_GETADDRINFO).

Sample test case using Mozilla LDAP C SDK:
Before adding the change>
./ldapsearch -h '[fe80::204:23ff:fe24:f6c3%eth0]' -p 10000 -b "o=netscaperoot" "(uid=*)"
ldap_search: Can't connect to the LDAP server - No route to host

After adding the change>
./ldapsearch -h '[fe80::204:23ff:fe24:f6c3%eth0]' -p 10000 -b "o=netscaperoot" "(uid=*)"
version: 1
dn: uid=admin, ou=Administrators, ou=TopologyManagement, o=NetscapeRoot
[...]

Comment 14

[Noriko Hosoi]

Attachment: proposed test case (cvs diff mozilla/nsprpub/pr/tests/ipv6.c)

Test case for the proposed fix in comment #13.

There are some platform dependencies in handing scope id:
1) Solaris 9 and HP-UX 11.23 do not take the address format <IPv6_addr>%<interface>; RHEL does not take one without "%<interface>". 2) The function getaddrinfo on Solaris 9 and HP-UX 11.23 does not fill in the scope_id field in sockaddr, but the following operations work just fine without the information; while without scope_id, connect on RHEL fails. That is, current ldapsearch (from HEAD) works fine on Solaris and HP-UX, but lt does not on RHEL. RHEL requires the NSPR fix: using getaddrinfo and getnameinfo in PR_StringToNetAddr and PR_NetAddrToString, respectively.

Thus, the scope id checking is done just on Linux for now (plus link local, only).  On the other platforms, the test is just checking whether PR_StringToNetAddr works fine for the IPv6 numeric names (link local, site local, and global) or not.

Sample test results:
[RHEL4]
Checking if scope_id is filled in...success .. correct scope_id 2 (fe80::200:ff:fe00:a0a0%eth0)
[Solaris 9 & HP-UX 11.23]
Checking if scope_id is filled in...success .. no error from PR_StringToNetAddr

Comment 15

[Wan-Teh Chang]

Attachment: Fix proposal v1.1 (cvs diff pr/src/misc/prnetdb.c)

Noriko, your patch still needs work.  I've wanted to work on
it, but haven't had time.  You can create your own NSPR
branch with this patch so that I don't block your progress.

The only change I made to your patch is to move the definition
of the PR_AI_NUMERICHOST macro from the public header prnetdb.h
to the C source file prnetdb.c.  For this bug, it's not necessary
to add PR_AI_NUMERICHOST to the NSPR API.  So let's hide this
macro to prevent your (LDAP) code from accidentally using
PR_AI_NUMERICHOST.

Also for NSPR patches please don't use the -w and -t options for
cvs diff.

Comment 16

[Wan-Teh Chang]

Attachment: Noriko's patch (do not check in)

Comment 17

[Wan-Teh Chang]

Attachment: Noriko's patch edited by Wan-Teh (do not check in)

Comment 18

[Wan-Teh Chang]

Attachment: Noriko's patch edited by Wan-Teh v2 (do not check in)

Noriko, please test this patch.  Also, since we just released
NSPR 4.6.3, you should upgrade to NSPR 4.6.3.

The only change I made in this patch is to the PR_StringToNetAddr
function.  The change is that PR_StringToNetAddr function won't
touch the 'port' field of the PRNetAddr union as before.

Comment 19

[Wan-Teh Chang]

Attachment: Proposed patch (for the NSPR tip)

Noriko, please review and test this patch.  This patch should only
be applied to the NSPR tip.  Don't apply it to the NSPR_4_6_BRANCH.

If you prefer to test the NSPR_4_6_BRANCH, let me know.  Thanks.

Comment 20

[Noriko Hosoi]

(In reply to comment #19)
> Created an attachment (id=255963) [details]
> Proposed patch (for the NSPR tip)
> 
> Noriko, please review and test this patch.  This patch should only
> be applied to the NSPR tip.  Don't apply it to the NSPR_4_6_BRANCH.
> 
> If you prefer to test the NSPR_4_6_BRANCH, let me know.  Thanks.
> 
If you could make it easily, it'd be great.  I'm trying to create an rpm package from the source checked out from HEAD, but I'm having some difficulties...
Thanks, Wan-Teh!

Comment 21

[Noriko Hosoi]

(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=255963) [details] [details]
> > Proposed patch (for the NSPR tip)
> > 
> > Noriko, please review and test this patch.  This patch should only
> > be applied to the NSPR tip.  Don't apply it to the NSPR_4_6_BRANCH.
> > 
> > If you prefer to test the NSPR_4_6_BRANCH, let me know.  Thanks.

Oops, I should have been more specific.  We are using this tag: NSPR_4_6_4_RTM.

Comment 22

[Wan-Teh Chang]

Attachment: Proposed patch (for NSPR_4_6_4_RTM)

Noriko, please try this patch.  This patch can also be applied
to NSPR_4_6_5_RTM or NSPR_4_6_6_RTM.

Comment 23

[Noriko Hosoi]

Comment on attachment 255985 [details] [diff] [review]
Proposed patch (for NSPR_4_6_4_RTM)

I built Mozilla LDAP C SDK and Fedora Directory Server linked with the ipv6.patch applied NSPR 4.6.4 and tested between the LDAP clients and the servers.

Supported client platforms: Win2K, Win2003, RHEL4, Solaris9, HP-UX11.23
Supported server platforms: RHEL4, Solaris9, HP-UX11.23

Due to the network configuration, tests were done on the following combinations and the tests passed for all the cases.
-------------------+--------------------
LDAP clients          LDAP server
-------------------+--------------------
Win2K             --> RHEL4 (32-bit)
Win2K             --> Solaris9 (64-bit)
Win2003           --> RHEL4 (32-bit)
Win2003           --> Solaris9 (64-bit)
RHEL4 (32-bit)    --> Solaris9 (64-bit)
Solaris9 (64-bit) --> RHEL4 (32-bit)
-------------------+--------------------
LDAP clients          LDAP server
-------------------+--------------------
HP-UX11.23        --> RHEL4 (64-bit)
RHEL4 (64-bit)    --> HP-UX11.23
-------------------+--------------------

I also reviewed the ipv6.patch code.  It looks good to me.

Comment 24

[Wan-Teh Chang]

Comment on attachment 255963 [details] [diff] [review]
Proposed patch (for the NSPR tip)

I checked in the patch on the NSPR trunk (NSPR 4.7).

Checking in include/md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.33; previous revision: 3.32
done
Checking in include/md/_winnt.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_winnt.h,v  <--  _winnt.h
new revision: 3.32; previous revision: 3.31
done
Checking in src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.52; previous revision: 3.51
done

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

This will be in NSPR 4.6.7 ?

Comment 26

[Wan-Teh Chang]

Comment on attachment 255963 [details] [diff] [review]
Proposed patch (for the NSPR tip)

Nelson, I don't plan to include this patch in NSPR 4.6.7.  Our
products will apply this patch to the NSPR 4.6.x they're using.
(Our build system supports applying local patches to official
source distributions.)  If you're interested in having this fix
in NSPR 4.6.7, please review this patch.

I checked in this patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Mozilla 1.9 Alpha 3).

Checking in include/md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.19.2.14; previous revision: 3.19.2.13
done
Checking in include/md/_winnt.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_winnt.h,v  <--  _winnt.h
new revision: 3.22.4.10; previous revision: 3.22.4.9
done
Checking in src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.21.2.31; previous revision: 3.21.2.30
done

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 255963 [details] [diff] [review]
Proposed patch (for the NSPR tip)

For this review, I relied on these man pages:
http://www.opengroup.org/onlinepubs/009695399/functions/getaddrinfo.html
http://www.opengroup.org/onlinepubs/009695399/functions/getnameinfo.html

I have two issues with this patch.  I'm not sure they're major enough to 
stop it form being checked in, but perhaps the bug should not be closed
until they're addressed.

1. I'm not sure that the flags AI_NUMERICHOST and NI_NUMERICHOST, as used
in this patch, are sufficient.  It appears to me that to get completely 
numeric results, or use completely numeric input, it may also be necessary
to set some of these other flags: NI_NUMERICSERV, NI_NUMERICSCOPE, and/or
AI_NUMERICSERV.

2. All non-zero return values are mapped to PR_INVALID_ARGUMENT_ERROR,
which tells the caller "It's all your fault".  While many of the return 
values do map to PR_INVALID_ARGUMENT_ERROR, some don't.  For example:
- EAI_AGAIN seems to be EAGAIN, and might map to PR_PENDING_INTERRUPT_ERROR.
  Perhaps we should immediately retry at least once or twice when it occurs.
- EAI_MEMORY should map to PR_OUT_OF_MEMORY_ERROR
- EAI_SYSTEM should cause NSPR to interpret errno as it does in function
  _MD_unix_map_default_error()
- EAI_FAMILY maps to PR_ADDRESS_NOT_SUPPORTED_ERROR

Comment 28

[Wan-Teh Chang]

Nelson, thanks for your review comments.  We don't need to use
AI_NUMERICSERV and NI_NUMERICSERV because we're not translating
the port numbers to/from strings (i.e., the 'servname' and the
'service' arguments are NULL in our calls to getaddrinfo and
getnameinfo).  NI_NUMERICSCOPE is a new flag.  It's probably not
implemented in any OS yet, for example, neither Solaris 10 nor
Windows implements NI_NUMERICSCOPE:
http://docs.sun.com/app/docs/doc/816-5170/6mbb5es97?q=ps&a=view
http://msdn2.microsoft.com/en-us/library/ms738532.aspx

I'll see what I can do about error code mapping.  The way
we call getaddrinfo and getnameinfo in this patch limits
the possible errors to caller's fault.  (For example, it
should be impossible to get the EAI_AGAIN, EAI_MEMORY, and
EAI_SYSTEM errors.)  This is why we only set the
PR_INVALID_ARGUMENT_ERROR error as an approximation.

(PR_NetAddrToString should map EAI_OVERFLOW to PR_BUFFER_OVERFLOW_ERROR,
but EAI_OVERFLOW is not returned by Windows.)

Comment 29

[Wan-Teh Chang]

Attachment: Map error codes (work in progress)

On Windows, Microsoft documentation recommends calling WSAGetLastError()
instead of using the return value of getaddrinfo or getnameinfo.  I still
need to work on the error code mapping on non-Windows platforms.