324305 - tstclnt unable to resolve hostnames to IP addresses on HPUX 11i

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

We have a build/test system named hploan1 which runs HPUX 11.11i (IIRC).
ssl.sh fails on it.  The tstclnt program returns an error, saying that it
cannot resolve the host name.  The tstclnt code may be seen here:

http://lxr.mozilla.org/mozilla/source/security/nss/cmd/tstclnt/tstclnt.c#546

There are a number of other bugs that are related, at least potentially.

https://bugzilla.mozilla.org/show_bug.cgi?id=161610
    tstclnt doesn't accept IPv6 addresses.  The fix for this bug introduced
    the use of PR_GetIPNodeByName to tstclnt.  Previously tstclnt used 
    PR_GetHostByName.

https://bugzilla.mozilla.org/show_bug.cgi?id=211596
    PR_GetIPNodeByName fails on HPUX 11i  (sounds familiar)

https://bugzilla.mozilla.org/show_bug.cgi?id=214752 
    PR_GetIPNodeByName sets wrong NSPR error code for ERANGE error.

If the present implementation of PR_GetIPNodeByName doesn't work on HPUX, then IMO another implementation should be created for HPUX, one that does work. 
The point of NSPR, after all, is to isolate applications from these sorts of
platform differences.  

But this bug report is an NSS bug, about tstclnt.  I'm going to propose 
that tstclnt be changed to fall back on PR_GetHostByName if and when PR_GetIPNodeByName fails.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch, v1

This patch is one of several alternative proposals I will make.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: alternative HP-specific patch

This alternative would address the problem specifically for HPUX 11.
It would also provide slightly more diagnostic output.

Wan-Teh, I invite your comments on these approaches.  
I will not ask for review until I have done more testing and debugging.

Comment 3

[Wan-Teh Chang]

Nelson, PR_GetIPNodeByName is deprecated.  The new
preferred name resolution function that handles both
IPv4 and IPv6 is PR_GetAddrInfoByName.  The related
functions are PR_FreeAddrInfo, PR_EnumerateAddrInfo,
and PR_GetCanonNameFromAddrInfo.  They are all declared
and documented in "prnetdb.h".

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: convert to PR_GetAddrInfoByName

I found that PR_EnumerateAddrInfo uses the same return value to mean
"end of list" and also "error, unable to return valid result".
So, I decided to use the content of the result area to detect whether
it had succeeded or not.  
Does this look right to you, Wan-Teh?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I have tested this third patch on the HPUX system where PR_GetIPNodeByName
failed.  With this patch, it succeeds.  

Comment 6

[Wan-Teh Chang]

Comment on attachment 209261 [details] [diff] [review]
convert to PR_GetAddrInfoByName

PR_EnumerateAddrInfo cannot fail if you pass
valid inputs to it.  Similarly PR_EnumerateHostEnt
cannot fail if you pass valid inputs to it.

Comment 7

[Wan-Teh Chang]

I found that the documentation of PR_EnumerateHostEnt
says it may fail and return -1.  This is wrong.  With
its prototype, the only error checking we can do is
to see if enumIndex < 0.  We can't do the more useful
test of checking if enumIndex is beyond the end of the
h_addr_list array.

There is another subtle point about the end of enumeration:
when PR_EnumerateAddrInfo returns NULL or PR_EnumerateHostEnt
returns 0, the enumeration ends, meaning you just step
beyond the end of the address list, and you should not
use the PRNetAddr.  The documentation could be improved.

