Closed Bug 49866 Opened 20 years ago Closed 20 years ago

nsSpecialSystemDirectory.cpp UMR & Misuse of GetEnvironmentVariable.

Categories

(Core :: XPCOM, defect, P3)

x86
Linux
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: dougt, Assigned: dougt)

Details

(Keywords: crash, Whiteboard: [dogfood+])

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.
Keywords: nsbeta3
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
Putting on [dogfood+] radar.
Whiteboard: [dogfood+]
I will fix it today.  
Status: NEW → ASSIGNED
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.
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).
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.  
Fix checked in...marking as such.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
please verify
Hey Doug, I'd like to verify this bug.  Have you tested your fix out and is it ok.
Thanks.
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.