Closed Bug 737463 Opened 8 years ago Closed 8 years ago

nsJARInputStream leaks if it is closed before the stream is inflated

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: chrisccoulson, Assigned: chrisccoulson)

Details

Attachments

(1 file, 1 obsolete file)

When running Firefox in valgrind, I see this (I only ran it for a minute or so here):

==32263== 150,192 bytes in 21 blocks are definitely lost in loss record 8,480 of 8,480
==32263==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==32263==    by 0x953A7C4: MOZ_Z_inflateInit2_ (inflate.c:187)
==32263==    by 0x8154A25: gZlibInit(z_stream_s*) (nsZipArchive.cpp:122)
==32263==    by 0x81569ED: nsJARInputStream::InitFile(nsJAR*, nsZipItem*) (nsJARInputStream.cpp:80)
==32263==    by 0x815862A: nsJAR::GetInputStreamWithSpec(nsACString_internal const&, nsACString_internal const&, nsIInputStream**) (nsJAR.cpp:375)
==32263==    by 0x8EA1863: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
==32263==    by 0x89C2B93: CallMethodHelper::Call() (XPCWrappedNative.cpp:3021)
==32263==    by 0x89C3157: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2318)
==32263==    by 0x89C8CFB: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1539)
==32263==    by 0x91EE7AC: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), js::CallArgs const&) (jscntxtinlines.h:314)
==32263==    by 0x9201B70: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:513)
==32263==    by 0x91FBB5A: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2684)
==32263==    by 0x920178D: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:469)
==32263==    by 0x9202AFB: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:667)
==32263==    by 0x9233B70: EvalKernel(JSContext*, js::CallArgs const&, EvalType, js::StackFrame*, JSObject&) (jsobj.cpp:1052)
==32263==    by 0x92341D7: js::DirectEval(JSContext*, js::CallArgs const&) (jsobj.cpp:1117)
==32263==    by 0x91FC5DB: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2643)
==32263==    by 0x920178D: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:469)
==32263==    by 0x9202F22: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:667)
==32263==    by 0x90F9471: EvaluateUCScriptForPrincipalsCommon(JSContext*, JSObject*, JSPrincipals*, JSPrincipals*, unsigned short const*, unsigned int, char const*, unsigned int, JS::Value*, JSVersion) (jsapi.cpp:5266)
==32263==    by 0x90F9588: JS_EvaluateUCScriptForPrincipalsVersionOrigin (jsapi.cpp:5303)
==32263==    by 0x866DA99: nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, unsigned int, nsAString_internal*, bool*) (nsJSEnvironment.cpp:1448)
==32263==    by 0x848E408: nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&) (nsScriptLoader.cpp:923)
==32263==    by 0x848E87C: nsScriptLoader::ProcessRequest(nsScriptLoadRequest*) (nsScriptLoader.cpp:816)
==32263==    by 0x849148E: nsScriptLoader::ProcessPendingRequests() (nsScriptLoader.cpp:983)
==32263==    by 0x8491835: nsScriptLoader::OnStreamComplete(nsIStreamLoader*, nsISupports*, unsigned int, unsigned int, unsigned char const*) (nsScriptLoader.cpp:1202)
==32263==    by 0x8074DD9: nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsStreamLoader.cpp:127)
==32263==    by 0x8046731: nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (nsBaseChannel.cpp:745)
==32263==    by 0x805228F: nsInputStreamPump::OnStateStop() (nsInputStreamPump.cpp:583)
==32263==    by 0x8052361: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:405)
==32263==    by 0x8E76CE6: nsInputStreamReadyEvent::Run() (nsStreamUtils.cpp:114)
==32263==    by 0x8E8B08F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:657)
==32263==    by 0x8E49CD3: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
==32263==    by 0x8D9122E: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:134)
==32263==    by 0x8EB6913: MessageLoop::RunInternal() (message_loop.cc:208)
==32263==    by 0x8EB693B: MessageLoop::Run() (message_loop.cc:201)
==32263==    by 0x8CB2F3A: nsBaseAppShell::Run() (nsBaseAppShell.cpp:189)
==32263==    by 0x8AF72EF: nsAppStartup::Run() (nsAppStartup.cpp:295)
==32263==    by 0x802726D: XRE_main (nsAppRunner.cpp:3703)
==32263==    by 0x4014A7: main (nsBrowserApp.cpp:190)

There's several more of these types of leak in the valgrind log too.

It seems to be trivially reproducible by installing the extension developer addon and doing this in the JS console:

file=Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile)
file.initWithPath("/usr/lib/firefox-trunk/omni.ja")
reader=Components.classes["@mozilla.org/libjar/zip-reader;1"].createInstance(Components.interfaces.nsIZipReader)
reader.open(file)
reader.getInputStream("components/browser.xpt").close()
This seems to fix it. Obviously with this patch it is no longer "aggressive about ending the inflation" (as per the comment in nsJARInputStream::ContinueInflate). If that matters, then I can take a different approach to it instead.
OS: Linux → All
Hardware: x86_64 → All
Attachment #607577 - Flags: review?(taras.mozilla)
Comment on attachment 607577 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated

cant review this until april. maybe mwu can
Attachment #607577 - Flags: review?(taras.mozilla) → review?(mwu)
(In reply to Chris Coulson from comment #1)
> Created attachment 607577 [details] [diff] [review]
> Don't leak in nsJARInputStream if the stream is closed before being fully
> inflated
> 
> This seems to fix it. Obviously with this patch it is no longer "aggressive
> about ending the inflation" (as per the comment in
> nsJARInputStream::ContinueInflate). If that matters, then I can take a
> different approach to it instead.

I don't think calling inflateEnd twice really matters, at least according to https://hg.mozilla.org/mozilla-central/file/3e4735893504/modules/zlib/src/inflate.c#l1238 .
Hi Michael,

Perhaps I'm missing something else, but would this not end up being a double-free?

https://hg.mozilla.org/mozilla-central/file/3e4735893504/modules/zlib/src/inflate.c#l1245

AFAICT, state->window is left dangling, which is why I thought it would be unsafe to call a second time.
Errrr, actually, I see now ;)
Ok, this one just adds an extra call to inflateEnd() in nsJARInputStream::Close() and removes the changes in nsJARInputStream::ContinueInflate()
Attachment #607577 - Attachment is obsolete: true
Attachment #607577 - Flags: review?(mwu)
Attachment #609167 - Flags: review?(mwu)
Comment on attachment 609167 [details] [diff] [review]
Don't leak in nsJARInputStream if the stream is closed before being fully inflated (v2)

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

Wow, I must've missed this review request during the craziness the last few weeks. Sorry about the delay.
Attachment #609167 - Flags: review?(mwu) → review+
Keywords: checkin-needed
Assignee: nobody → chrisccoulson
https://hg.mozilla.org/integration/mozilla-inbound/rev/226937c24338
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Target Milestone: mozilla14 → ---
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/226937c24338
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.