49866 - nsSpecialSystemDirectory.cpp UMR & Misuse of GetEnvironmentVariable.

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[Doug Turner (:dougt)]

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.

Comment 1

[John Bandhauer]

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.

Comment 2

[leger]

Putting on [dogfood+] radar.

Comment 3

[Doug Turner (:dougt)]

I will fix it today.  

Comment 4

[Doug Turner (:dougt)]

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

[Mark Anderson]

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).

Comment 6

[Doug Turner (:dougt)]

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.  

Comment 7

[Doug Turner (:dougt)]

Fix checked in...marking as such.

Comment 8

[Tracy Walker [:tracy]]

please verify

Comment 9

[Paul Wyskoczka]

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

Comment 10

[leger]

Marking Verified.  Please reopen if problem reoccurs.