NT version of PR_Accept on IPv6 listen socket fails with INVALID_ARGS

RESOLVED FIXED in 4.6.8

Status

defect
P1
major
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: nelson, Assigned: nelson)

Tracking

4.6.8
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted file 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.
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. :(
Priority: -- → P1
Target Milestone: --- → 4.6.8
This bug goes back to rev 1.1, which was in NSPR 3.0
Version: 4.7 → 3.0
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.
Posted patch proposed patch, v1 (obsolete) — Splinter Review
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.
Assignee: wtc → nelson
Status: NEW → ASSIGNED
Attachment #272102 - Flags: review?
Comment on attachment 272102 [details] [diff] [review]
proposed patch, v1

Wan-Teh, please review.
Attachment #272102 - Flags: review? → review?(wtc)

Comment 6

12 years ago
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.
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.  
Posted patch proposed patch v2 (obsolete) — Splinter Review
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.
Attachment #272102 - Attachment is obsolete: true
Attachment #272373 - Flags: review?(wtc)
Attachment #272102 - Flags: review?(wtc)
Attachment #272373 - Attachment is patch: true

Comment 9

12 years ago
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.
Attachment #272373 - Flags: review?(wtc) → review+
> 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

12 years ago
Don't worry about the macro name.
This is how I addressed Wan-teh's review comments.
Attachment #272373 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
reopening. I also need to fix this on the branch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 16

12 years ago
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
Attachment #273745 - Flags: review+
Blocks: 402669
You need to log in before you can comment on or make changes to this bug.