Closed Bug 532476 Opened 15 years ago Closed 15 years ago

Memory leak in mailnews/mime/src/mimemoz2.cpp.

Categories

(MailNews Core :: MIME, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1a1

People

(Reporter: adam.buchbinder, Assigned: adam.buchbinder)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.10 (karmic) Firefox/3.5.5
Build Identifier: comm-central commit e7199a17fcc4

cppcheck, a static code analysis tool, reported:

[mailnews/mime/src/mimemoz2.cpp:1990]: (error) Memory leak: tmpPtr

Reproducible: Always




In mimeSetNewURL(), tmpPtr is allocated but not deallocated; the pointer goes out of scope at the end of the function, but the memory is still allocated, causing a leak.

I'm not attaching a patch because I'm not absolutely certain of how to proceed here; I want to use free() since the memory was allocated via strdup(), but why are we duplicating the string in the first place; why not just set msd->url_name to tmpPtr directly and avoid having to free anything?

I'm marking this as minor, because I haven't been able to generate an actual crash or detectable leak. Please advise if I should mark this sort of issue with a different severity.
Adam could you provide a patch for this one too ?
I didn't submit a patch because I wasn't sure of the correct way to fix it; this is my best guess, so please let me know if it needs to be changed.

I'm guessing that mimeSetNewURL() doesn't own the data pointed to by 'url', so I'm not putting that into an internal structure. However, since the function here allocates tmpPtr itself, there's no such restriction on it. Finally, if this function is called repeatedly (and assuming the caller allocates and frees the 'url' string properly), it shouldn't leak memory.
Attachment #415902 - Flags: superreview?(bugzilla)
Attachment #415902 - Flags: review?(bienvenu)
Comment on attachment 415902 [details] [diff] [review]
Patch to current tip to avoid duplicating tmpPtr.

or we could change url_name to an nsCString and avoid all this.
thx for looking at this, Adam. Does the cppcheck code turn up other issues?
Comment on attachment 415902 [details] [diff] [review]
Patch to current tip to avoid duplicating tmpPtr.

making it an nsCString would cause a big ripple effect. So this is fine.

While you're here, you could change this:

   if (!tmpPtr)
     return NS_ERROR_FAILURE;

to return NS_ERROR_OUT_OF_MEMORY.
Attachment #415902 - Flags: review?(bienvenu) → review+
Comment on attachment 415902 [details] [diff] [review]
Patch to current tip to avoid duplicating tmpPtr.

sr=Standard8 with the change that David mentioned.
Attachment #415902 - Flags: superreview?(bugzilla) → superreview+
Assignee: nobody → adam.buchbinder
Status: UNCONFIRMED → NEW
Ever confirmed: true
I've updated the patch to change the error code returned. Thanks for the feedback.

(Also, cppcheck reports plenty of other errors; I'm reporting them as I understand them to avoid wasting time with false positives.)
Attachment #415902 - Attachment is obsolete: true
Attachment #417111 - Flags: superreview?(bugzilla)
Attachment #417111 - Flags: review?(bienvenu)
Comment on attachment 417111 [details] [diff] [review]
Patch to current tip to avoid duplicating tmpPtr.

looks good, thx.
Attachment #417111 - Flags: review?(bienvenu) → review+
Attachment #417111 - Flags: superreview?(bugzilla) → superreview+
Checked in: http://hg.mozilla.org/comm-central/rev/73f80416a378

Thanks for the patch Adam.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: