Closed Bug 294017 Opened 19 years ago Closed 19 years ago

IsValidNetAddrLen returns PR_FALSE on ipv6 address type

Categories

(NSPR :: NSPR, defect)

x86
Windows XP
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bambas, Assigned: shanmus)

Details

Attachments

(6 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050331 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050331 Firefox/1.0+

nsprpub/pr/src/io/prsocket.c line 1082, function SocketGetName. Call of
IsValidNetAddrLen returns PR_FALSE on the ip address what is ipv6 family. This
value is passed to PR_ASSERT macro and application fails.

This address is obtained with call of _PR_MD_GETSOCKNAME what also returns the
size of gettered address data. 

_PR_MD_GETSOCKNAME on windows xp platform calls getsockname function from
winsock. This method is an originator of the address length value. Value
returned by getsockname is 28 bytes.

Oposite value what is the above one tested against is size of PRNetAddr::ipv6
member. This value is 32 bytes. It is obtaned using macro PR_NETADDR_SIZE
(nsprpub\pr\include\private\primpl.h line 1425)

If all calls of PR_ASSERT(IsValidNetAddrLen(addr, len) == PR_TRUE) are commented
everything works fine - means using ipv6 binded server (see Steps to Reproduce).

Reproducible: Always

Steps to Reproduce:
Start nsIServerSocket initiated to ipv6 address like this:
	nsCOMPtr<nsIServerSocket> piServerSocketIpv6;
...
	PRNetAddr addr;
	PR_SetNetAddr(PR_IpAddrAny, PR_AF_INET6, nPort, &addr);
	rv = piServerSocketIpv6->InitWithAddress(&addr, 5, type);

Inside InitWithAddress fails the PR_ASSERT(IsValidNetAddrLen(addr, len) == PR_TRUE).
Shanmu, could you take a look at this?

I believe this bug is similar to bug 40542 comment 24 and bug 51135.
There is a mismatch between the size of struct sockaddr_in6 (28 bytes)
and the ipv6 union member of PRNetAddr (32 bytes).  This is caused by
the definition of struct in6_addr (see <ws2tcpip.h>):

struct in6_addr {
    union {
        u_char Byte[16];
        u_short Word[8];
    } u;
};

This structure only needs to be 2-byte aligned.

Compare this to struct PRIPv6Addr (see "prio.h"):

struct PRIPv6Addr {
        union {
                PRUint8  _S6_u8[16];
                PRUint16 _S6_u16[8];
                PRUint32 _S6_u32[4];
                PRUint64 _S6_u64[2];
        } _S6_un;
};

The PRUint64 union member causes this structure to be
8-byte aligned.

The fix should be similar to the code in _solaris.h and
_openvms.h (in mozilla/nsprpub/pr/include/md) at and below
#define _PR_HAVE_MD_SOCKADDR_IN6.

Shanmu, could you prepare a patch for the bug reporter
to try?
Assignee: wtchang → shanmus
Status: UNCONFIRMED → NEW
Ever confirmed: true
Wan 傍eh,
	In order to make it a multiple of 4, we need to remove the field 
PRUint64 _S6_u64[2] from the union _S6_un in the structure PRIPv6Addr. 
Just want to make sure that I am doing the right thing.
Status: NEW → ASSIGNED
Shanmu,

I described the fix at the end of comment 1.

The PRIPv6Addr structure is a publicly exported data
type, so we can't change it.

What we can do is to define the _PR_HAVE_MD_SOCKADDR_IN6,
and define the _md_sockaddr_in6 structure appropriately
in _win95.h and _winnt.h.  You can use the similar code
in _openvms.h and _solaris.h as examples.
Attached patch proposed fix (obsolete) — Splinter Review
Proposed fix
Reporter,
   Can you test this fix to see if this solves this problem?
Comment on attachment 184583 [details] [diff] [review]
proposed fix

The prio.h and primpl.h changes aren't necessary.  Windows doesn't
have Unix domain sockets (for local IPC).

Please generate patches using "cvs diff -u".  The -u flag generates
unified diffs, which are easier to code review.
				\
>+struct _md_in6_addr {
>+	    union {
>+	        PRUint8  _S6_u8[16];
>+	        PRUint32 _S6_u32[4];
>+	        PRUint32 __S6_align;
>+	    } _S6_un;
>+};

This is not isomorphic to the struct in6_addr definition on Windows
(in <ws2tcpip.h>):

struct in6_addr {
    union {
	u_char Byte[16];
	u_short Word[8];
    } u;
};
Attachment #184583 - Flags: review-
My ws2tcpip.h under C:/cygwin/usr/include/w32api has the in6_addr defined as

struct in6_addr {
    union {
        u_char	_S6_u8[16];
        u_short	_S6_u16[8];
        u_long	_S6_u32[4];
        } _S6_un;
};

Am I looking at the wrong file?
Based on the comment on ws2tcpip.h, I am attaching a new fix using "cvs diff
-u"(without the primpl.h and prio.h changes).
I have compiled mozilla with all three patches. With these results:

attachment 184583 [details] [diff] [review] (both versions):
priometh.c
\nsprpub\pr\src\io\priometh.c(318) : error C2143: syntax error : missing ')'
before ';'

attachment 184665 [details] [diff] [review]:
priometh.c
_obj-browser\dist\include\nspr\md\_win95.h(81) : error C2020: '_S6_u32' :
'union' member redefinition
\nsprpub\pr\src\io\priometh.c(318) : error C2143: syntax error : missing ')' 

where _obj-browser\ is my object folder.

Also there are problems applying patches 184583 - files not found, have to be
specified manualy.
The changes to files prio.h and primpl.h will solve this build problems. Are you
sure you changed both the files when you applied the patch 184583? 

Wan-Teh, looks like I need the changes to the prio.h and primpl.h to get this
built on a Windows machine. Please advise.

The correct way to fix the priometh.c compilation error
is to add the missing ')' to the PR_NETADDR_SIZE macro
definition, rather using the PR_NETADDR_SIZE macro
definition for Unix.

Shanmu, you looked at the wrong header file.  You
should develop NSPR with MSVC (or MinGW) and look at
the headers in Microsoft Platform SDK.	For this bug
the header file of interest is ws2tcpip.h.  In any
case, I've extracted the struct in6_addr definition
from ws2tcpip.h for you.
Attachment #184583 - Attachment is obsolete: true
Comment on attachment 184665 [details] [diff] [review]
Changed the fix as per Wan-Teh's comments.

Shanmu, another comment:

>+#define _PR_HAVE_MD_SOCKADDR_IN6
>+/* similar to struct in6_addr on Windows (ws2tcpip.h) */
>+struct _md_in6_addr {
>+	    union {
>+		PRUint8  _S6_u8[16];
>+	        PRUint16 _S6_u16[8];
>+	        PRUint32 _S6_u32[4];
>+	    } _S6_un;
>+};

In NSPR, avoid using tabs unless the original code uses it.

The reason we avoid tabs is that people disagree what the
right tab stop should be.  The default tab stop in vi is 8.
Many people use 4.  Some people use 2.	So, best to avoid
tabs and just indent with four spaces.
Compiled succesfully and succesfully tested on Windows XP.
Attachment #184911 - Flags: review?(wtchang)
Comment #6 by Wan-Teh suggests that the structure _md_in6_addr deifined here is
not isomorphic to the one defined in ws2tcpip.h. 
This patch fixes the priometh.c compilation error and has the structure
_md_in6_addr isomorphic to the one defined in ws2tcpip.h.
Comment on attachment 185663 [details] [diff] [review]
Fix with the structure  _md_in6_addr isomorphic to Windows

>Index: pr/include/md/_win95.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v
>retrieving revision 3.29
>diff -u -r3.29 _win95.h
>--- pr/include/md/_win95.h	25 Apr 2004 15:00:47 -0000	3.29
>+++ pr/include/md/_win95.h	8 Jun 2005 11:55:20 -0000
>@@ -71,6 +71,23 @@
>     struct sockaddr *ai_addr;
>     struct addrinfo *ai_next;
> };
>+#define _PR_HAVE_MD_SOCKADDR_IN6
>+/* isomorphic to struct in6_addr on Windows */
>+struct _md_in6_addr {
>+    union {
>+        PRUint8  _S6_u8[16];
>+        PRUint32 _S6_u32[4];
>+    } _S6_un;
>+};
>+/* isomorphic to struct sockaddr_in6 on Windows */
>+struct _md_sockaddr_in6 {
>+    PRUint16 sin6_family;
>+    PRUint16 sin6_port;
>+    PRUint32 sin6_flowinfo;
>+    struct _md_in6_addr sin6_addr;
>+    PRUint32 sin6_scope_id;
>+};
> #endif
> #endif
> #define _PR_HAVE_THREADSAFE_GETHOST
>Index: pr/include/md/_winnt.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/md/_winnt.h,v
>retrieving revision 3.29
>diff -u -r3.29 _winnt.h
>--- pr/include/md/_winnt.h	25 Apr 2004 15:00:47 -0000	3.29
>+++ pr/include/md/_winnt.h	8 Jun 2005 11:55:21 -0000
>@@ -86,6 +86,23 @@
>     struct sockaddr *ai_addr;
>     struct addrinfo *ai_next;
> };
>+#define _PR_HAVE_MD_SOCKADDR_IN6
>+/* isomorphic to struct in6_addr on Windows */
>+struct _md_in6_addr {
>+    union {
>+        PRUint8  _S6_u8[16];
>+        PRUint32 _S6_u32[4];
>+    } _S6_un;
>+};
>+/* isomorphic to struct sockaddr_in6 on Windows */
>+struct _md_sockaddr_in6 {
>+    PRUint16 sin6_family;
>+    PRUint16 sin6_port;
>+    PRUint32 sin6_flowinfo;
>+    struct _md_in6_addr sin6_addr;
>+    PRUint32 sin6_scope_id;
>+};
> #endif
> #endif
> #define _PR_HAVE_THREADSAFE_GETHOST
>Index: pr/include/private/primpl.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v
>retrieving revision 3.83
>diff -u -r3.83 primpl.h
>--- pr/include/private/primpl.h	17 Jan 2005 22:00:53 -0000	3.83
>+++ pr/include/private/primpl.h	8 Jun 2005 11:55:25 -0000
>@@ -1409,7 +1409,7 @@
> #define PR_NETADDR_SIZE(_addr) 					\
>         ((_addr)->raw.family == PR_AF_INET		\
>         ? sizeof((_addr)->inet)					\
>-        : sizeof(struct _md_sockaddr_in6)
>+        : sizeof(struct _md_sockaddr_in6))
> #endif /* defined(XP_UNIX) */
> 
> #else
This patch is not working. Compilation is success but PR_NETADDR_SIZE macro
returns 32 bytes lenght. Length of the PRNetAddres is corrent (28 bytes)

