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/fwl/cfwl_caret.cpp | 2 +- xfa/fwl/cfwl_caret.h | 2 +- xfa/fwl/cfwl_scrollbar.cpp | 3 +-- xfa/fwl/cfwl_spinbutton.cpp | 4 +--- xfa/fwl/cfwl_timer.cpp | 4 ++++ xfa/fwl/cfwl_timer.h | 7 ++++--- xfa/fwl/cfwl_timerinfo.cpp | 6 ++++++ xfa/fwl/cfwl_timerinfo.h | 9 ++++----- xfa/fwl/cfwl_widget.h | 8 +++++--- 9 files changed, 27 insertions(+), 18 deletions(-) (limited to 'xfa/fwl') diff --git a/xfa/fwl/cfwl_caret.cpp b/xfa/fwl/cfwl_caret.cpp index a6ea9ddb56..21387a47f9 100644 --- a/xfa/fwl/cfwl_caret.cpp +++ b/xfa/fwl/cfwl_caret.cpp @@ -97,7 +97,7 @@ void CFWL_Caret::OnDrawWidget(CFX_Graphics* pGraphics, CFWL_Caret::Timer::Timer(CFWL_Caret* pCaret) : CFWL_Timer(pCaret) {} void CFWL_Caret::Timer::Run(CFWL_TimerInfo* pTimerInfo) { - CFWL_Caret* pCaret = static_cast(m_pWidget); + CFWL_Caret* pCaret = static_cast(m_pWidget.Get()); if (!(pCaret->GetStates() & FWL_STATE_CAT_HightLight)) pCaret->SetStates(FWL_STATE_CAT_HightLight); else diff --git a/xfa/fwl/cfwl_caret.h b/xfa/fwl/cfwl_caret.h index e0c07d301a..da0548c2bf 100644 --- a/xfa/fwl/cfwl_caret.h +++ b/xfa/fwl/cfwl_caret.h @@ -51,7 +51,7 @@ class CFWL_Caret : public CFWL_Widget { const CFX_Matrix* pMatrix); std::unique_ptr m_pTimer; - CFWL_TimerInfo* m_pTimerInfo; // not owned. + CFX_UnownedPtr m_pTimerInfo; }; #endif // XFA_FWL_CFWL_CARET_H_ diff --git a/xfa/fwl/cfwl_scrollbar.cpp b/xfa/fwl/cfwl_scrollbar.cpp index 72797ed315..79e8b48115 100644 --- a/xfa/fwl/cfwl_scrollbar.cpp +++ b/xfa/fwl/cfwl_scrollbar.cpp @@ -485,8 +485,7 @@ void CFWL_ScrollBar::DoMouseHover(int32_t iItem, CFWL_ScrollBar::Timer::Timer(CFWL_ScrollBar* pToolTip) : CFWL_Timer(pToolTip) {} void CFWL_ScrollBar::Timer::Run(CFWL_TimerInfo* pTimerInfo) { - CFWL_ScrollBar* pButton = static_cast(m_pWidget); - + CFWL_ScrollBar* pButton = static_cast(m_pWidget.Get()); if (pButton->m_pTimerInfo) pButton->m_pTimerInfo->StopTimer(); diff --git a/xfa/fwl/cfwl_spinbutton.cpp b/xfa/fwl/cfwl_spinbutton.cpp index 6e58b69baf..65bd6cdd3a 100644 --- a/xfa/fwl/cfwl_spinbutton.cpp +++ b/xfa/fwl/cfwl_spinbutton.cpp @@ -336,7 +336,6 @@ void CFWL_SpinButton::OnKeyDown(CFWL_MessageKey* pMsg) { CFWL_Event wmPosChanged(CFWL_Event::Type::Click, this); DispatchEvent(&wmPosChanged); - RepaintRect(bUpEnable ? m_rtUpButton : m_rtDnButton); } @@ -344,8 +343,7 @@ CFWL_SpinButton::Timer::Timer(CFWL_SpinButton* pToolTip) : CFWL_Timer(pToolTip) {} void CFWL_SpinButton::Timer::Run(CFWL_TimerInfo* pTimerInfo) { - CFWL_SpinButton* pButton = static_cast(m_pWidget); - + CFWL_SpinButton* pButton = static_cast(m_pWidget.Get()); if (!pButton->m_pTimerInfo) return; diff --git a/xfa/fwl/cfwl_timer.cpp b/xfa/fwl/cfwl_timer.cpp index 0cd3b2b768..2734e49d78 100644 --- a/xfa/fwl/cfwl_timer.cpp +++ b/xfa/fwl/cfwl_timer.cpp @@ -12,6 +12,10 @@ #include "xfa/fwl/ifwl_adaptertimermgr.h" #include "xfa/fxfa/cxfa_ffapp.h" +CFWL_Timer::CFWL_Timer(CFWL_Widget* parent) : m_pWidget(parent) {} + +CFWL_Timer::~CFWL_Timer() {} + CFWL_TimerInfo* CFWL_Timer::StartTimer(uint32_t dwElapse, bool bImmediately) { const CFWL_App* pApp = m_pWidget->GetOwnerApp(); if (!pApp) diff --git a/xfa/fwl/cfwl_timer.h b/xfa/fwl/cfwl_timer.h index 6c9079acfe..93de009ed1 100644 --- a/xfa/fwl/cfwl_timer.h +++ b/xfa/fwl/cfwl_timer.h @@ -7,6 +7,7 @@ #ifndef XFA_FWL_CFWL_TIMER_H_ #define XFA_FWL_CFWL_TIMER_H_ +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_system.h" class CFWL_TimerInfo; @@ -14,14 +15,14 @@ class CFWL_Widget; class CFWL_Timer { public: - explicit CFWL_Timer(CFWL_Widget* parent) : m_pWidget(parent) {} - virtual ~CFWL_Timer() {} + explicit CFWL_Timer(CFWL_Widget* parent); + virtual ~CFWL_Timer(); virtual void Run(CFWL_TimerInfo* hTimer) = 0; CFWL_TimerInfo* StartTimer(uint32_t dwElapse, bool bImmediately); protected: - CFWL_Widget* m_pWidget; // Not owned. + CFX_UnownedPtr m_pWidget; }; #endif // XFA_FWL_CFWL_TIMER_H_ diff --git a/xfa/fwl/cfwl_timerinfo.cpp b/xfa/fwl/cfwl_timerinfo.cpp index 8c7aaebbbb..ee4746af3f 100644 --- a/xfa/fwl/cfwl_timerinfo.cpp +++ b/xfa/fwl/cfwl_timerinfo.cpp @@ -8,6 +8,12 @@ #include "xfa/fwl/ifwl_adaptertimermgr.h" +CFWL_TimerInfo::CFWL_TimerInfo(IFWL_AdapterTimerMgr* mgr) : m_pMgr(mgr) { + ASSERT(mgr); +} + +CFWL_TimerInfo::~CFWL_TimerInfo() {} + void CFWL_TimerInfo::StopTimer() { m_pMgr->Stop(this); } diff --git a/xfa/fwl/cfwl_timerinfo.h b/xfa/fwl/cfwl_timerinfo.h index a47fede60f..ae77ef5d59 100644 --- a/xfa/fwl/cfwl_timerinfo.h +++ b/xfa/fwl/cfwl_timerinfo.h @@ -7,21 +7,20 @@ #ifndef XFA_FWL_CFWL_TIMERINFO_H_ #define XFA_FWL_CFWL_TIMERINFO_H_ +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_system.h" class IFWL_AdapterTimerMgr; class CFWL_TimerInfo { public: - explicit CFWL_TimerInfo(IFWL_AdapterTimerMgr* mgr) : m_pMgr(mgr) { - ASSERT(mgr); - } - virtual ~CFWL_TimerInfo() {} + explicit CFWL_TimerInfo(IFWL_AdapterTimerMgr* mgr); + virtual ~CFWL_TimerInfo(); void StopTimer(); private: - IFWL_AdapterTimerMgr* m_pMgr; // Not owned. + CFX_UnownedPtr m_pMgr; }; #endif // XFA_FWL_CFWL_TIMERINFO_H_ diff --git a/xfa/fwl/cfwl_widget.h b/xfa/fwl/cfwl_widget.h index b556e7a424..dd8fb5654b 100644 --- a/xfa/fwl/cfwl_widget.h +++ b/xfa/fwl/cfwl_widget.h @@ -6,8 +6,10 @@ #ifndef XFA_FWL_CFWL_WIDGET_H_ #define XFA_FWL_CFWL_WIDGET_H_ + #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_coordinates.h" #include "core/fxcrt/fx_system.h" #include "xfa/fwl/cfwl_event.h" @@ -96,10 +98,10 @@ class CFWL_Widget : public IFWL_WidgetDelegate { void SetDelegate(IFWL_WidgetDelegate* delegate) { m_pDelegate = delegate; } IFWL_WidgetDelegate* GetDelegate() { - return m_pDelegate ? m_pDelegate : this; + return m_pDelegate ? m_pDelegate.Get() : this; } const IFWL_WidgetDelegate* GetDelegate() const { - return m_pDelegate ? m_pDelegate : this; + return m_pDelegate ? m_pDelegate.Get() : this; } const CFWL_App* GetOwnerApp() const { return m_pOwnerApp; } @@ -182,7 +184,7 @@ class CFWL_Widget : public IFWL_WidgetDelegate { CXFA_FFWidget* m_pLayoutItem; uint32_t m_nEventKey; - IFWL_WidgetDelegate* m_pDelegate; // Not owned. + CFX_UnownedPtr m_pDelegate; }; #endif // XFA_FWL_CFWL_WIDGET_H_ -- cgit v1.2.3