Closed
Bug 737463
Opened 12 years ago
Closed 12 years ago
nsJARInputStream leaks if it is closed before the stream is inflated
Categories
(Core :: Networking: JAR, defect)
Core
Networking: JAR
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()
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Updated•12 years ago
|
Attachment #607577 -
Flags: review?(taras.mozilla)
Comment 2•12 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•12 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•12 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•12 years ago
|
||
Errrr, actually, I see now ;)
Assignee | ||
Comment 6•12 years ago
|
||
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•12 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•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Assignee: nobody → chrisccoulson
Comment 9•12 years ago
|
||
Backed out whole merge for bustage per Yoric (Bug 743574): https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Target Milestone: mozilla14 → ---
Updated•12 years ago
|
Keywords: checkin-needed
Comment 10•12 years ago
|
||
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/226937c24338
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•