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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Daniel Holbert [:dholbert]]

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.)

Comment 4

[Daniel Holbert [:dholbert]]

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.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 1140478-fix.patch

(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

Comment 6

[Jason Orendorff [: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...

Comment 7

[Daniel Holbert [:dholbert]]

(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.

Comment 8

[Daniel Holbert [:dholbert]]

Try run across a selection of android-flavored platforms:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46f316c6219

Comment 9

[Daniel Holbert [:dholbert]]

Try run looks fine; landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9a161f0645

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e9a161f0645