Closed
Bug 532476
Opened 15 years ago
Closed 15 years ago
Memory leak in mailnews/mime/src/mimemoz2.cpp.
Categories
(MailNews Core :: MIME, defect)
MailNews Core
MIME
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.1a1
People
(Reporter: adam.buchbinder, Assigned: adam.buchbinder)
Details
Attachments
(1 file, 1 obsolete file)
752 bytes,
patch
|
Bienvenu
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•15 years ago
|
||
Adam could you provide a patch for this one too ?
Assignee | ||
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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.
Comment 4•15 years ago
|
||
thx for looking at this, Adam. Does the cppcheck code turn up other issues?
Comment 5•15 years ago
|
||
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 6•15 years ago
|
||
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+
Updated•15 years ago
|
Assignee: nobody → adam.buchbinder
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 7•15 years ago
|
||
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 8•15 years ago
|
||
Comment on attachment 417111 [details] [diff] [review] Patch to current tip to avoid duplicating tmpPtr. looks good, thx.
Attachment #417111 -
Flags: review?(bienvenu) → review+
Updated•15 years ago
|
Attachment #417111 -
Flags: superreview?(bugzilla) → superreview+
Updated•15 years ago
|
Keywords: checkin-needed
Comment 9•15 years ago
|
||
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.
Description
•