From cc205131b021ebded854958973f445ed121da1b8 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Tue, 16 May 2017 14:01:47 -0700 Subject: Introduce CFX_UnownedPtr to detect lifetime inversion issues. There are places where an object "child" has a raw pointer back to object "owner" with the understanding that owner will always outlive child. Violating this constraint can lead to use after free, but this requires finding two paths: one that frees the objects in the wrong order, and one that uses the object after the free. The purpose of this patch is to detect the constraint violation even when the second path is not hit. We create a template that is used in place of TYPE*. It's dtor, when a memory tool is present, goes out and probes the first byte of the object to which it points. Used in "child", this allows the memory tool to prove that the "owner" is still alive at the time the child is destroyed, and hence the constraint is never violated. Change-Id: I2a6d696d51dda4a79ee2f00a6752965e058a6417 Reviewed-on: https://pdfium-review.googlesource.com/5475 Commit-Queue: Tom Sepez Reviewed-by: dsinclair Reviewed-by: Lei Zhang --- xfa/fxfa/cxfa_ffapp.cpp | 8 +++--- xfa/fxfa/cxfa_ffapp.h | 8 ++++-- xfa/fxfa/cxfa_ffdoc.cpp | 11 ++++---- xfa/fxfa/cxfa_ffdoc.h | 2 +- xfa/fxfa/cxfa_ffdocview.cpp | 46 ++++++++++++++++--------------- xfa/fxfa/cxfa_ffdocview.h | 11 ++++---- xfa/fxfa/cxfa_widgetacciterator.cpp | 2 +- xfa/fxfa/cxfa_widgetacciterator.h | 3 +- xfa/fxfa/parser/cxfa_resolveprocessor.cpp | 2 +- 9 files changed, 50 insertions(+), 43 deletions(-) (limited to 'xfa/fxfa') diff --git a/xfa/fxfa/cxfa_ffapp.cpp b/xfa/fxfa/cxfa_ffapp.cpp index 742cd14c30..7b97116c7a 100644 --- a/xfa/fxfa/cxfa_ffapp.cpp +++ b/xfa/fxfa/cxfa_ffapp.cpp @@ -23,10 +23,10 @@ #include "xfa/fxfa/cxfa_ffwidgethandler.h" #include "xfa/fxfa/cxfa_fontmgr.h" -CXFA_FFApp::CXFA_FFApp(IXFA_AppProvider* pProvider) - : m_pProvider(pProvider), - m_pWidgetMgrDelegate(nullptr), - m_pFWLApp(pdfium::MakeUnique(this)) {} +CXFA_FFApp::CXFA_FFApp(IXFA_AppProvider* pProvider) : m_pProvider(pProvider) { + // Ensure fully initialized before making an app based on |this|. + m_pFWLApp = pdfium::MakeUnique(this); +} CXFA_FFApp::~CXFA_FFApp() {} diff --git a/xfa/fxfa/cxfa_ffapp.h b/xfa/fxfa/cxfa_ffapp.h index 0a6409d3d1..1ae2073d82 100644 --- a/xfa/fxfa/cxfa_ffapp.h +++ b/xfa/fxfa/cxfa_ffapp.h @@ -13,6 +13,7 @@ #include "core/fpdfapi/parser/cpdf_stream.h" #include "core/fpdfapi/parser/cpdf_stream_acc.h" #include "core/fxcrt/cfx_retain_ptr.h" +#include "core/fxcrt/cfx_unowned_ptr.h" #include "xfa/fgas/font/cfgas_fontmgr.h" #include "xfa/fwl/cfwl_app.h" #include "xfa/fxfa/fxfa.h" @@ -44,7 +45,7 @@ class CXFA_FFApp { IFWL_AdapterTimerMgr* GetTimerMgr() const; CXFA_FontMgr* GetXFAFontMgr() const; CFWL_WidgetMgrDelegate* GetWidgetMgrDelegate() const { - return m_pWidgetMgrDelegate; + return m_pWidgetMgrDelegate.Get(); } void ClearEventTargets(); @@ -70,12 +71,15 @@ class CXFA_FFApp { std::unique_ptr m_pFontSource; #endif std::unique_ptr m_pAdapterWidgetMgr; - CFWL_WidgetMgrDelegate* m_pWidgetMgrDelegate; // not owned. // |m_pFWLApp| has to be released first, then |m_pFWLTheme| since the former // may refers to theme manager and the latter refers to font manager. std::unique_ptr m_pFWLTheme; std::unique_ptr m_pFWLApp; + + // |m_pWidgetMgrDelegate| has to be released before |m_pFWLApp|, since + // |m_pFWLApp| is its owner. + CFX_UnownedPtr m_pWidgetMgrDelegate; }; #endif // XFA_FXFA_CXFA_FFAPP_H_ diff --git a/xfa/fxfa/cxfa_ffdoc.cpp b/xfa/fxfa/cxfa_ffdoc.cpp index bddc02a9ce..f7bad38934 100644 --- a/xfa/fxfa/cxfa_ffdoc.cpp +++ b/xfa/fxfa/cxfa_ffdoc.cpp @@ -321,21 +321,20 @@ bool CXFA_FFDoc::OpenDoc(CPDF_Document* pPDFDoc) { return true; } -bool CXFA_FFDoc::CloseDoc() { - if (m_DocView) +void CXFA_FFDoc::CloseDoc() { + if (m_DocView) { m_DocView->RunDocClose(); - + m_DocView.reset(); + } CXFA_Document* doc = m_pDocumentParser ? m_pDocumentParser->GetDocument() : nullptr; if (doc) doc->ClearLayoutData(); - m_DocView.reset(); - m_pNotify.reset(nullptr); + m_pNotify.reset(); m_pApp->GetXFAFontMgr()->ReleaseDocFonts(this); m_HashToDibDpiMap.clear(); m_pApp->ClearEventTargets(); - return true; } CPDF_Document* CXFA_FFDoc::GetPDFDoc() { diff --git a/xfa/fxfa/cxfa_ffdoc.h b/xfa/fxfa/cxfa_ffdoc.h index 1cfbb50a2c..d40e7ba0c8 100644 --- a/xfa/fxfa/cxfa_ffdoc.h +++ b/xfa/fxfa/cxfa_ffdoc.h @@ -60,7 +60,7 @@ class CXFA_FFDoc { bool OpenDoc(const CFX_RetainPtr& pStream); bool OpenDoc(CPDF_Document* pPDFDoc); - bool CloseDoc(); + void CloseDoc(); CXFA_Document* GetXFADoc() { return m_pDocumentParser->GetDocument(); } CXFA_FFApp* GetApp() { return m_pApp; } diff --git a/xfa/fxfa/cxfa_ffdocview.cpp b/xfa/fxfa/cxfa_ffdocview.cpp index 058116be72..c1a8a67336 100644 --- a/xfa/fxfa/cxfa_ffdocview.cpp +++ b/xfa/fxfa/cxfa_ffdocview.cpp @@ -61,9 +61,6 @@ CXFA_FFDocView::CXFA_FFDocView(CXFA_FFDoc* pDoc) m_bInLayoutStatus(false), m_pDoc(pDoc), m_pXFADocLayout(nullptr), - m_pFocusAcc(nullptr), - m_pFocusWidget(nullptr), - m_pOldFocusWidget(nullptr), m_iStatus(XFA_DOCVIEW_LAYOUTSTATUS_None), m_iLock(0) {} @@ -143,14 +140,16 @@ void CXFA_FFDocView::StopLayout() { nullptr); } m_CalculateAccs.clear(); - if (m_pFocusAcc && !m_pFocusWidget) { - SetFocusWidgetAcc(m_pFocusAcc); - } + if (m_pFocusAcc && !m_pFocusWidget) + SetFocusWidgetAcc(m_pFocusAcc.Get()); + m_iStatus = XFA_DOCVIEW_LAYOUTSTATUS_End; } + int32_t CXFA_FFDocView::GetLayoutStatus() { return m_iStatus; } + void CXFA_FFDocView::ShowNullTestMsg() { int32_t iCount = pdfium::CollectionSize(m_arrNullTestMsg); CXFA_FFApp* pApp = m_pDoc->GetApp(); @@ -316,8 +315,8 @@ CXFA_FFDocView::CreateWidgetAccIterator() { return pdfium::MakeUnique(pFormRoot); } -CXFA_FFWidget* CXFA_FFDocView::GetFocusWidget() { - return m_pFocusWidget; +CXFA_FFWidget* CXFA_FFDocView::GetFocusWidget() const { + return m_pFocusWidget.Get(); } void CXFA_FFDocView::KillFocus() { @@ -329,12 +328,13 @@ void CXFA_FFDocView::KillFocus() { m_pFocusWidget = nullptr; m_pOldFocusWidget = nullptr; } + bool CXFA_FFDocView::SetFocus(CXFA_FFWidget* hWidget) { CXFA_FFWidget* pNewFocus = hWidget; - if (m_pOldFocusWidget == pNewFocus) { + if (m_pOldFocusWidget == pNewFocus) return false; - } - CXFA_FFWidget* pOldFocus = m_pOldFocusWidget; + + CXFA_FFWidget* pOldFocus = m_pOldFocusWidget.Get(); m_pOldFocusWidget = pNewFocus; if (pOldFocus) { if (m_pFocusWidget != m_pOldFocusWidget && @@ -345,15 +345,15 @@ bool CXFA_FFDocView::SetFocus(CXFA_FFWidget* hWidget) { if (!pOldFocus->IsLoaded()) { pOldFocus->LoadWidget(); } - pOldFocus->OnSetFocus(m_pFocusWidget); + pOldFocus->OnSetFocus(m_pFocusWidget.Get()); m_pFocusWidget = pOldFocus; pOldFocus->OnKillFocus(pNewFocus); } } - if (m_pFocusWidget == m_pOldFocusWidget) { + if (m_pFocusWidget == m_pOldFocusWidget) return false; - } - pNewFocus = m_pOldFocusWidget; + + pNewFocus = m_pOldFocusWidget.Get(); if (m_pListFocusWidget && pNewFocus == m_pListFocusWidget) { m_pFocusAcc = nullptr; m_pFocusWidget = nullptr; @@ -362,29 +362,30 @@ bool CXFA_FFDocView::SetFocus(CXFA_FFWidget* hWidget) { return false; } if (pNewFocus && (pNewFocus->GetStatus() & XFA_WidgetStatus_Visible)) { - if (!pNewFocus->IsLoaded()) { + if (!pNewFocus->IsLoaded()) pNewFocus->LoadWidget(); - } - pNewFocus->OnSetFocus(m_pFocusWidget); + pNewFocus->OnSetFocus(m_pFocusWidget.Get()); } m_pFocusAcc = pNewFocus ? pNewFocus->GetDataAcc() : nullptr; m_pFocusWidget = pNewFocus; m_pOldFocusWidget = m_pFocusWidget; return true; } + CXFA_WidgetAcc* CXFA_FFDocView::GetFocusWidgetAcc() { - return m_pFocusAcc; + return m_pFocusAcc.Get(); } + void CXFA_FFDocView::SetFocusWidgetAcc(CXFA_WidgetAcc* pWidgetAcc) { CXFA_FFWidget* pNewFocus = pWidgetAcc ? pWidgetAcc->GetNextWidget(nullptr) : nullptr; if (SetFocus(pNewFocus)) { m_pFocusAcc = pWidgetAcc; - if (m_iStatus == XFA_DOCVIEW_LAYOUTSTATUS_End) { - m_pDoc->GetDocEnvironment()->SetFocusWidget(m_pDoc, m_pFocusWidget); - } + if (m_iStatus == XFA_DOCVIEW_LAYOUTSTATUS_End) + m_pDoc->GetDocEnvironment()->SetFocusWidget(m_pDoc, m_pFocusWidget.Get()); } } + void CXFA_FFDocView::DeleteLayoutItem(CXFA_FFWidget* pWidget) { if (m_pFocusAcc == pWidget->GetDataAcc()) { m_pFocusAcc = nullptr; @@ -392,6 +393,7 @@ void CXFA_FFDocView::DeleteLayoutItem(CXFA_FFWidget* pWidget) { m_pOldFocusWidget = nullptr; } } + static int32_t XFA_ProcessEvent(CXFA_FFDocView* pDocView, CXFA_WidgetAcc* pWidgetAcc, CXFA_EventParam* pParam) { diff --git a/xfa/fxfa/cxfa_ffdocview.h b/xfa/fxfa/cxfa_ffdocview.h index 761397c4b0..83d077b548 100644 --- a/xfa/fxfa/cxfa_ffdocview.h +++ b/xfa/fxfa/cxfa_ffdocview.h @@ -11,6 +11,7 @@ #include #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "xfa/fxfa/cxfa_eventparam.h" #include "xfa/fxfa/cxfa_ffdoc.h" @@ -56,7 +57,7 @@ class CXFA_FFDocView { CXFA_WidgetAcc* pWidgetAcc); CXFA_FFWidgetHandler* GetWidgetHandler(); std::unique_ptr CreateWidgetAccIterator(); - CXFA_FFWidget* GetFocusWidget(); + CXFA_FFWidget* GetFocusWidget() const; void KillFocus(); bool SetFocus(CXFA_FFWidget* hWidget); CXFA_FFWidget* GetWidgetByName(const CFX_WideString& wsName, @@ -117,10 +118,10 @@ class CXFA_FFDocView { CXFA_FFDoc* const m_pDoc; std::unique_ptr m_pWidgetHandler; - CXFA_LayoutProcessor* m_pXFADocLayout; // not owned. - CXFA_WidgetAcc* m_pFocusAcc; // not owned. - CXFA_FFWidget* m_pFocusWidget; // not owned. - CXFA_FFWidget* m_pOldFocusWidget; // not owned. + CXFA_LayoutProcessor* m_pXFADocLayout; // Not owned. + CFX_UnownedPtr m_pFocusAcc; + CFX_UnownedPtr m_pFocusWidget; + CFX_UnownedPtr m_pOldFocusWidget; std::map> m_mapPageInvalidate; std::vector m_ValidateAccs; std::vector m_CalculateAccs; diff --git a/xfa/fxfa/cxfa_widgetacciterator.cpp b/xfa/fxfa/cxfa_widgetacciterator.cpp index 373fba99dc..ed780853df 100644 --- a/xfa/fxfa/cxfa_widgetacciterator.cpp +++ b/xfa/fxfa/cxfa_widgetacciterator.cpp @@ -19,7 +19,7 @@ CXFA_WidgetAcc* CXFA_WidgetAccIterator::MoveToNext() { while (pItem) { m_pCurWidgetAcc = static_cast(pItem->GetWidgetData()); if (m_pCurWidgetAcc) - return m_pCurWidgetAcc; + return m_pCurWidgetAcc.Get(); pItem = m_ContentIterator.MoveToNext(); } return nullptr; diff --git a/xfa/fxfa/cxfa_widgetacciterator.h b/xfa/fxfa/cxfa_widgetacciterator.h index ab18e833c2..0ad94260f3 100644 --- a/xfa/fxfa/cxfa_widgetacciterator.h +++ b/xfa/fxfa/cxfa_widgetacciterator.h @@ -7,6 +7,7 @@ #ifndef XFA_FXFA_CXFA_WIDGETACCITERATOR_H_ #define XFA_FXFA_CXFA_WIDGETACCITERATOR_H_ +#include "core/fxcrt/cfx_unowned_ptr.h" #include "xfa/fxfa/parser/cxfa_traversestrategy_xfacontainernode.h" class CXFA_Node; @@ -22,7 +23,7 @@ class CXFA_WidgetAccIterator { private: CXFA_ContainerIterator m_ContentIterator; - CXFA_WidgetAcc* m_pCurWidgetAcc; // not owned. + CFX_UnownedPtr m_pCurWidgetAcc; }; #endif // XFA_FXFA_CXFA_WIDGETACCITERATOR_H_ diff --git a/xfa/fxfa/parser/cxfa_resolveprocessor.cpp b/xfa/fxfa/parser/cxfa_resolveprocessor.cpp index d17a1bab50..add8cb8fcb 100644 --- a/xfa/fxfa/parser/cxfa_resolveprocessor.cpp +++ b/xfa/fxfa/parser/cxfa_resolveprocessor.cpp @@ -103,7 +103,7 @@ int32_t CXFA_ResolveProcessor::ResolveAnyChild(CXFA_ResolveNodesData& rnd) { return pdfium::CollectionSize(rnd.m_Objects); } std::vector tempNodes; - for (CXFA_Object* pObject : rnd.m_Objects) + for (auto* pObject : rnd.m_Objects) tempNodes.push_back(pObject->AsNode()); m_pNodeHelper->CountSiblings(findNode, XFA_LOGIC_Transparent, &tempNodes, bClassName); -- cgit v1.2.3