318877 - NSPR initialization crashes if OS_TARGET is WINNT on AMD64

Reporter

Description

•

19 years ago

NSPR initialization crashes if NSPR is built 64-bit with OS_TARGET=WINNT on x86_64 . Here is the stack, as seen from shlibsign :

 	ntdll.dll!0000000078ef3320() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
>	libnspr4.dll!PR_Assert(const char * s=0x000000000067b910, const char * file=0x000000000067b940, int ln=238)  Line 538	C
 	libnspr4.dll!_PR_InitMW()  Line 239	C
 	libnspr4.dll!_PR_InitStuff()  Line 245	C
 	libnspr4.dll!_PR_ImplicitInitialization()  Line 265	C
 	libnspr4.dll!PR_NewLock()  Line 197	C
 	nss3.dll!00000000004ea7eb() 	
 	nss3.dll!000000000048525e() 	
 	nss3.dll!0000000000485e72() 	
 	shlibsign.exe!main(int argc=4, char * * argv=0x000000000088ddb0)  Line 224 + 0xc bytes	C
 	shlibsign.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	kernel32.dll!0000000078d5965c() 	

If I set OS_TARGET to WIN95 and build 64-bit , I don't see this crash.

Wan-Teh Chang

Assignee

Comment 1

•

19 years ago

The assertion that failed is at line 238:

231 void _PR_InitMW(void)
232 {
233 #ifdef WINNT
234     /*
235      * We use NT 4's InterlockedCompareExchange() to operate
236      * on PRMWStatus variables.
237      */
238     PR_ASSERT(sizeof(PVOID) == sizeof(PRMWStatus));
239     TimerInit();
240 #endif

Looking at the current InterlockedCompareExchange documentation
in MSDN, I think that assertion should be

238     PR_ASSERT(sizeof(LONG) == sizeof(PRMWStatus));

and all the (PVOID *) and (PVOID) type casts in related code
should be changed to (LONG *) and (LONG) type casts.

Julien Pierre

Reporter

Comment 2

•

19 years ago

Attached patch working patch (obsolete) — Details — Splinter Review

Thanks, Wan-Teh. Your suggestions solved the problem and allowed shlibsign in the NSS build to run successfully. This is the patch that made it work.

Attachment #205106 - Flags: review?(wtchang)

Wan-Teh Chang

Assignee

Comment 3

•

19 years ago

Comment on attachment 205106 [details] [diff] [review]
working patch

Thanks, Julien.  All the (PVOID) casts need to be
changed to (LONG) casts.  Here is an example:

>-    if (InterlockedCompareExchange((PVOID *)&desc->outcome,
>+    if (InterlockedCompareExchange((LONG *)&desc->outcome,
>         (PVOID)PR_MW_TIMEOUT, (PVOID)PR_MW_PENDING) != (PVOID)PR_MW_PENDING)

Also, please test the patch under MSVC 6.0.

Attachment #205106 - Flags: review?(wtchang) → review-

Julien Pierre

Reporter

Comment 4

•

19 years ago

Attached patch updated patch — Details — Splinter Review

Tested successfully under VC6 .

Attachment #205106 - Attachment is obsolete: true

Attachment #205209 - Flags: review?(wtchang)

Wan-Teh Chang

Assignee

Comment 5

•

19 years ago

Comment on attachment 205209 [details] [diff] [review]
updated patch

I think this patch is fine, but it generates the following
compiler warnings under MSVC 6.0 with the winbase.h header
that comes with MSVC 6.0.   That's why we are using PVOID
instead of LONG in the current source code.  I hope we can
figure out a solution to eliminate the compiler warnings,
but I don't know how to determine the "version" of the
winbase.h header.

If we can't figure out a solution, I am willing to live
with these compiler warnings.

sh ../../../../mozilla/nsprpub/build/cygwin-wrapper cl -Foprmwait.obj -c      -W
3 -nologo -GF -Gy -MD -GT -G5 -O2   -UDEBUG -U_DEBUG  -DNDEBUG=1 -DXP_PC=1 -DWIN
32=1 -DWINNT=1 -D_X86_=1  -DFORCE_PR_LOG -D_NSPR_BUILD_ -I../../../dist/include/
nspr -I../../../../mozilla/nsprpub/pr/include -I../../../../mozilla/nsprpub/pr/i
nclude/private  /cygdrive/c/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsp
rpub/pr/src/io/prmwait.c
prmwait.c
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
671) : warning C4047: 'function' : 'void ** ' differs in levels of indirection f
rom 'long *'
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
671) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 1
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
672) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 2
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
672) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 3
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
672) : warning C4047: '!=' : 'void *' differs in levels of indirection from 'lon
g '
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
852) : warning C4047: 'function' : 'void ** ' differs in levels of indirection f
rom 'long *'
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
852) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 1
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
853) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 2
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
853) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actual
 parameter 3
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
854) : warning C4047: '==' : 'void *' differs in levels of indirection from 'lon
g '
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1097) : warning C4047: 'function' : 'void ** ' differs in levels of indirection
from 'long *'
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1097) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 1
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1098) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 2
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1098) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 3
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1098) : warning C4047: '==' : 'void *' differs in levels of indirection from 'lo
ng '
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1188) : warning C4047: 'function' : 'void ** ' differs in levels of indirection
from 'long *'
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1188) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 1
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1189) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 2
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1189) : warning C4022: 'InterlockedCompareExchange' : pointer mismatch for actua
l parameter 3
c:/nspr-tip/win95.opt/pr/src/io/../../../../mozilla/nsprpub/pr/src/io/prmwait.c(
1190) : warning C4047: '==' : 'void *' differs in levels of indirection from 'lo
ng '

