Closed
Bug 1319456
Opened 8 years ago
Closed 8 years ago
[e10s] Crash in std::_Hash<T>::equal_range
Categories
(Core :: Printing: Output, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: philipp, Assigned: bobowen)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main51+])
Crash Data
Attachments
(1 file)
(deleted),
patch
|
bas.schouten
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is
report bp-8a500d05-e87e-4aad-a2d2-0c0592161122.
=============================================================
Crashing Thread (0)
Frame Module Signature Source
0 xul.dll std::_Hash<std::_Uset_traits<mozilla::gfx::DrawTargetD2D1*, std::_Uhash_compare<mozilla::gfx::DrawTargetD2D1*, std::hash<mozilla::gfx::DrawTargetD2D1*>, std::equal_to<mozilla::gfx::DrawTargetD2D1*> >, std::allocator<mozilla::gfx::DrawTargetD2D1*>, 0> >::equal_range(mozilla::gfx::DrawTargetD2D1* const&) vs2015u2/VC/include/xhash:641
1 xul.dll mozilla::gfx::DrawEventRecorderPrivate::RemoveStoredObject(mozilla::gfx::ReferencePtr) gfx/2d/DrawEventRecorder.h:42
2 xul.dll nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::ShiftData<nsTArrayInfallibleAllocator>(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int) obj-firefox/dist/include/nsTArray-inl.h:261
3 xul.dll mozilla::gfx::PathRecording::`scalar deleting destructor'(unsigned int)
4 xul.dll nsTArray_Impl<mozilla::dom::CanvasRenderingContext2D::ContextState, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) obj-firefox/dist/include/nsTArray.h:1905
5 xul.dll mozilla::dom::CanvasRenderingContext2D::`scalar deleting destructor'(unsigned int)
6 xul.dll SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2685
7 xul.dll nsCycleCollector::FreeSnowWhite(bool) xpcom/base/nsCycleCollector.cpp:2859
8 xul.dll nsCycleCollector_doDeferredDeletion() xpcom/base/nsCycleCollector.cpp:4134
9 xul.dll AsyncFreeSnowWhite::Run() js/xpconnect/src/XPCJSRuntime.cpp:142
10 xul.dll nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1076
11 xul.dll NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:290
12 xul.dll nsPrintingProxy::ShowPrintDialog(mozIDOMWindowProxy*, nsIWebBrowserPrint*, nsIPrintSettings*) embedding/components/printingui/ipc/nsPrintingProxy.cpp:113
13 xul.dll nsPrintEngine::DoCommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) layout/printing/nsPrintEngine.cpp:616
14 xul.dll nsPrintEngine::CommonPrint(bool, nsIPrintSettings*, nsIWebProgressListener*, nsIDOMDocument*) layout/printing/nsPrintEngine.cpp:405
15 xul.dll nsPrintEngine::Print(nsIPrintSettings*, nsIWebProgressListener*) layout/printing/nsPrintEngine.cpp:788
16 xul.dll nsDocumentViewer::Print(nsIPrintSettings*, nsIWebProgressListener*) layout/base/nsDocumentViewer.cpp:3727
17 xul.dll nsGlobalWindow::PrintOuter(mozilla::ErrorResult&) dom/base/nsGlobalWindow.cpp:7388
18 xul.dll nsGlobalWindow::Print(mozilla::ErrorResult&) dom/base/nsGlobalWindow.cpp:7414
19 xul.dll mozilla::dom::WindowBinding::print obj-firefox/dom/bindings/WindowBinding.cpp:2527
20 xul.dll mozilla::dom::WindowBinding::genericMethod obj-firefox/dom/bindings/WindowBinding.cpp:13778
21 xul.dll js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:453
22 @0xfffffff5
23 @0xfffffff5
24 @0xfffffff5
25 kernel32.dll InterlockedExchangeAdd
26 xul.dll mozilla::detail::ThreadLocal<mozilla::dom::ScriptSettingsStackEntry*>::set(mozilla::dom::ScriptSettingsStackEntry* const) obj-firefox/dist/include/mozilla/ThreadLocal.h:203
27 xul.dll nsWrapperCache::GetWrapper() dom/base/nsWrapperCacheInlines.h:19
28 xul.dll mozilla::dom::ToJSValue<mozilla::dom::EventTarget>(JSContext*, mozilla::dom::EventTarget&, JS::MutableHandle<JS::Value>) obj-firefox/dist/include/mozilla/dom/ToJSValue.h:154
29 xul.dll mozilla::dom::Event::GetEventPopupControlState(mozilla::WidgetEvent*, nsIDOMEvent*) dom/events/Event.cpp:712
30 @0x2
31 xul.dll mozilla::ipc::MessageChannel::OnMaybeDequeueOne() ipc/glue/MessageChannel.cpp:1566
32 winmm.dll timeGetTime
33 xul.dll mozilla::detail::RunnableMethodImpl<bool ( mozilla::ipc::MessageChannel::*)(void), 0, 1>::Run() obj-firefox/dist/include/nsThreadUtils.h:764
34 xul.dll mozilla::ipc::MessageChannel::DequeueTask::Run() obj-firefox/dist/include/mozilla/ipc/MessageChannel.h:569
35 xul.dll nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1076
crashes in the content process with this signature are spiking since firefox 50 builds on windows. currently they account for around 1.2% of all content process crashes in firefox 50.0.
Correlations for Firefox Release:
(100.0% in signature vs 36.07% overall) dom_ipc_enabled = 1
(100.0% in signature vs 37.04% overall) reason = EXCEPTION_ACCESS_VIOLATION_READ
(94.97% in signature vs 42.29% overall) "D2D1.1-" in app_notes = true
Updated•8 years ago
|
Component: Untriaged → Printing: Output
Priority: -- → P3
Comment 1•8 years ago
|
||
Bob, the crash seemed to be related to printing recording canvas, do you have any insight about the root cause or pointer to progress this bug ?
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 2•8 years ago
|
||
MozReview-Commit-ID: 1bJxJnKXzVc
Attachment #8813557 -
Flags: review?(bas)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Comment 3•8 years ago
|
||
Thanks for taking prompt feedback and support in this bug.
Flags: needinfo?(bobowencode)
Priority: P3 → P1
Assignee | ||
Updated•8 years ago
|
Attachment #8813557 -
Attachment is private: true
Assignee | ||
Updated•8 years ago
|
Group: core-security
Assignee | ||
Updated•8 years ago
|
Crash Signature: [@ std::_Hash<T>::equal_range]
Updated•8 years ago
|
Attachment #8813557 -
Attachment is private: false
Updated•8 years ago
|
Group: core-security → gfx-core-security
Crash Signature: [@ std::_Hash<T>::equal_range ]
Comment 4•8 years ago
|
||
Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording
Review of attachment 8813557 [details] [diff] [review]:
-----------------------------------------------------------------
Will this not cause a reference cycle?
Attachment #8813557 -
Flags: review?(bas) → review+
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Bas Schouten (:bas.schouten) from comment #4)
> Comment on attachment 8813557 [details] [diff] [review]
> Hold DrawEventRecorders in the correct structure in PathRecording
>
> Review of attachment 8813557 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Will this not cause a reference cycle?
I don't think so.
The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id for objects during playback. It relies on the Creation and Destruction events to keep track of them.
It doesn't hold a reference to anything else as far as I can see.
Also, the other *Recording types hold references to the recorder, so we'd presumably see cycles there if there was a problem.
Does this reasoning sound correct?
Flags: needinfo?(bas)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)
> I don't think so.
> The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id
> for objects during playback. It relies on the Creation and Destruction
> events to keep track of them.
Erm, this should have said "as a unique ID to know what objects it has already recorded.".
Comment 7•8 years ago
|
||
Given that this affects multiple branches, it'll need a sec rating of moderate or less or sec-approval before it can land.
tracking-firefox50:
--- → ?
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The issue is pretty obvious from this patch, because it changes from holding raw pointers to RefPtrs.
I'm not sure how easy this would be to exploit given that the potential use after free happens later on during GC or CC.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Patch is a one line change and the check-in comment if fairly vague.
Which older supported branches are affected by this flaw?
I believe that the code landing in 18, but it wasn't actually used in release until Fx50.
If not all supported branches, which bug introduced the flaw?
The code landed as part of bug 792207.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch applies cleanly to release (50).
How likely is this patch to cause regressions; how much testing does it need?
I think the only possible issue would be reference cycles, but I don't think that could happen. Waiting on Bas's opinion.
Attachment #8813557 -
Flags: sec-approval?
Comment 10•8 years ago
|
||
Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording
sec-approval+ for trunk. We should backport to Aurora and Beta, at least, as well so please nominate patches for those branches.
Attachment #8813557 -
Flags: sec-approval? → sec-approval+
Comment 11•8 years ago
|
||
(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Bas Schouten (:bas.schouten) from comment #4)
> > Comment on attachment 8813557 [details] [diff] [review]
> > Hold DrawEventRecorders in the correct structure in PathRecording
> >
> > Review of attachment 8813557 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > Will this not cause a reference cycle?
>
> I don't think so.
> The DrawEventRecorderPrivate only holds raw pointers in a map as a unique id
> for objects during playback. It relies on the Creation and Destruction
> events to keep track of them.
>
> It doesn't hold a reference to anything else as far as I can see.
> Also, the other *Recording types hold references to the recorder, so we'd
> presumably see cycles there if there was a problem.
>
> Does this reasoning sound correct?
I think you're right, yes, thanks!
Flags: needinfo?(bas)
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b57b890350b0e607b4d9b4ca266a761d20fae15e
Bug 1319456: Hold DrawEventRecorders in the correct structure in PathRecording. r=bas
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording
Requesting for release as well, in case there is something else that drives a dot release.
Approval Request Comment
[Feature/Bug causing the regression]:
Bug 792207 originally, but not actually triggered until bug 1156742.
[User impact if declined]:
They will continue experiencing content process crashes after printing in some circumstances.
[Is this code covered by automated tests?]:
No
[Has the fix been verified in Nightly?]:
No
[Needs manual test from QE? If yes, steps to reproduce]:
Yes - The crashes occur during GC or CC after printing in some circumstances, unfortunately we don't have steps to reproduce.
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
No
[Why is the change risky/not risky?]:
Very simple change to hold RefPtrs instead of raw pointers. Only slight concern was that this could introduce a reference cycle, but we're confident that's not the case.
[String changes made/needed]:
None
Attachment #8813557 -
Flags: approval-mozilla-release?
Attachment #8813557 -
Flags: approval-mozilla-beta?
Attachment #8813557 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years ago
|
Crash Signature: [@ std::_Hash<T>::equal_range ] → [@ std::_Hash<T>::equal_range ]
[@ std::_Hash<T>::_End ]
[@ std::_Hash<T>::erase ]
Assignee | ||
Comment 14•8 years ago
|
||
Comment 15•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
Comment 16•8 years ago
|
||
Comment on attachment 8813557 [details] [diff] [review]
Hold DrawEventRecorders in the correct structure in PathRecording
Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 7.
Attachment #8813557 -
Flags: approval-mozilla-beta?
Attachment #8813557 -
Flags: approval-mozilla-beta+
Attachment #8813557 -
Flags: approval-mozilla-aurora?
Attachment #8813557 -
Flags: approval-mozilla-aurora+
Comment 17•8 years ago
|
||
Comment 18•8 years ago
|
||
Updated•8 years ago
|
Blocks: 792207
Group: gfx-core-security → core-security-release
status-firefox-esr45:
--- → unaffected
Keywords: csectype-uaf,
sec-high
Assignee | ||
Updated•8 years ago
|
Attachment #8813557 -
Flags: approval-mozilla-release?
Updated•8 years ago
|
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Comment 19•8 years ago
|
||
This looks like it's going to be difficult to test without any STR/test cases. I went through the comments that have been submitted along side the crash reports, but I couldn't find any STR either. I went through some of the crash reports and attempted to reproduce the issue with some of the links that have been submitted alongside the crash reports but couldn't reproduce.
Looking at the crashes, I don't see this happening in any of the builds that have the fix from this patch. However, I didn't see any crashes occurring in anything other than early versions of 51beta and the current released versions [1] so I'm not sure that's the best indicator in telling us that this has been fixed.
If anyone has any idea's on how we can make sure this has been fixed, please let me know!
[1] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=std%3A%3A_Hash%3CT%3E%3A%3Aequal_range#summary
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•