nsJARInputStream leaks if it is closed before the stream is inflated

RESOLVED FIXED in mozilla14

Status

()

Core
Networking: JAR
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Chris Coulson, Assigned: Chris Coulson)

Tracking

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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()
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

5 years ago
Attachment #607577 - Flags: review?(taras.mozilla)

Comment 2

5 years ago
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)

Comment 3

5 years ago
(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 .
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Errrr, actually, I see now ;)
(Assignee)

Comment 6

5 years ago
Created attachment 609167 [details] [diff] [review]
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()
Attachment #607577 - Attachment is obsolete: true
Attachment #607577 - Flags: review?(mwu)
Attachment #609167 - Flags: review?(mwu)

Comment 7

5 years ago
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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
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 → ---
Keywords: checkin-needed
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.