MLK: leak in nsProfile::GetCurrentProfileDir()

VERIFIED FIXED in M7

Status

Core Graveyard
Profile: BackEnd
P3
critical
VERIFIED FIXED
19 years ago
2 years ago

People

(Reporter: Bruce Mitchener, Assigned: racham)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

19 years ago
This leads to the largest leak in M6 (over time).

nsGlobalHistory::GetHistoryDir() calls nsProfile::GetCurrentProfileDir().
GetCurrentProfileDir() calls GetCurrentProfile() with the address of a char*
profileName.

nsProfile::GetCurrentProfile() does:

*profileName = (char*) PR_Malloc(sizeof(char) * _MAX_LENGTH);

A couple of lines later, it does (when an operation has failed):

profileName = '\0';

This would appear to be the source of the leak, as back up one stack frame in
nsProfile::GetCurrentProfileDir(), it checks to see if profileName (a local var
in that scope) is null or not, and if not, it PR_DELETE()s it.  this assignment
of profileName to '\0' while it is a char** would appear incorrect.  If it is
correct, then the memory needs to be freed up somewhere.

For a related bug (and why this code is getting called so often), check out bug
#6922.

Comment 1

19 years ago
selmer, racham, gayatrib;  is there anyone to look at this over the weekend?
(Reporter)

Comment 2

19 years ago
Okay. Nothing so complicated as all that.  Found the problem and going to attach
a patch in about 5 minutes.  Don't know the profileName = '\0' thing is a
problem or not, but m_reg->GetString() allocates memory, so you don't need to
allocate memory before passing the char** into it.
(Reporter)

Comment 3

19 years ago
Created attachment 242 [details] [diff] [review]
patch to fix huge memory leak
(Reporter)

Comment 4

19 years ago
Well, looking around a bit more, there's another instance of this same thing
happening elsewhere.  Lines 933 and 934 of version 1.11 of nsProfile.cpp down in
nsProfile::CopyRegKey() have the same errors.  These mallocs should not be
needed.  A quick glance at all of the other uses of nsRegistry::GetString()
don't seem to show any other violations.

Updated

19 years ago
Target Milestone: M6

Comment 5

19 years ago
bruce,  do you want to check the patch in for M6?
(Reporter)

Comment 6

19 years ago
sure. will do so today.
(Reporter)

Comment 7

19 years ago
Fix checked in for the main problem.  Don't want to mark this fixed until the
whole file has been reviewed and fixed.
(Reporter)

Comment 8

19 years ago
I nailed down the critical portion.  Should we move the rest of the bug (the
review of the rest of the file) to M7?

Updated

19 years ago
Target Milestone: M6 → M7

Comment 9

19 years ago
ok. -> m7.  how big is the current leak after your first fix?

Comment 10

19 years ago
It's really a pain to have a single bug track multiple problems/actions.
Bhuvan, please open a new bug for the review process and then mark this one
fixed.  Steve
(Assignee)

Updated

19 years ago
Status: NEW → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

19 years ago
Bug #6984 to continue the review of the other segments of profile code.

Updated

19 years ago
Status: RESOLVED → VERIFIED

Updated

19 years ago
Component: Profile Manager → Profile Manager BackEnd

Comment 12

19 years ago
Moving all Profile Manager bugs to new Profile Manager Backend component.
Profile Manager component to be deleted.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.