388117 - ssl.sh hangs or fails on Windows when IPv6 is enabled

Status: RESOLVED WONTFIX

(NSS :: Test, defect, P5)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

While investigating NSPR bug 387954, I enabled IPv6 on my WinXP system
using the command "ipv6 install".  After doing so, ssl.sh began to hang.

The problem seems to be related to the fact that tstclnt is capable of
being an IPv4 or IPv6 client, while strsclnt and selfserv are only 
capable of IPv4.  

When IPv6 is enabled on Windows, and host name resolution is done with 
PR_GetAddrInfoByName and PR_EnumerateAddrInfo, "localhost" resolves to 
the IPv6 loopback address [::1] rather than to 127.0.0.1, and other 
hostnames that are listed in the hosts file with an Ip address of 
127.0.0.1 also resolve to [::1] instead.  Consequently, a program
such as tstclnt that uses PR_GetAddrInfoByName to resolve addresses, 
and uses either an IPv4 or IPv6 socket depending on the address family
of the resolve address will always use IPv6 when trying to contact 
localhost (by any name).  

My tests seem to indicate that, on windows,

a) a client using an AF_INET6 socket CANNOT connect to a server using an 
AF_INET listen socket.

b) SOME clients that use AF_INET sockets can connect to SOME (but not all) 
servers that use AF_INET6 listen sockets.  I have not yet determined why
this works with some combinations and not with others.  I had no trouble
connecting to the test server program whose source is attached to bug 
387954 with an IPv4 client.  But when I put the same server code into 
selfserv and tried there, I could not connect to selfserv with an IPv4
client.  I need to repeat all these tests.

c) when a client using a socket of one type tries to connect to a server 
that uses a listen socket of the other type, and it fails to connect, the
connection error is WSAECONREF, connection refused.  Even though the 
client and server are trying to use the same port number, the OS decides
there is no listen socket that satisfies the client's connect target, 
and so refuses the connection.  It fails immediately, but our tstclnt 
program waits a few seconds and retries, a large number of times, creating
the appearance that the tstclnt program is hung.  

In an effort to get ssl.sh working again, I converted selfserv to use an
AF_INET6 listen socket, using essentially the same code as in the attachment
to bug 387954.  Then tstclnt immediately began to work with selfserv, but
strsclnt did not.  strsclnt also began to appear to hang, and the connections
to selfserv were not completing.  So, then I converted strsclnt to use the 
same code as tstclnt, code that can choose the type of socket based on the 
resolved address.  With that change made, ssl.sh passes.  

So, I now have a patch for selfserv and another for strsclnt.  I'm satisfied
with the patch for strsclnt, but not entirely with the patch for selfserv.  
Since selfserv doesn't do any address resolution call before trying to listen,
it has no basis on which to decide whether to use an IPv4 or IPv6 listen 
socket.  So the patch presently always chooses AF_INET6 (IPv6) for the listen
socket.  I think this is doomed to fail on a system that does not have IPv6
support enabled.  

I am now seaerching for the best way to choose between listening on an IPv4 
or IPv6 socket.  Some ideas that occur to me include:

1) try IPv6 first, and if that fails, close that socket and try again with 
an IPv4 socket.

2) try some other way to "probe" the system to see if it supports IPv6, and
base the choice on that.

3) use BOTH IPv4 and IPv6 listen sockets on separate listen threads, or 
use non-blocking IO on the listen sockets and a single thread to poll both
of them.  

I am attaching two patches, the strsclnt patch that I believe is ready for
review, and the selfserv patch that I think is NOT ready for review.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: selfserv patch v1 - always listen on AF_INET6 socket

This patch works on IPv6-enabled systems, but probably does not work on 
ANY IPv4-only platform.  Needs work.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh can you suggest a good platform-independent way for an NSPR based
application to probe the system to determine IPv6 capability?

Some ideas that occur to me include:
a) try to resolve "localhost" with PR_GetAddrInfoByName and use the 
address family in the resolved address.

b) try to open an AF_INET6 socket and bind it to PR_Ip_Addr_any.  

Is there a better solution?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

In comment 0, I wrote:

> b) SOME clients that use AF_INET sockets can connect to SOME (but not all) 
> servers that use AF_INET6 listen sockets.

I can no longer get ANY AF_INET clients to connect to ANY AF_INET6 servers.
So, I think my original observation was mistaken regarding this one point.

