Closed Bug 1441828 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: fan.gang.cn, Unassigned)

Details

Attachments

(1 file)

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
Marco, I see you are familiar with nsBrowserApp.cpp, could you please advise where this bug should go under? Maybe toolkit?
Flags: needinfo?(mcastelluccio)
Maybe Toolkit :: Startup and Profile System?
Component: Untriaged → Startup and Profile System
Flags: needinfo?(mcastelluccio)
Product: Firefox → Toolkit
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.
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
(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.
(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.
(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.
setenv is essentially implemented the same way we call putenv. It just hides the leak in the libc instead of our code.
You need to log in before you can comment on or make changes to this bug.