1441828 - Potential memory leak in browser/app/nsBrowserApp.cpp:179

Status: RESOLVED INVALID

(Toolkit :: Startup and Profile System, defect)

Description

[fan.gang.cn]

Attachment: Screenshot from 2018-02-28 22:20:53.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36

Steps to reproduce:

Scan Code with SourceBrella Pinpoint


Actual results:

The memory allocated by strdup(appEnv) on line 179 is never freed in the program. 

File: browser/app/nsBrowserApp.cpp:179

Comment 1

[Adrian Florinescu [:aflorinescu]]

Marco, I see you are familiar with nsBrowserApp.cpp, could you please advise where this bug should go under? Maybe toolkit?

Comment 2

[Marco Castelluccio [:marco]]

Maybe Toolkit :: Startup and Profile System?

Comment 3

[Nathan Froyd [:froydnj]]

Thanks for the report.  The string passed to putenv becomes part of the environment, so freeing it after calling putenv would be incorrect.  This behavior is not a leak.

Comment 4

[fan.gang.cn]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Thanks for the report.  The string passed to putenv becomes part of the
> environment, so freeing it after calling putenv would be incorrect.  This
> behavior is not a leak.

Thanks for your reply. 

I was not familiar with the putenv call when filling this report. After googling a while, it looks like to me putenv itself is a "dangerous" function and many people suggest use setenv() instead since setenv() will copy the string. 

Here is a note from the Linux man page. For Glibc 2.0-2.1.1 and 4.4BSD, this place will definitely result in a memory leakage. 

Would you consider use setenv() at here? 


NOTES         top
       The putenv() function is not required to be reentrant, and the one in
       glibc 2.0 is not, but the glibc 2.1 version is.

       Since version 2.1.2, the glibc implementation conforms to SUSv2: the
       pointer string given to putenv() is used.  In particular, this string
       becomes part of the environment; changing it later will change the
       environment.  (Thus, it is an error is to call putenv() with an
       automatic variable as the argument, then return from the calling
       function while string is still part of the environment.)  However,
       glibc versions 2.0 to 2.1.1 differ: a copy of the string is used.  On
       the one hand this causes a memory leak, and on the other hand it
       violates SUSv2.

       The 4.4BSD version, like glibc 2.0, uses a copy.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to fan.gang.cn from comment #4)
> Here is a note from the Linux man page. For Glibc 2.0-2.1.1 and 4.4BSD, this
> place will definitely result in a memory leakage. 

Such old systems are not really a concern here, since the minimum glibc requirement is 2.2x, and I doubt Firefox even runs on 4.4BSD.

> Would you consider use setenv() at here? 

Sure, we could do that instead.

Comment 6

[fan.gang.cn]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> (In reply to fan.gang.cn from comment #4)
> > Here is a note from the Linux man page. For Glibc 2.0-2.1.1 and 4.4BSD, this
> > place will definitely result in a memory leakage. 
> 
> Such old systems are not really a concern here, since the minimum glibc
> requirement is 2.2x, and I doubt Firefox even runs on 4.4BSD.
> 
> > Would you consider use setenv() at here? 
> 
> Sure, we could do that instead.

Got it, thanks.

Comment 7

[Mike Hommey [:glandium]]

setenv is essentially implemented the same way we call putenv. It just hides the leak in the libc instead of our code.