I fear it will be necessary to use two listen sockets for a server to be 
able to receive connections from either type of client.  :(

Comment 4

[Wan-Teh Chang]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

On some Unix platforms, an AF_INET6 listening socket can accept
both IPv4 and IPv6 connections unless the listening socket is
marked as IPv6 only (there is a socket option for that).

This convenient feature does not work on Windows and other Unix
platforms.  So for portability, selfserv should have one listening
socket for each of AF_INET and AF_INET6, and the AF_INET6 listening
socket should be marked IPv6 only (by setting the IPV6_V6ONLY option
to 1).  NSPR doesn't expose the IPV6_V6ONLY socket option yet.

Comment 5

[Wan-Teh Chang]

(In reply to comment #2)
NSPR detects the presence of IPv6 by trying to open an
AF_INET6 socket.  You'll need to implement this test by
calling the socket() function directly, because
PR_OpenTCPSocket(PR_AF_INET6) may return a fake IPv6
socket that only works with IPv4-mapped IPv6 addresses.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 5:  Oh dear, that's nasty.  
Sounds like NSPR needs to offer a portable way for the application to 
probe the local system's IPv6 capabilities.  

I am working on another patch to selfserv to use two listen sockets. 
Unfortunately, it is going to be a rather large patch because it will add
more threads and need to manage them.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This bug now depends on bug 190467, which requests that NSPR support 
the IPV6_V6ONLY socket option.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: selfserv patch v2 - 2 listen sockets & 2 threads

This patch implements two separate listen sockets, one for IPv4 and one 
for IPv6, and separate threads to listen on them.  It works on my WinXP box.

I am attaching it to make it easily available to me on other systems for 
building and testing.  It is not for review at this time, because it has
at least the following problems:

a) it uses the flawed NSPR method of determining if the system supports 
IPv6 or not.

b) it does not set the IPV6_V6ONLY option, and it does not check to see 
if IPv6 sockets also can serve to listen on IPv4 ports.  

c) it does not generate a fatal error if it is running on an IPv6 system
but fails to bind an IPv6 socket.  It only generates a fatal error if it
fails to bind both IPv4 and IPv6 sockets.

Comment 9

[Julien Pierre]

Nelson,

Is there possibly any way we could avoid requiring two listen sockets and listener threads for IPv4 and IPv6 in a cross-platform way ? This is already a lot of new code for selfserv, and imagine what it will be for actual production server applications to implement.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment #9, no, on Windows, there is no alternative to using
two listen sockets, one for each address family.  Not if we want the server
to be able to answer requests received over either IPv4 or IPv6.  There are
also reportedly some Unix systems that behave this way, though I don't 
know which ones exactly.

I believe we do want selfserv to be able to receive and respond to both
IPv4 and IPv6 connections in the same process.  Presently, most NSS clients
use IPv4 only, but one (tstclnt) already uses IPv4 OR IPv6 automatically,
depending on the family of the address it gets back from the call that 
resolves the host name string to an IP address.  Given that our tests use
localhost (or equivalent), on an IPv6 capable system tstclnt will use IPv6
and the rest will use IPv4.  

We can modify all our clients to automatically switch between IPv4 and Ipv6, 
as tstclnt does now.  The first patch attached to this bug does exactly that
for strsclnt, and bug 366614 requests that that also be done to all the 
other test programs.  But I think it is useful to test our ability to do 
both, and also serves as an example for other NSPR-based products.   

Yes, it's more complicated that one would like, but I think real-world 
products must do it.  (That is, I believe the ability to support both IPv4
and IPv6 in the same server is (or will soon be) a common real-world 
requirement for real server products.)  So, it makes sense for our flagship 
test server program (:-) to illuminate the path forward, IMO.

Comment 11

[Julien Pierre]

Nelson,

I am not sure that we really need to make selfserv listen to both IPv4 and IPv6 at the same time. I think doing them each separately is sufficient. If you can get it to work reliably cross-platform, great, but IMO we only need to make sure they both work separately.

As far as the tstclnt issue goes, we should be able to fix that independently. Maybe selfserv should also take a hostname argument to select the binding address. Most real servers have an argument for that. This way, we can be sure that tstclnt and selfserv agree on what type of IP to use. There are probably other ways to deal with this.

