Last Comment Bug 678997 - memory leak in widget/src/android/AndroidBridge.cpp
: memory leak in widget/src/android/AndroidBridge.cpp
Status: RESOLVED FIXED
[MemShrink][good first bug][mentor=jdm]
: mlk, mobile
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Doug Turner (:dougt)
:
:
Mentors:
Depends on:
Blocks: cppcheck
  Show dependency treegraph
 
Reported: 2011-08-15 08:35 PDT by David Volgyes
Modified: 2011-09-01 15:00 PDT (History)
7 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
AndroidBridge.diff (1013 bytes, patch)
2011-08-15 08:35 PDT, David Volgyes
no flags Details | Diff | Splinter Review
patch v.2 (1.19 KB, patch)
2011-08-31 09:35 PDT, Doug Turner (:dougt)
doug.turner: review+
Details | Diff | Splinter Review

Description David Volgyes 2011-08-15 08:35:07 PDT
Created attachment 553182 [details] [diff] [review]
AndroidBridge.diff

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

cppcheck 1.49 (http://cppcheck.sourceforge.net/) found a plenty of potential null pointer dereference. This is one of them.


Actual results:

cppcheck tells everything:
widget/src/android/AndroidBridge.cpp:87]: (error) Allocation with strdup, putenv doesn't release it.

The corresponding line is that:
    putenv(strdup("NSS_DISABLE_UNLOAD=1"));





Expected results:

I see that strdup should solve the "constness" problem.
However, you should release the memory after putenv.
char*tmp=strdup("NSS_DISABLE_UNLOAD=1");
putenv(tmp);
free(tmp);

This possible solution is attached.
Comment 1 Brad Lassey [:blassey] (use needinfo?) 2011-08-17 23:59:01 PDT
Comment on attachment 553182 [details] [diff] [review]
AndroidBridge.diff

Review of attachment 553182 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/android/AndroidBridge.cpp
@@ +85,5 @@
>       * Conveniently, NSS has an env var that can prevent it from unloading.
>       */
> +    char* tmp=strdup("NSS_DISABLE_UNLOAD=1");
> +    putenv(tmp);
> +    free(tmp);

how about this?
static char* nss_disable_unload = "NSS_DISABLE_UNLOAD=1";
putenv(nss_disable_unload);
Comment 2 David Volgyes 2011-08-18 00:12:06 PDT
Nice. Faster and safer.
Comment 3 Josh Matthews [:jdm] 2011-08-31 06:01:28 PDT
It would be nice for someone to pick this up. All that needs to be done is modify the patch to follow Brad's suggestion in comment 1.
Comment 4 Ed Morley [:emorley] 2011-08-31 06:10:42 PDT
Unassigning from David per his request (bug 679610 comment 4).

For anyone looking at this bug, feel free to take it, David has very kindly provided a patch, but will not have time to follow it through.
Comment 5 Doug Turner (:dougt) 2011-08-31 09:35:38 PDT
Created attachment 557213 [details] [diff] [review]
patch v.2
Comment 6 Alon Zakai (:azakai) 2011-08-31 09:56:22 PDT
Wouldn't just removing the strdup work here?
Comment 7 Doug Turner (:dougt) 2011-08-31 09:59:40 PDT
that work work too.  r+ w/ that change?
Comment 8 Alon Zakai (:azakai) 2011-08-31 10:06:16 PDT
Yes.
Comment 10 Ed Morley [:emorley] 2011-09-01 01:58:39 PDT
http://hg.mozilla.org/mozilla-central/rev/f872cb091586

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