nsSpecialSystemDirectory.cpp UMR & Misuse of GetEnvironmentVariable.

VERIFIED FIXED

Status

()

P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

({crash})

Trunk
x86
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dogfood+])

(Assignee)

Description

18 years ago
The change to -r1.31 of nsSpecialSystemDirectory.cpp under line 587 is a little 
broken.

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsSpecialSystemDirectory.cpp#58
7

This appears to cause a UMR in Purify. GetEnvironmentVariable is documented to 
return 0 if the var was not found in the environment. This leaves you calling 
PL_strlen on an uninitialied buffer. It looks like you cloned or modified 
mcmullen's code.

I don't know why it is using ">= 0". It looks like '> 0' is appropriate since it 
looks like you only want it to go this way if "HOME" is set.

It is not clear to me if the case below that where it checks for "HOMEDRIVE" 
ought to be '>= 0' or '> 0'. 

If '>= 0' *is* appropriate, then you should do "path[0] = 0;" before the call to 
GetEnvironmentVariable to ensure that the buffer is safely zero terminated 
first.

Note that release builds will leave garbage in auto variables, while debug 
builds generally initialize them to 0.
(Assignee)

Updated

18 years ago
Keywords: nsbeta3

Comment 1

18 years ago
This is a *very* likely cause of bug 48110 which keeps *many* people from using 
the release build of the product. Having a bit of code like this that will work 
consistently in debug builds and be unpredicatable in release builds is *very* 
dangerous. I suggest that one of you people who wrote, reviewed, or checked in 
this faulty code DROP EVERYTHING RIGHT NOW and just fix this broken code.
Keywords: crash, dogfood

Comment 2

18 years ago
Putting on [dogfood+] radar.
Whiteboard: [dogfood+]
(Assignee)

Comment 3

18 years ago
I will fix it today.  
Status: NEW → ASSIGNED
(Assignee)

Comment 4

18 years ago
jband is correct.  >= is evil.  When I added the check for the HOME directory, I
just copied and pasted the checks for HOMEDIR/HOMEPATH. I should have review the
calls.  

The fix has been reviewed and tested.  I will check it in when the tree opens.

Comment 5

18 years ago
For what it's worth, as the reporter of bug 48110, I'd like to point out that 
"set home=c:\windows\temp" (just to get a HOME directory) does not fix 48110; it 
still crashes even with a HOME directory.  Of course, it's entirely possible 
that it's an additional consequence of this problem (without the simple remedy 
of setting HOME), but I figured I'd bring it to your attention (since none of 
you are on 48110's CC: list).
(Assignee)

Comment 6

18 years ago
hmm.  if you sent HOME do something and still crash, my bet is that it is
happening somewhere else.  With any luck, tomorrow will include a fix to this
area.  
(Assignee)

Comment 7

18 years ago
Fix checked in...marking as such.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 8

18 years ago
please verify

Comment 9

18 years ago
Hey Doug, I'd like to verify this bug.  Have you tested your fix out and is it ok.
Thanks.

Comment 10

18 years ago
Marking Verified.  Please reopen if problem reoccurs.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.