I think production servers may use different approaches to deal with the problem. The accept logic in servers is already fairly optimized and this is probably not something they will want to rewrite. Most products already support virtual servers and multiple listen sockets/threads. The administrators can just use that feature to deal with having multiple listen sockets, some for IPv4 and others IPv6, as needed. So in theory this may not require code changes on the products' part.

Comment 12

[Julien Pierre]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

This looks OK except that addr.inet.port is uninitialized in the case where a hostname string is passed in.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

Julien: I believe the port member of the PRNetAddr is initialized by the 
following call:
>+	    enumPtr = PR_EnumerateAddrInfo(enumPtr, addrInfo, port, &addr);

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Also, note that this code comes straight out of tstclnt, where it works 
correctly.  Tstclnt connects to the correct port number.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

asking for re-review, in light of the preceding comments.  The code is tested in strsclnt and known to connect to the right port.  All.sh passes.

Comment 16

[Julien Pierre]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

My bad.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

requesting second review for NSS 3.11 branch. 
Wan-Teh has asked for someone else to do this review.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

strclnt patch committed on trunk.
Checking in strsclnt.c; new revision: 1.57; previous revision: 1.56

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272279 [details] [diff] [review]
strsclnt patch v1, make it use IPv4 or IPv6, like tstclnt (checked in)

Committed strsclnt change on 3.11 branch.
Checking in strsclnt.c; new revision: 1.43.2.10; previous revision: 1.43.2.9

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272715 [details] [diff] [review]
selfserv patch v2 - 2 listen sockets & 2 threads

Wan-Teh,  You're our reigning IPv6 guru.  :)
I'd like you to review this patch.

I now use this patch in all my Windows test builds.
I must use it, or else ssl.sh stops at the first test
that uses selfserv.  This is because my Windows system
has IPv6 enabled.

I don't know how well this will work on other platforms.
I'd like to try it on the trunk, to see how well it does.
If it fails, it can be backed out.  It only patches one
source file, so it's not very invasive.

Comment 21

[Wan-Teh Chang]

I won't have time to review this patch next week.
If you need a review soon, please ask someone else.
Sorry.

Comment 22

[Wan-Teh Chang]

Comment on attachment 272715 [details] [diff] [review]
selfserv patch v2 - 2 listen sockets & 2 threads

r=wtc.  I have some comments and suggested changes.

First, an IPv6 NSPR socket may be one of the three kinds
depending on the platform.
- IPv6 only, for example, on IPv6-enabled Windows XP.
- IPv6/IPv4 dual stack, for example, on IPv6-enabled Solaris.
- IPv4 only (handled as IPv4-mapped IPv6 addresses), for
  example on any system without IPv6 support.

With your patch, selfserv will have an IPv4 listening
NSPR socket, plus one of these IPv6 listening NSPR socket.
On the latter two kinds of platforms, both listening
sockets will accept connections from IPv4 clients.
This should work.  I just want to point this out.

We should add the support of the IPV6_V6ONLY socket option
to NSPR, so that we can make sure we create an IPv6 only
NSPR socket.  Then, if we can't do that, we'll just have
an IPv4 listening socket in selfserv.

>-static PRThread * acceptorThread;
>+static PRThread * IPv4AcceptorThread;
>+static PRThread * IPv6AcceptorThread;

I suggest naming these variables ipv4AcceptorThread
or acceptorThreadIPv4, so that their names begin with
a lowercase letter.

>-    FPRINTF(stderr, "selfserv: Closing listen socket.\n");
>+    FPRINTF(stderr, "selfserv: do_accepts: exiting.\n");
>     VLOG(("selfserv: do_accepts: exiting"));
>-    if (listen_sock) {
>-        PR_Close(listen_sock);
>-    }

Rather than updating the FPRINTF message, you should move
it along with the listen_sock closing code to the main function.
The new FPRINTF message is redundant with the VLOG message.

Why did you move the listen_sock closing code from do_accepts
to the main function?

>+acceptWrapper(void * arg)

I wanted to suggest that you rename this accept_wrapper to
make it look like the related do_accepts function.  Then
I discovered that this file doesn't have a consistent
naming convention for functions.  Both the foo_bar and
fooBar styles are used.  Sigh.

>+    do_accepts( args->listen_sock, args->model_sock, args->requestCert);

Delete the space after the opening parenthesis '('.

