Closed Bug 801957 Opened 12 years ago Closed 12 years ago

Heap-use-after-free in XPCNativeSet::Mark

Categories

(Core :: JavaScript Engine, defect)

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + fixed
firefox19 + verified
firefox-esr10 --- unaffected

People

(Reporter: inferno, Assigned: mccr8)

References

Details

(5 keywords, Whiteboard: [asan][adv-track-main17+])

Attachments

(2 files)

Attached file Testcase (deleted) —
Reproducing on trunk easily if you launch multiple firefox instances (like ~10-15 with the testcaseon cmd line). ================================================================= ==28870== ERROR: AddressSanitizer heap-use-after-free on address 0x7fe1fa737d82 at pc 0x7fe2284641fd bp 0x7fff82ee0f30 sp 0x7fff82ee0f28 READ of size 2 at 0x7fe1fa737d82 thread T0 #0 0x7fe2284641fc in XPCNativeSet::IsMarked() const js/xpconnect/src/xpcprivate.h:2100 #1 0x7fe2284639e2 in XPCNativeSet::Mark() js/xpconnect/src/XPCInlines.h:502 #2 0x7fe2287e5e0b in XPCWrappedNative::TraceInside(JSTracer*) js/xpconnect/src/xpcprivate.h:2877 #3 0x7fe22884c5c6 in MarkWrappedNative(JSTracer*, JSObject*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:676 #4 0x7fe2288163ff in XPC_WN_NoHelper_Trace(JSTracer*, JSObject*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:685 #5 0x7fe2360491da in js::GCMarker::processMarkStackTop(js::SliceBudget&) js/src/gc/Marking.cpp:1255 #6 0x7fe236044320 in js::GCMarker::drainMarkStack(js::SliceBudget&) js/src/gc/Marking.cpp:1299 #7 0x7fe23478cf54 in DrainMarkStack(JSRuntime*, js::SliceBudget&) js/src/jsgc.cpp:4266 #8 0x7fe23478aab2 in IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) js/src/jsgc.cpp:4328 #9 0x7fe23478857a in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) js/src/jsgc.cpp:4533 #10 0x7fe23473d044 in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) js/src/jsgc.cpp:4647 #11 0x7fe23472caf6 in js::GCSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason, long) js/src/jsgc.cpp:4687 #12 0x7fe23467eb9b in js::IncrementalGC(JSRuntime*, js::gcreason::Reason, long) js/src/jsfriendapi.cpp:172 #13 0x7fe225788783 in nsJSContext::GarbageCollectNow(js::gcreason::Reason, nsJSContext::IsIncremental, nsJSContext::IsCompartment, nsJSContext::IsShrinking, long) dom/base/nsJSEnvironment.cpp:2955 #14 0x7fe2257d2599 in GCTimerFired(nsITimer*, void*) dom/base/nsJSEnvironment.cpp:3215 #15 0x7fe22d582322 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:472 #16 0x7fe22d58382a in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:555 #17 0x7fe22d546f30 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612 #18 0x7fe22d1d7e5b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220 #19 0x7fe22bb7de4d in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:117 #20 0x7fe22d820e41 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:215 #21 0x7fe22d820c76 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:208 #22 0x7fe22d820b5b in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:182 #23 0x7fe22b01ea6a in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163 #24 0x7fe229c4d774 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290 #25 0x7fe21fbf5acd in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3792 #26 0x7fe21fbfb945 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3858 #27 0x7fe21fbfe7f4 in XRE_main toolkit/xre/nsAppRunner.cpp:3933 #28 0x40bb13 in do_main(int, char**) browser/app/nsBrowserApp.cpp:174 #29 0x409255 in main browser/app/nsBrowserApp.cpp:279 #30 0x7fe23f4a576c in ?? ??:0 0x7fe1fa737d82 is located 2 bytes inside of 72-byte region [0x7fe1fa737d80,0x7fe1fa737dc8) freed by thread T0 here: #0 0x4c3660 in __interceptor_free ??:? #1 0x7fe23c31d406 in moz_free memory/mozalloc/mozalloc.cpp:51 #2 0x7fe228800ebc in operator delete[](void*) ../../../dist/include/mozilla/mozalloc.h:236 #3 0x7fe22867fae5 in NativeSetSweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*) js/xpconnect/src/XPCJSRuntime.cpp:144 #4 0x7fe2345f748e in JS_DHashTableEnumerate js/src/jsdhash.cpp:708 #5 0x7fe22867f610 in NativeSetMap::Enumerate(JSDHashOperator (*)(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*), void*) js/xpconnect/src/XPCMaps.h:447 #6 0x7fe22867c7f0 in XPCJSRuntime::FinalizeCallback(JSFreeOp*, JSFinalizeStatus, int) js/xpconnect/src/XPCJSRuntime.cpp:882 #7 0x7fe23479085e in BeginSweepPhase(JSRuntime*) js/src/jsgc.cpp:3907 #8 0x7fe23478aec4 in IncrementalCollectSlice(JSRuntime*, long, js::gcreason::Reason, js::JSGCInvocationKind) js/src/jsgc.cpp:4355 #9 0x7fe23478857a in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) js/src/jsgc.cpp:4533 #10 0x7fe23473d044 in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) js/src/jsgc.cpp:4647 #11 0x7fe23473d912 in js::GCFinalSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason) js/src/jsgc.cpp:4694 #12 0x7fe23467edb0 in js::FinishIncrementalGC(JSRuntime*, js::gcreason::Reason) js/src/jsfriendapi.cpp:178 #13 0x7fe2340a0f75 in JS_TransplantObject js/src/jsapi.cpp:1560 #14 0x7fe2295feb55 in xpc::TransplantObject(JSContext*, JSObject*, JSObject*) js/xpconnect/wrappers/WrapperFactory.cpp:632 #15 0x7fe22588b9b6 in nsGlobalWindow::SetNewDocument(nsIDocument*, nsISupports*, bool) dom/base/nsGlobalWindow.cpp:2025 #16 0x7fe221b4251e in DocumentViewerImpl::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool) layout/base/nsDocumentViewer.cpp:927 #17 0x7fe221b3fd88 in DocumentViewerImpl::Init(nsIWidget*, nsIntRect const&) layout/base/nsDocumentViewer.cpp:677 #18 0x7fe2296e01bd in nsDocShell::SetupNewViewer(nsIContentViewer*) docshell/base/nsDocShell.cpp:8084 #19 0x7fe2296ab99d in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) docshell/base/nsDocShell.cpp:6139 #20 0x7fe2296d652e in nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) docshell/base/nsDocShell.cpp:7870 #21 0x7fe22976dc29 in nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) docshell/base/nsDSURIContentListener.cpp:122 #22 0x7fe22979c7ef in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) uriloader/base/nsURILoader.cpp:654 #23 0x7fe229798114 in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) uriloader/base/nsURILoader.cpp:356 #24 0x7fe229796a4c in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) uriloader/base/nsURILoader.cpp:248 #25 0x7fe21fd2f882 in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) netwerk/base/src/nsBaseChannel.cpp:740 #26 0x7fe21fd31386 in non-virtual thunk to nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) :? #27 0x7fe21fdcab24 in nsInputStreamPump::OnStateStart() netwerk/base/src/nsInputStreamPump.cpp:417 #28 0x7fe21fdc9d21 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/src/nsInputStreamPump.cpp:368 #29 0x7fe21fdcd13e in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) :? previously allocated by thread T0 here: #0 0x4c3720 in malloc ??:? #1 0x7fe23c31d55a in moz_xmalloc memory/mozalloc/mozalloc.cpp:57 #2 0x7fe228806804 in operator new[](unsigned long) ../../../dist/include/mozilla/mozalloc.h:212 #3 0x7fe228805a5d in XPCNativeSet::GetNewOrUsed(XPCCallContext&, XPCNativeSet*, XPCNativeInterface*, unsigned short) js/xpconnect/src/XPCWrappedNativeInfo.cpp:610 #4 0x7fe2287bb687 in XPCWrappedNative::ExtendSet(XPCCallContext&, XPCNativeInterface*) js/xpconnect/src/XPCWrappedNative.cpp:1906 #5 0x7fe2287bee8d in XPCWrappedNative::InitTearOff(XPCCallContext&, XPCWrappedNativeTearOff*, XPCNativeInterface*, int) js/xpconnect/src/XPCWrappedNative.cpp:2176 #6 0x7fe22879b1db in XPCWrappedNative::FindTearOff(XPCCallContext&, XPCNativeInterface*, int, tag_nsresult*) js/xpconnect/src/XPCWrappedNative.cpp:2000 #7 0x7fe2285cf0a1 in XPCConvert::NativeInterface2JSObject(XPCLazyCallContext&, JS::Value*, nsIXPConnectJSObjectHolder**, xpcObjectHelper&, nsID const*, XPCNativeInterface**, bool, tag_nsresult*) js/xpconnect/src/XPCConvert.cpp:939 #8 0x7fe22888f4c7 in xpc_qsXPCOMObjectToJsval(XPCLazyCallContext&, qsObjectHelper&, nsID const*, XPCNativeInterface**, JS::Value*) js/xpconnect/src/XPCQuickStubs.cpp:993 #9 0x7fe22892a6ed in nsIDOMDocument_AdoptNode(JSContext*, unsigned int, JS::Value*) objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:3465 #10 0x7fe234a24cdf in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:364 #11 0x7fe2349d4995 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) js/src/jsinterp.cpp:2370 #12 0x7fe234933b3e in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) js/src/jsinterp.cpp:324 #13 0x7fe234a3244d in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) js/src/jsinterp.cpp:509 #14 0x7fe234a340db in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:546 #15 0x7fe23415517d in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5685 #16 0x7fe2257a2ece in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) dom/base/nsJSEnvironment.cpp:1507 #17 0x7fe22595e076 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) dom/base/nsGlobalWindow.cpp:9701 #18 0x7fe225914574 in nsGlobalWindow::RunTimeout(nsTimeout*) dom/base/nsGlobalWindow.cpp:9960 #19 0x7fe22595bf28 in nsGlobalWindow::TimerCallback(nsITimer*, void*) dom/base/nsGlobalWindow.cpp:10227 #20 0x7fe22d582322 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:472 #21 0x7fe22d58382a in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:555 #22 0x7fe22d546f30 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612 #23 0x7fe22d1d7e5b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220 #24 0x7fe22bb7d7e6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82 Shadow byte and word: 0x1ffc3f4e6fb0: fd 0x1ffc3f4e6fb0: fd fd fd fd fd fd fd fd More shadow bytes: 0x1ffc3f4e6f90: fd fd fd fd fd fd fd fd 0x1ffc3f4e6f98: fd fd fd fd fd fd fd fd 0x1ffc3f4e6fa0: fa fa fa fa fa fa fa fa 0x1ffc3f4e6fa8: fa fa fa fa fa fa fa fa =>0x1ffc3f4e6fb0: fd fd fd fd fd fd fd fd 0x1ffc3f4e6fb8: fd fd fd fd fd fd fd fd 0x1ffc3f4e6fc0: fa fa fa fa fa fa fa fa 0x1ffc3f4e6fc8: fa fa fa fa fa fa fa fa 0x1ffc3f4e6fd0: fd fd fd fd fd fd fd fd Stats: 265M malloced (287M for red zones) by 422691 calls Stats: 44M realloced by 25100 calls Stats: 237M freed by 293749 calls Stats: 103M really freed by 204660 calls Stats: 472M (120919 full pages) mmaped in 118 calls mmaps by size class: 8:294894; 9:32764; 10:8190; 11:14329; 12:2048; 13:1536; 14:1280; 15:256; 16:1088; 17:1280; 18:112; 19:40; 20:24; mallocs by size class: 8:349661; 9:35520; 10:9968; 11:17885; 12:2769; 13:1971; 14:1656; 15:342; 16:1279; 17:1395; 18:183; 19:40; 20:22; frees by size class: 8:237898; 9:26534; 10:6492; 11:14686; 12:1864; 13:1717; 14:1482; 15:288; 16:1195; 17:1377; 18:159; 19:38; 20:19; rfrees by size class: 8:178622; 9:9642; 10:2826; 11:9649; 12:881; 13:864; 14:578; 15:156; 16:969; 17:387; 18:81; 19:4; 20:1; Stats: malloc large: 1640 small slow: 2367 ==28870== ABORTING
Severity: normal → critical
Component: General → XPConnect
Keywords: crash, testcase
Product: Firefox → Core
Whiteboard: [asan]
Fwiw, the stack is somewhat similar to bug 606116. The trigger in the test appears to be: document.implementation.createDocument("", "", null).adoptNode(tCF0); where tCF0 is an <audio> element.
Assignee: nobody → continuation
Keywords: sec-critical
From the stacks and digging around in the code and history, it looks like bug 729760 may have regressed bug 758471. The problem in bug 758471 that billm fixed was that XPC can only rely on the marking for NativeSets if there was a full GC: if the GC didn't mark a compartment, it wouldn't mark its NativeSets, or something like that. One little wrinkle there was that we can be doing a full GC, but a new compartment can be created during the GC, which we don't mark, so we behave like a compartmental GC. It looks like in bug 729760, the piece of code for one of the callbacks was moved, and somehow in the process the argument that said if we're doing a full GC got turned into the one on the rt, not the computed one.
Blocks: 729760
Attached patch use the right isFull (deleted) — Splinter Review
I'll try to figure out how to make a test case for this.
Comment on attachment 672507 [details] [diff] [review] use the right isFull I guess we should fix this even if it doesn't fix the test case. (I haven't reproduced the test case locally yet.)
Attachment #672507 - Flags: review?(wmccloskey)
Attachment #672507 - Flags: feedback?(jcoppeard)
Attachment #672507 - Flags: review?(wmccloskey) → review+
I haven't managed to make a minimal test case yet to trigger this, but with the original test case you can eventually get |isFull != rt->gcIsFull| when you reload it over and over again, so that seems like further evidence it is the same problem.
I also get this assertion when I reload the test case a bunch of times, even with my patch: ###!!! ASSERTION: About to remove a different wrapper with the same nsISupports identity! This will most likely cause serious problems!: '!wrapperInMap || wrapperInMap == wrapper', file /Users/amccreight/mz/cent4/js/xpconnect/src/XPCMaps.h, line 129 I don't know how "serious" serious actually is. That sounds vaguely similar to some other problems we've seen.
Component: XPConnect → JavaScript Engine
Comment on attachment 672507 [details] [diff] [review] use the right isFull [Security approval request comment] How easily can the security issue be deduced from the patch? You could probably tell from digging around that this argument was introduced in bug 758471 (which is public now), and then figure out that something regressed that bug. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no Which older supported branches are affected by this flaw? 17/18/19 If not all supported branches, which bug introduced the flaw? 729760 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch is trivial, I don't think there will be problems backporting. How likely is this patch to cause regressions; how much testing does it need? The patch is very simple, but on the other hand, it is in a very critical portion of code. Though I guess it did take two cycles for somebody to find the problem.
Attachment #672507 - Flags: sec-approval?
Comment on attachment 672507 [details] [diff] [review] use the right isFull sec-approval+
Attachment #672507 - Flags: sec-approval? → sec-approval+
Keywords: csec-uaf
Comment on attachment 672507 [details] [diff] [review] use the right isFull Review of attachment 672507 [details] [diff] [review]: ----------------------------------------------------------------- Yes this looks right. There's was reason to use the pre-computed version of isFull rather than the up-to-date one.
Attachment #672507 - Flags: feedback?(jcoppeard) → feedback+
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Branch approval nominations would be awesome now, let's try to get this into 17.0 beta 3 so we have more time to collect new crash data.
Comment on attachment 672507 [details] [diff] [review] use the right isFull [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 729760 User impact if declined: possible security problems Testing completed (on m-c, etc.): landed on m-c yesterday Risk to taking this patch (and alternatives if risky): should be fairly, these two values are almost the same, and nobody noticed that it was actually wrong for a long time String or UUID changes made by this patch: none
Attachment #672507 - Flags: approval-mozilla-beta?
Attachment #672507 - Flags: approval-mozilla-aurora?
Attachment #672507 - Flags: approval-mozilla-beta?
Attachment #672507 - Flags: approval-mozilla-beta+
Attachment #672507 - Flags: approval-mozilla-aurora?
Attachment #672507 - Flags: approval-mozilla-aurora+
Abhishek, can you still reproduce this crash? I think I fixed it, but it would be nice to be sure. Thanks.
Attachment #673517 - Attachment description: Bug Bounty Nomination → Bounty Awarded $3000
(In reply to Andrew McCreight [:mccr8] from comment #16) > Abhishek, can you still reproduce this crash? I think I fixed it, but it > would be nice to be sure. Thanks. Tested on my trunk build which had the fix and crash does not happen. Just to be safe, i reversed applied your patch to make sure that crash happened. So, indeed your fix worked.
Great, thanks for the confirmation.
Whiteboard: [asan] → [asan][adv-track-main17+]
Attachment #673517 - Attachment description: Bounty Awarded $3000 → Bounty Awarded $3000 [paid]
Group: core-security
Flags: sec-bounty+
I guess we can call this verified fixed based on reporter's comment 17.
(In reply to Paul Silaghi [QA] from comment #19) > I guess we can call this verified fixed based on reporter's comment 17. Verified the issue on 2012-10-15 Nightly Debug using the testcase from comment 0 but I could not reproduce the same error as described in the comment. Instead I found the assertion written in comment 6. No assertion found for 2013-02-06 Debug build. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:19.0) Gecko/20130204 Firefox/19.0 Build ID: 20130204160822
(In reply to Andrew McCreight [:mccr8] from comment #3) > Created attachment 672507 [details] [diff] [review] > use the right isFull > > I'll try to figure out how to make a test case for this. Any success with this? Even if it only reproduces under ASan, we can commit a test if it's working reliably. If not, then we should mark this in-testsuite-.
Flags: needinfo?(continuation)
Jon Coppeard added a test for this when it was regressed again later. :)
Flags: needinfo?(continuation)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: