387954 - NT version of PR_Accept on IPv6 listen socket fails with INVALID_ARGS

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: test program source

Tested with trunk (NSPR 4.7 Beta), but also seen with recent NSPR releases
(4.6.x).

The attached program runs fine when run on WinXP with the Win95 version of NSPR.
But when run with the NT version of NSPR, it fails and prints this message:

PR_Accept(): no socket! NSPR ERR: -5987, Invalid function argument

I traced it into _PR_MD_FAST_ACCEPT, to this call to AcceptEX in ntio.c 
at line 1351:

    rv = AcceptEx((SOCKET)osfd,
                  accept_sock,
                  me->md.acceptex_buf,
                  0,
                  INET_ADDR_PADDED,
                  INET_ADDR_PADDED,
                  &bytes,
                  &(me->md.overlapped.overlapped));

The stack looked like this:

libnspr4.dll!_PR_MD_FAST_ACCEPT(PRFileDesc * fd=0x003953f0, PRNetAddr * raddr=0x0012fe84, unsigned int * rlen=0x0012fe20, unsigned int timeout=4294967295, int fast=0, void (void *)* callback=0x00000000, void * callbackArg=0x00000000)  Line 1362

libnspr4.dll!SocketAccept(PRFileDesc * fd=0x003953f0, PRNetAddr * addr=0x0012fe84, unsigned int timeout=4294967295)  Line 424 + 0x1b bytes

libnspr4.dll!PR_Accept(PRFileDesc * fd=0x003953f0, PRNetAddr * addr=0x0012fe84, unsigned int timeout=4294967295)  Line 199 + 0x16 bytes	

IPv6serv.exe!main(int argc=1, char * * argv=0x00393098)  Line 66 + 0x13 bytes

The call returned zero, and err was 10022, which is WSAEINVAL.

I'm afraid this is a P1 for Sun.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure what the next 4.6.x version will be.

The problem occurs in function _md_get_recycled_socket (ntio.c line 968)
which is called by _PR_MD_FAST_ACCEPT to get the socket that will be used
as the accepted socket.  It sets af = AF_INET; unconditionally.  
Setting a breakpoint in that function, and setting af to 0x17 (AF_INET6)
in that function, causes the AcceptEX to succeed.