Reed Loden [:reed]

Updated

•

18 years ago

QA Contact: wtchang → nspr

Makoto Kato [:m_kato] (PTO until 5/6)

Comment 6

•

17 years ago

(In reply to comment #0)

Although I believe that current NSPR of Firefox branch supports x64 Windows build, could you verify it?

After that, I think that you should close this as WORKFORME.

Julien Pierre

Reporter

Comment 7

•

17 years ago

Wan-Teh, I don't know of a way to detect the winbase version eithre. The patch works with both vc6 and vc8, in both 32 and 64-bit modes. The only problem is the warnings with vc6. But it is still correct. I think we should just check it in. Please give r+ if you agree.

Julien Pierre

Reporter

Comment 8

•

17 years ago

Makoto, Firefox uses the OS_TARGET=WIN95 version of NSPR, not OS_TARGET=WINNT, so it will not run into this bug. Sun server products use OS_TARGET=WINNT .

Julien Pierre

Reporter

Comment 9

•

17 years ago

Comment on attachment 205209 [details] [diff] [review]
updated patch

Wan-Teh, please re-review. I believe the compiler warnings this patch generates with some versions of compilers and Windows SDK are minor and the value of the patch outweighs them.

Attachment #205209 - Flags: review?(wtc) → superreview?(wtc)

Wan-Teh Chang

Assignee

Comment 10

•

17 years ago

Comment on attachment 205209 [details] [diff] [review]
updated patch

r=wtc.

Attachment #205209 - Flags: superreview?(wtc) → superreview+

Julien Pierre

Reporter

Comment 11

•

17 years ago

Thanks for the review, Wan-Teh. I checked this in to the NSPR trunk .

Checking in prmwait.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prmwait.c,v  <--  prmwait.c
new revision: 3.19; previous revision: 3.18
done

Status: NEW → RESOLVED

Closed: 17 years ago

Priority: -- → P2

Resolution: --- → FIXED

Target Milestone: --- → 4.7

Mike Kaply [:mkaply]

Comment 12

•

16 years ago

Sorry to comment in an old bug, just had a question. We had made these changes so we could get win64 working with an earlier NSPR.

We also had to make this change:

	#if !defined(_AMD64_)				=== > added
	    typedef #define _PR_HAVE_ATOMIC_CAS
	#endif						=== > added

Any reason why you didn't have to make that change?

Note this is thirdhand on my part, so I have no experience with the code, I just have the diff.

Wan-Teh Chang

Assignee

Comment 13

•

16 years ago

We made an equivalent change to
mozilla/nsprpub/pr/include/md/_winnt.h in rev. 3.31:

#if defined(_M_IX86) || defined(_X86_)  === > added
#define _PR_HAVE_ATOMIC_CAS
#endif                                  === > added

See the patch attachment 200365 [details] [diff] [review] in bug 225859.

working patch 19 years ago Julien Pierre 4.20 KB, patch	wtc : review-	Details \| Diff \| Splinter Review
updated patch 19 years ago Julien Pierre 4.69 KB, patch	wtc : superreview+	Details \| Diff \| Splinter Review

Bugzilla

Quick Search

NSPR initialization crashes if OS_TARGET is WINNT on AMD64

Categories

(NSPR :: NSPR, defect, P2)

Tracking

(Not tracked)

People

(Reporter: julien.pierre, Assigned: wtc)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file, 1 obsolete file)

Description

Comment 1

Comment 2

Comment 3

Comment 4

Comment 5

Updated

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Comment 11

Comment 12

Comment 13

Attachment

General

Description

File Name

Content Type