Last Comment Bug 679612 - cppcheck indicates resource leak in APKOpen.cpp
: cppcheck indicates resource leak in APKOpen.cpp
Status: RESOLVED FIXED
[good first bug] [mentor=jdm]
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Atul Aggarwal
:
Mentors:
Depends on:
Blocks: cppcheck
  Show dependency treegraph
 
Reported: 2011-08-16 23:39 PDT by David Volgyes
Modified: 2011-09-01 01:39 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch for close(fd) (1.67 KB, patch)
2011-08-28 08:21 PDT, Atul Aggarwal
mwu.code: review+
Details | Diff | Review

Description David Volgyes 2011-08-16 23:39:36 PDT
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.
Comment 1 Michael Wu [:mwu] 2011-08-22 02:21:29 PDT
APKOpen.cpp is all mozilla code.
Comment 2 Josh Matthews [:jdm] 2011-08-22 06:24:12 PDT
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.
Comment 3 Michael Wu [:mwu] 2011-08-22 17:28:17 PDT
(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 Josh Matthews [:jdm] 2011-08-22 17:40:39 PDT
In that case ignore comment 2. Let's just add fclose calls to every exit point in the function.
Comment 5 Atul Aggarwal 2011-08-28 08:21:07 PDT
Created attachment 556394 [details] [diff] [review]
v1 patch for close(fd)
Comment 6 Doug Turner (:dougt) 2011-08-28 09:13:25 PDT
mwu should review all changes to this file
Comment 7 Michael Wu [:mwu] 2011-08-30 11:44:16 PDT
Comment on attachment 556394 [details] [diff] [review]
v1 patch for close(fd)

Heh.
Comment 8 Ed Morley [:emorley] 2011-08-31 05:05:53 PDT
In my queue :-)
Comment 9 Ed Morley [:emorley] 2011-08-31 06:15:45 PDT
Quick sanity check:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=46750bfed1e1
...then onto inbound afterwards :-)
Comment 11 Ed Morley [:emorley] 2011-09-01 01:39:05 PDT
http://hg.mozilla.org/mozilla-central/rev/1cd727227440

Note You need to log in before you can comment on or make changes to this bug.