IsValidNetAddrLen return false and assertions fail.
Attached patch Revised patchSplinter Review
In my previous patch, by mistake, I added the member PRUint32 __sin6_src_id in
the structure _md_sockaddr_in6, which made the size 32. I am sure this new
pacth will fix this.
Attachment #184911 - Flags: review?(wtchang) → review-
Comment on attachment 186065 [details] [diff] [review]
Revised patch

Shanmu, this patch still has two problems.  First, the
definition of struct _md_in6_addr is not isomorphic to
struct in6_addr because PRUint32 _S6_u32[4] is not
isomorphic to u_short Word[8].

Second, please move the new code to the outside of the

  #ifndef AI_CANONNAME
  ...
  #endif

block in _win95.h and _winnt.h.  See the code that
uses _PR_HAVE_MD_SOCKADDR_IN6 and struct _md_sockaddr_in6
in primpl.h; these two are used if _PR_INET6 is not
defined.  So the new code must be there whether AI_CANONNAME
is defined or not.
Attachment #186065 - Flags: review-
Attached patch Revised patchSplinter Review
I have included the changes suggested by Wan-Teh in comment #19.
(In reply to comment #20)
> Created an attachment (id=186577) [edit]
> Revised patch
> 
> I have included the changes suggested by Wan-Teh in comment #19.

I compiled and tested this patch. It works well for me - address sizes are correct.
Comment on attachment 186577 [details] [diff] [review]
Revised patch

Thank you, Shanmu. r=wtc.
Attachment #186577 - Flags: review+
I checked in the last patch after minor editing (changed
"PRUint16 sin6_family" to "PRInt16 sin6_family" to better match
"short sin6_family" in struct sockaddr_in6) on the NSPR trunk
for NSPR 4.6.1.

Checking in md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.30; previous revision: 3.29
done
Checking in md/_winnt.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_winnt.h,v  <--  _winnt.h
new revision: 3.30; previous revision: 3.29
done
Checking in private/primpl.h;
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v  <--  primpl.h
new revision: 3.84; previous revision: 3.83
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.6.1
(In reply to comment #12)
> The reason we avoid tabs is that people disagree what the
> right tab stop should be.  The default tab stop in vi is 8.
> Many people use 4.  Some people use 2.  So, best to avoid
> tabs and just indent with four spaces.

Everyone is welcome to set the tabstop to whatever they want in their editor.

By mandating the use of 4-spaces, you are taking away that liberty and force everyone to edit with a 4-position tabstop.
You need to log in before you can comment on or make changes to this bug.