Now, the challenge is to figure out how to get _md_get_recycled_socket 
to set af correctly.  I suspect it needs to be an argument to the function.
Seems like the caller (_PR_MD_FAST_ACCEPT) should be able to get it out of
the PRFileDesc's private area somewhere, but it doesn't seem to be recorded
there (or I don't see it).  

BTW, it is now obvious that IPv6 server-side code was never tested with 
NSPR's NT flavor. :(

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This bug goes back to rev 1.1, which was in NSPR 3.0

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I see in the declaration of type PRFilePrivate that there is an af member
that is conditionally compiled in.  See
http://lxr.mozilla.org/nspr/source/nsprpub/pr/include/private/primpl.h#1748

I think it's time to make this unconditional.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch, v1

This bug uses the same techniques used for VMS.  
Then attached test program passes the test with this patch in place.
Wan-Teh, I don't expect you to like this first version, 
but I hope you can suggest what will be satisfactory.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272102 [details] [diff] [review]
proposed patch, v1

Wan-Teh, please review.

Comment 6

[Wan-Teh Chang]

Comment on attachment 272102 [details] [diff] [review]
proposed patch, v1

Nelson, your patch is on the right track.  The fix is indeed to
split the socket recycle list in ntio.c into two lists, one each
for AF_INET and AF_INET6, and to store the socket address family
in PRFileDesc.  You just need to find a better way to store the
socket address family.  It is fine to just use the existing
fd->secret->af, but we need to use a new macro for this purpose
or have fd->secret->af unconditionally.  You can also store the
address family in fd->secret->md (struct _MDFileDesc) for WINNT.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
do you have any objection to fd->secret->af being defined unconditionally 
even if it is only used on some platforms and not on others?
I do not intend to do all the work to actually use it on all platforms.

I found that my patch was not quite complete on Windows.  It was not 
setting the secret->af field on the sockets returned from PR_Accept. 
That doesn't seem particularly crucial, but IMO it should be consistent
within the platform.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch v2

This patch changes the meaning of the old define _PR_STRICT_ADDR_LEN,
and adds a new symbol _PR_NEED_SECRET_AF, which causes the af field 
in the secret struct to be defined.  Now the strict addr len symbol
only causes the address to be set according to the address family,
and is only set on VMS, as before.

Comment 9

[Wan-Teh Chang]

Comment on attachment 272373 [details] [diff] [review]
proposed patch v2

r=wtc.

The _PR_NEED_SECRET_AF macro name is a little hard to understand.

The "#ifdef XXX" to "#if defined(XXX)" changes are not necessary.
It is better to use #ifdef XXX consistently.

In _winnt.h, you should define _PR_NEED_SECRET_AF in the section
for "Internal configuration macros".

In primpl.h, the comment for the secret->af field needs to be
updated.

In ptio.c, the following change should be undone:

>-#ifdef _PR_STRICT_ADDR_LEN
>+#if defined(_PR_NEED_SECRET_AF)
>     if (addr)
>     {
>         /*
>          * Set addr->raw.family just so that we can use the
>          * PR_NETADDR_SIZE macro.
>          */
>         addr->raw.family = fd->secret->af;
>+#ifdef _PR_STRICT_ADDR_LEN
>         addr_len = PR_NETADDR_SIZE(addr);
>+#endif
>     }
> #endif

This whole block of code is only necessary for the _PR_STRICT_ADDR_LEN
case.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

> The _PR_NEED_SECRET_AF macro name is a little hard to understand.

Wan-Teh, I'm willing to change it to a better name, but I don't know what
is hard to understand about it, so I don't know how to improve it.  
Please suggest an alternative that you find easier to understand.

_PR_NEED_AF_IN_SECRET  ?
_PR_STORE_AF_IN_SECRET ?
_PR_KEEP_THE_AF        ?
_PR_REMEMBER_THE_AF    ?
_PR_AFFABLE            ?
_PR_AFLAC              ?  (:-)

Would it be OK to remove that ifdef, and make af be an unconditional part 
of the secret structure on all platforms, whether it was needed or not?

Comment 11

[Wan-Teh Chang]

Don't worry about the macro name.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch, as checked in

This is how I addressed Wan-teh's review comments.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Fixed on trunk

include/md/_openvms.h;    new revision: 1.19;  previous revision: 1.18
include/md/_winnt.h;      new revision: 3.33;  previous revision: 3.32
include/private/primpl.h; new revision: 3.87;  previous revision: 3.86
src/io/prsocket.c;        new revision: 3.59;  previous revision: 3.58
src/md/windows/ntio.c;    new revision: 3.44;  previous revision: 3.43
src/pthreads/ptio.c;      new revision: 3.106; previous revision: 3.105

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

reopening. I also need to fix this on the branch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

On branch:

pr/include/md/_openvms.h;    new revision: 1.18.2.1;  previous: 1.18
pr/include/md/_winnt.h;      new revision: 3.30.2.1;  previous: 3.30
pr/include/private/primpl.h; new revision: 3.84.2.2;  previous: 3.84.2.1
pr/src/io/prsocket.c;        new revision: 3.57.2.1;  previous: 3.57
pr/src/md/windows/ntio.c;    new revision: 3.40.2.2;  previous: 3.40.2.1
pr/src/pthreads/ptio.c;      new revision: 3.101.2.2; previous: 3.101.2.1

Comment 16

[Wan-Teh Chang]

Comment on attachment 273745 [details] [diff] [review]
Patch, as checked in

Thank you, Nelson.

In pr/include/private/primpl.h, we should not mention accept specifically
so that this comment will stay correct when we have other reasons to save
the address family in fd->secret->af.  It is fine to just fix this comment
on the trunk.

>+#ifdef _PR_NEED_SECRET_AF
>+    PRUint16 af;        /* If the platform's implementation of accept()
>+                         * requires knowing the address family of the 
>+			 * socket, we save the address family here. */
> #endif