737463 - nsJARInputStream leaks if it is closed before the stream is inflated

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Chris Coulson]

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

Comment 1

[Chris Coulson]

Attachment: 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.

Comment 2

[(dormant account)]

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

Comment 3

[Michael Wu [: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 .

Comment 4

[Chris Coulson]

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.

Comment 5

[Chris Coulson]

Errrr, actually, I see now ;)

Comment 6

[Chris Coulson]

Attachment: Don't leak in nsJARInputStream if the stream is closed before being fully inflated (v2)

Ok, this one just adds an extra call to inflateEnd() in nsJARInputStream::Close() and removes the changes in nsJARInputStream::ContinueInflate()

Comment 7

[Michael Wu [: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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/226937c24338

Comment 9

[Richard Newman [:rnewman]]

Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321

Comment 10

[Richard Newman [:rnewman]]

Disregard that; PEBKAC. Did not get backed out. I misread TBPL.

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/226937c24338