Closed
Bug 679612
Opened 13 years ago
Closed 13 years ago
cppcheck indicates resource leak in APKOpen.cpp
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla9
People
(Reporter: david.volgyes, Assigned: atulagrwl)
References
Details
(Whiteboard: [good first bug] [mentor=jdm])
Attachments
(1 file)
(deleted),
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110812233755
Steps to reproduce:
cppcheck found some error in other-licenses/android/APKOpen.cpp.
Actual results:
The warnings:
[other-licenses/android/APKOpen.cpp:293]: (error) Resource leak: fd
[other-licenses/android/APKOpen.cpp:512]: (error) Resource leak: fd
1st, 2nd: There are several exit point in APKOpen, where a file is open, and it is not closed at exit.
Expected results:
You should close the file before the exit points.
Sometimes it is tricky, because the file is memory mapped, so first you have to unmap, and close the file after the unmap.
As far as I see this is upstream code, so maybe someone should propagate the error-report to the upstream code.
Updated•13 years ago
|
Component: General → Widget: Android
OS: Linux → Android
Product: Firefox → Core
QA Contact: general → android
Hardware: x86_64 → ARM
Comment 1•13 years ago
|
||
APKOpen.cpp is all mozilla code.
Comment 2•13 years ago
|
||
If that's the case, then some enterprising person should be able to fix this up without much fuss. I think a class that closes the file handle in its destructor would be the best way to solve the problem. See http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/readstrings/readstrings.cpp#60 for an example.
Whiteboard: [good first bug] [mentor=jdm]
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•13 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #2)
> If that's the case, then some enterprising person should be able to fix this
> up without much fuss. I think a class that closes the file handle in its
> destructor would be the best way to solve the problem. See
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/readstrings/
> readstrings.cpp#60 for an example.
In the case of APKOpen.cpp, we aim to use as few C++ features as possible. IIRC it's c++ due to linking issues.
Comment 4•13 years ago
|
||
In that case ignore comment 2. Let's just add fclose calls to every exit point in the function.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #556394 -
Flags: review?(josh)
Updated•13 years ago
|
Attachment #556394 -
Flags: review?(mwu)
Comment 6•13 years ago
|
||
mwu should review all changes to this file
Updated•13 years ago
|
Attachment #556394 -
Flags: review?(josh)
Comment 7•13 years ago
|
||
Comment on attachment 556394 [details] [diff] [review]
v1 patch for close(fd)
Heh.
Attachment #556394 -
Flags: review?(mwu) → review+
Updated•13 years ago
|
Assignee: nobody → atulagrwl
Keywords: checkin-needed
Comment 9•13 years ago
|
||
Quick sanity check:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=46750bfed1e1
...then onto inbound afterwards :-)
Comment 10•13 years ago
|
||
Target Milestone: --- → mozilla9
Comment 11•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•