231195 - NSPR_LOG_FILE=WinDebug will crash if PR_LogCleanup is called

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[timeless]

Environment:
NSPR_LOG_FILE=WinDebug
NSPR_LOG_MODULES=all:5

Steps:
build mozilla
fatal step:
running shlibsign.exe
commandline:
r:\______\mozilla\opt-i586-pc-msvc\nss\shlibsign.exe -v -i
r:/______/mozilla/opt-i586-pc-msvc/dist/lib/softokn3.dll

Stack trace:
 	msvcr70.dll!fclose(_iobuf * stream=0xfffffffe)  Line 51 + 0x3	C
>	nspr4.dll!_PR_LogCleanup()  Line 273 + 0x7	C
 	nspr4.dll!PR_Cleanup()  Line 437	C
 	shlibsign.exe!004012ee() 	
 	5b5d5e5f()	

Reason:
closing a special file "Don't do that"

Comment 2

[timeless]

Attachment: patch against mozilla 1.6 branch

Comment 3

[Wan-Teh Chang]

Comment on attachment 140043 [details] [diff] [review]
patch against mozilla 1.6 branch

r=wtc.	Thanks for the bug report and the patch.

This patch is correct.	I will propose an alternate
patch though (matter of style).

Two minor issues with this patch.

1. In the second chunk of this patch, you have an
if-else statement nested with #ifdef.  That kind of
nesting is hard to understand and should be avoided
if possible.

2. In the second chunk of this patch, with your rewrite,
the statement

    return (PRBool) (newLogFile != 0);

should be changed to

    return PR_TRUE;

because newLogFile is guaranteed to be nonzero at that
point.

Comment 4

[Wan-Teh Chang]

Attachment: Alternate patch

Note that I still use #ifdef XP_PC to follow the
original usage.  I like your use of
#ifdef WIN32_DEBUG_FILE though.

Comment 5

[Wan-Teh Chang]

Comment on attachment 140089 [details] [diff] [review]
Alternate patch

OK,  I see why timeless rewrote the PR_SetLogFile
function.  Good catch, timeless.

I will attach a new patch.

Comment 6

[Wan-Teh Chang]

Attachment: Alternate patch v2

This is timeless's patch, edited to NSPR's (or
rather my personal) style.

Comment 7

[Wan-Teh Chang]

Comment on attachment 140098 [details] [diff] [review]
Alternate patch v2

Timeless, please review and test this patch.  Thanks.

Comment 8

[timeless]

Comment on attachment 140098 [details] [diff] [review]
Alternate patch v2

bugzilla's new visual diff shows that this change is fine.

Comment 9

[Wan-Teh Chang]

Fix checked in on the NSPR trunk (NSPR 4.5.1) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.7a).