Closed Bug 1140478 Opened 7 years ago Closed 7 years ago

PowerManagerService.cpp leaks the buffer returned by PrintJSStack(); should free with JS_smprintf_free()

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Tracking Status
firefox39 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

PowerManagerService.cpp has calls to PrintJSStack(), like so:
> 25 #ifdef ANDROID
> 26 #include <android/log.h>
> 27 extern "C" char* PrintJSStack();
> 28 static void LogFunctionAndJSStack(const char* funcname) {
> 29   char *jsstack = PrintJSStack();
> 30   __android_log_print(ANDROID_LOG_INFO, "PowerManagerService", \
> 31                       "Call to %s. The JS stack is:\n%s\n",
> 32                       funcname,
> 33                       jsstack ? jsstack : "<no JS stack>");
> 34 }
http://mxr.mozilla.org/mozilla-central/source/dom/power/PowerManagerService.cpp?rev=469b786fd5f1#27
...which is called as part of the LOG_FUNCTION_AND_JS_STACK() macro.

PrintJSStack() returns a heap-allocated buffer, which we're leaking here -- we print it & return without freeing it.

According to [1], callers are supposed to free the PrintJSStack() return value via JS_smprintf_free().  (This documentation is actually for "xpc_PrintJSStack", but PrintJSStack is effectively a convenience wrapper for that function, via 2 layers of indirection.)

[1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/xpcprivate.h?rev=5f9adee38d45#2974
Side note: it looks like we only call LOG_FUNCTION_AND_JS_STACK() shortly before poweroff / reboot. So this leak isn't too bad, as leaks go.

Still, we should clean up after ourselves, if for no other reason than to document correct usage. This is the only caller of PrintJSStack() in the codebase, and it's unfortunate that this sole caller leaks the returned buffer.
Attached patch fix v1 (obsolete) — Splinter Review
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Note: the null-check before JS_smprintf_free is probably unnecessary, but I've got it there because:
 (1) I don't know if JS_smprintf_free is as null-tolerant as standard "free"
 (2) Including the null-check makes us consist with other calls to JS_smprintf_free, particularly with these checks in XPCThrower.cpp:
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCThrower.cpp?rev=160607e021a9#127
http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCThrower.cpp?rev=160607e021a9#148
(I also couldn't immediately find any checks where we *do* clearly call JS_smprintf_free on something that could be null.)
Attachment #8574037 - Flags: review?(nchen)
Comment on attachment 8574037 [details] [diff] [review]
fix v1

(Not sure who should review; cjones implemented PrintJSStack(), and bjacob added this invocation, and they're both mostly-gone at this point.

I initially tagged jchen, because he's touched this file recently, but in retrospect jorendorff is probably best-able to review this, since he reviewed the impl of PrintJSStack() and may even know whether my null-check is worth keeping or not.)
Attachment #8574037 - Flags: review?(nchen) → review?(jorendorff)
(In reply to Daniel Holbert [:dholbert] from comment #3)
> Note: the null-check before JS_smprintf_free is probably unnecessary, but
> I've got it there because: [...]

Actually - billm says I can assume that js_free handles nullptr in the same way that "free" does. So, since JS_smprintf_free is just a wrapper around js_free [1], that makes me confident that I don't need the null-check.

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jsprf.cpp?rev=63364da8765f&mark=960-960#954
Attachment #8574037 - Attachment is obsolete: true
Attachment #8574037 - Flags: review?(jorendorff)
Attachment #8574163 - Flags: review?(jorendorff)
Comment on attachment 8574163 [details] [diff] [review]
1140478-fix.patch

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

OK. Or you could change the API so it's not possible to make this mistake. We've got all these RAII helpers, might as well use them...
Attachment #8574163 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> OK.

Thanks!

> Or you could change the API so it's not possible to make this mistake.
> We've got all these RAII helpers, might as well use them...

I'm generally all for RAII goodness, but I'd rather not mess with the API right now. Just filing this bug, I'm already a little ways into a rathole (after researching PrintJSStack while reviewing a patch in bug 1129249, and discovering this apparently-broken usage here) -- and I probably shouldn't go too much deeper.

If someone else wants to come along later and make the API RAII-friendly (e.g. returning a UniquePtr<char> or something similar), then I'm all in support of that.
Try run across a selection of android-flavored platforms:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46f316c6219
Try run looks fine; landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9a161f0645
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/3e9a161f0645
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.