>+    if (prStatus != PR_SUCCESS) {
>+	if (af == PR_AF_INET)
>+	    errWarn("PR_Bind(PR_AF_INET)");
>+	else
>+	    errWarn("PR_BIND(PR_AF_INET6)");
>+	PR_Close(listen_sock);
>+	listen_sock = NULL;
>+	return NULL;
>     }

Since listen_sock is a local variable, it isn't necessary
to set it to NULL before returning from the function.

>+    if (listen_ipv4_sock && listen_ipv6_sock)
>+    	useModelSocket = PR_TRUE;

Why do we set useModelSocket to true if we have both
listen_ipv6_sock and listen_ipv6_sock?

>+	do {
>+	    /* We should really use some sort of "dummy" socket here. */
>+	    model_sock = PR_OpenTCPSocket(af);
>+	    if (model_sock != NULL)
>+	    	break;
>+	    if (af == PR_AF_INET)
>+		errExit("PR_OpenTCPSocket for model socket");
>+	    errWarn("PR_OpenTCPSocket for model socket");
>+	    af = PR_AF_INET;
>+	} while (model_sock == NULL);

At the end of each iteration, model_sock is NULL, so
model_sock == NULL is true.  You can replace this
do-while loop with for (;;) or while (1).  Unrolling
this loop may make the logic clearer.

>+    if (listen_ipv4_sock && listen_ipv6_sock) {
>+    	/* start two acceptor threads, and wait for them. */

Why don't you use the main thread to listen on one of
the listening sockets?

>+                        useLocalThreads ? PR_LOCAL_THREAD:PR_GLOBAL_THREAD,

Add spaces before and after the colon (:).  There are
two instances of this.

>-	exit(0);
>+	exit(!listen_ipv4_sock && !listen_ipv6_sock);

This change is incorrect in the case where the platform
supports both IPv4 and IPv6.  If a previous selfserv
process has freed the IPv4 TCP port but not the IPv6
TCP port, you'll get an exit status of 0 (success)
because we can bind to the IPv4 TCP port, but the new
selfserv process won't be able to bind to the IPv6 TCP
port.  It should be OK to ignore this issue.

>+	    /* Must have at least one good listen socket */
>+	    errExit("Got no Listen sockets");

Use lowercase "listen".  There are two instances of this.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - address Wan-Teh's review comments (backed out)

This patch is undoubtedly much bigger than the last one, 
because I addressed Wan-Teh's remarks about inconsistent 
function naming style.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

On trunk:
Checking in selfserv.c; new revision: 1.79; previous revision: 1.78
Watching to see what happens on Tinderbox.

Comment 25

[Wan-Teh Chang]

One problem with this patch is that if something goes wrong,
selfserv may still work in a degraded functionality mode, and
it's hard for us to detect that has happened.

An example is that on some platforms, PR_OpenTCPSocket(PR_AF_INET6)
may return an emulated IPv6 NSPR socket, which is implemented
by NSPR IO layering.  But a layered socket can't be inherited
by a child process.  So PR_GetInheritedFD(inheritableIPv6SockName)
will fail in a child process.  But this failure can't be easily
detected because the parent process will accept connections
and hide this problem.

These issues aren't necessarily serious, but with these issues
selfserv isn't a good sample server for listening for IPv4 and
IPv6 connection requests.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

I checked in patch v4 yesterday, after running ssl.sh with it on my WinXP
PC, and it passed 100%.  But when I checked it in, tinderbox went orange 
on some of the Solaris boxes and even on the one Windows box, which 
surprised me quite a bit.  

On Solaris, after succesfully running some (non-zero) number of tests,
it failed.  The log file contained these lines:

selfserv: PR_Bind(PR_AF_INET) returned error -5982:
Local Network address is in use.
selfserv: PR_BIND(PR_AF_INET6) returned error -5982:
Local Network address is in use.
selfserv: Got no listen sockets returned error -5982:
Local Network address is in use.
selfserv: normal termination

On Windows, the very first SSL test failed.  It behaved as if selfserv had 
simply not run.  tstclnt got error ECONNREF.  But there were no error 
messages from selfserv, at all.  I do not understand that behavior.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280420 [details] [diff] [review]
patch v3 - address Wan-Teh's review comments (backed out)

Note that the patch that is now correctly identified as v3, is the patch that was erroneously described as v4 in several preceding comments.

Comment 28

[Josh Aas]

Is this still an issue? If so, what might it mean for Gecko and Firefox, if anything?