So the original tstclnt code should really say:

	if (PR_EnumerateHostEnt(0, &hp, (PRUint16)atoi(port), &addr) <= 0) {
	    SECU_PrintError(progName, "error looking up host address");

or

	if (PR_EnumerateHostEnt(0, &hp, (PRUint16)atoi(port), &addr) == 0) {
	    SECU_PrintError(progName, "error looking up host address");

and the new code should say:

	if (PR_EnumerateAddrInfo(NULL, addrInfo, portno, &addr) == NULL) {
	    SECU_PrintError(progName, "error looking up host address");

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, was your comment 6 intended to convey r- ?
Your comment 7 seems to describe the old code, before this patch.  
Is it sufficient to change the third patch as follows:

-	if (addr.raw.family == PR_AF_UNSPEC) {
+	if (addr.raw.family == PR_AF_UNSPEC || enumPtr == NULL) {

Comment 9

[Wan-Teh Chang]

Attachment: convert to PR_GetAddrInfoByName, v2

Nelson, a patch is worth a thousand words, so I wrote
this patch to make my suggested changes precise.  Note
that I'm passing PR_AF_UNSPEC instead of PR_AF_INET as
the second argument to PR_GetAddrInfoByName, otherwise
you will only get IPv4 addresses, a regression from
the original code.

I didn't change the error strings in the original code
so I can read the diffs (from the original code) easily.
Feel free to change the error strings as appropriate.

Note that if you pass valid enumPtr and addrInfo arguments
to PR_GetAddrInfoByName, it will not fail.  Like libc's string.h
functions (strlen, strcmp, etc.), PR_EnumerateAddrInfo assumes
you pass valid arguments to it.  I don't want to debate this
design decision; we have to live with this assumption.  Also,
in general you can't assume that a function won't touch any field
of the output argument on failure.  This is why you shouldn't
test addr.raw.family to determine if PR_EnumerateAddrInfo failed.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh,  Your patch doesn't constrain the address families that 
PR_GetAddrInfoByName may return.  Consequently, the returned set of addresses 
could (theoretically) include addresses that are neither PR_AF_INET nor 
PR_AF_INETV6.  So, I think we need to have the enumeration loop there, to 
ensure that we find an address in one of those two forms.  Agreed? 

Comment 11

[Wan-Teh Chang]

Nelson, accepting only PR_AF_INET and PR_AF_INET6 is fine by me.

Comment 12

[Wan-Teh Chang]

Attachment: convert to PR_GetAddrInfoByName, v3

I found one problem with tstclnt: it always creates
an IPv4 socket.  This patch fixes that.  It also
checks for PR_AF_INET and PR_AF_INET6 addresses as
Nelson suggested.

Nelson, could you run unpatched tstclnt with the -v
(verbose) option on that HP-UX system and paste the
output from tstclnt's printHostNameAndAddr fuction?
The output is generated by this statement:

        FPRINTF(stderr, "%s: connecting to %s:%hu (address=%s)\n",
               progName, host, port, addrBuf);

I want to know exactly what failed.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213534 [details] [diff] [review]
convert to PR_GetAddrInfoByName, v3

Review comments:

>-    if (PR_IsNetAddrType(&addr, PR_IpAddrV4Mapped)) {
>-    	/* convert to IPv4.  */
>-	addr.inet.family = PR_AF_INET;
>-	memcpy(&addr.inet.ip, &addr.ipv6.ip.pr_s6_addr[12], 4);
>-	memset(&addr.inet.pad[0], 0, sizeof addr.inet.pad);
>-    }

I'm surprised that you're removing that code, because it was 
originally added at your suggestion.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=161610#c7

IIRC, there were some boxes that didn't correctly implement IPv6
and the lines of code above enabled them to work when the 
addresses were really mapped IPV4 addresses.


>-	    s = PR_NewTCPSocket();
>+	    s = PR_OpenTCPSocket(addr.raw.family);

If the sockets returned by PR_NewTCPSocket cannot be used for IPv6 
then ALL of NSS's cmd utilities need this change.  
Compare http://lxr.mozilla.org/security/search?string=PR_NewTCPSocket
with    http://lxr.mozilla.org/security/search?string=PR_OpenTCPSocket

I will attempt to do the test you requested.

Comment 14

[Wan-Teh Chang]

Nelson, in your original patch for bug 161610, you had

+#if !defined(_PR_INET6)
+    if (addr.ipv6.family == PR_AF_INET6 && 
+        PR_ntohl(addr.ipv6.ip.pr_s6_addr32[2]) == 0xffff) {
+    	/* local system doesn't support IPv6, so convert to IPv4.  HACK */
+	addr.inet.family = PR_AF_INET;
+	memcpy(&addr.inet.ip, &addr.ipv6.ip.pr_s6_addr[12], 4);
     }
+#endif

I suggested that you rewrote the above using PR_IsNetAddrType.
But I didn't realize that the above code wasn't needed at all,
and that more changes were needed for tstclnt to support IPv6.

Since tstclnt doesn't support IPv6, another way to fix this
bug is to revert to PR_GetHostByName and remove the above code.

Comment 15

[Wan-Teh Chang]

Attachment: Remove incomplete IPv6 address support

Now that I know the current IPv6 support in tstclnt is
only the support for IPv4-mapped IPv6 addresses, I don't
feel bad about backing out that support if a low-risk
patch is desired.  I certainly prefer going forward
with PR_GetAddrInfoByName.

Nelson, I read the bug's description again and realized
that it is PR_GetIPNodeByName, not PR_Connect, that fails.
So please disregard the test I requested in comment 12.

There is a test you can perform, if you have some spare
time.  In mozilla/nsprpub/pr/src/misc/prnetdb.c, there is
only one call to getipnodebyname():

#elif defined(_PR_HAVE_GETIPNODEBYNAME)
    h = getipnodebyname(name, md_af, tmp_flags, &error_num);
#else

Please add

    fprintf(stderr,
            "getipnodebyname(%s, %hu, %d, &error_num)=%p error_num=%d\n",
            name, md_af, tmp_flags, h, error_num);

after the getipnodebyname() call.  Thanks a lot.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

tstclnt's support for IPv6 was not intended to only work for IPv4 mapped 
addresses.  The intent was to increase the probability that tstclnt 
would succeed in the common case of IPv4 mapped addresses, when being used
on a machine with broken IPv6 support.  Essentially, it was to only use
the IPv6 address type for addresses that were not mapped IPv4 addresses.

However, I think we now know that the problem that appeared to be a broken
IPv6 implementation was actually the result of the PR_NewTCPSocket call
asking for an IPv4 socket.  SO, perhaps it is the case that the system
for which that old patch was developed would have worked with IPv6 if we
had requested a socket for the right family.  OTOH, it would be nice to 
continue to be able to fall back to using an IPv4 socket when an attempt
to use an IPv6 socket fails and the address is just a mapped IPv4 address.

Anyway, I agree that we want to move forward to IPv6 if we can get it to
work on all our platforms.  Christophe has a way to apply a patch to a
source tree after checkout and before all the different platforms begin
to build that common source tree in the nightly builds.  So I think I
should ask him to test your "convert to PR_GetAddrInfoByName, v3" patch
in that fashion, and if it works, we should go forward with it.

If it works, I think we also need a bug that asks for all NSS's test 
programs to be likewise fixed for IPv6.

Comment 17

[Julien Pierre]

Alexei,

I'm not sure if this has anything to do with the NSPR issue on HP-UX that you are working on.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213534 [details] [diff] [review]
convert to PR_GetAddrInfoByName, v3

Alexei, please SR for branch

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213534 [details] [diff] [review]
convert to PR_GetAddrInfoByName, v3

I committed this on the trunk to let it bake for a while.
cmd/tstclnt/tstclnt.c; new revision: 1.47; previous revision: 1.46
Will keep an eye on Tinderbox, assuming it comes back to life.

Comment 20

[Alexei Volkov]

Comment on attachment 213534 [details] [diff] [review]
convert to PR_GetAddrInfoByName, v3

r=alexei.volkov.bugs

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Committed on 3.11.branch
Checking in tstclnt.c; new revision: 1.42.2.3; previous revision: 1.42.2.2