Closed
Bug 294017
Opened 19 years ago
Closed 19 years ago
IsValidNetAddrLen returns PR_FALSE on ipv6 address type
Categories
(NSPR :: NSPR, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
4.6.1
People
(Reporter: bambas, Assigned: shanmus)
Details
Attachments
(6 files, 1 obsolete file)
1.75 KB,
patch
|
Details | Diff | Splinter Review | |
654 bytes,
patch
|
Details | Diff | Splinter Review | |
7.12 KB,
patch
|
wtc
:
review-
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
Details | Diff | Splinter Review | |
2.33 KB,
patch
|
wtc
:
review-
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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).
Comment 1•19 years ago
|
||
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.
Comment 3•19 years ago
|
||
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.
Reporter, Can you test this fix to see if this solves this problem?
Comment 6•19 years ago
|
||
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).
Reporter | ||
Comment 9•19 years ago
|
||
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.
Assignee | ||
Comment 10•19 years ago
|
||
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.
Comment 11•19 years ago
|
||
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 12•19 years ago
|
||
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.
Reporter | ||
Comment 13•19 years ago
|
||
Compiled succesfully and succesfully tested on Windows XP.
Reporter | ||
Updated•19 years ago
|
Attachment #184911 -
Flags: review?(wtchang)
Assignee | ||
Comment 14•19 years ago
|
||
Comment #6 by Wan-Teh suggests that the structure _md_in6_addr deifined here is not isomorphic to the one defined in ws2tcpip.h.
Assignee | ||
Comment 15•19 years ago
|
||
This patch fixes the priometh.c compilation error and has the structure _md_in6_addr isomorphic to the one defined in ws2tcpip.h.
Assignee | ||
Comment 16•19 years ago
|
||
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
Reporter | ||
Comment 17•19 years ago
|
||
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.
Assignee | ||
Comment 18•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #184911 -
Flags: review?(wtchang) → review-
Comment 19•19 years ago
|
||
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-
Assignee | ||
Comment 20•19 years ago
|
||
I have included the changes suggested by Wan-Teh in comment #19.
Reporter | ||
Comment 21•19 years ago
|
||
(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 22•19 years ago
|
||
Comment on attachment 186577 [details] [diff] [review] Revised patch Thank you, Shanmu. r=wtc.
Attachment #186577 -
Flags: review+
Comment 23•19 years ago
|
||
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
Comment 24•18 years ago
|
||
(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.
Description
•