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 --- fpdfsdk/cpdfsdk_interform.cpp | 8 ++++---- fpdfsdk/cpdfsdk_interform.h | 7 +++++-- fpdfsdk/cpdfsdk_pageview.h | 8 ++++++-- fpdfsdk/fpdfxfa/cpdfxfa_context.cpp | 2 +- fpdfsdk/fpdfxfa/cpdfxfa_context.h | 12 ++++++++---- fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp | 2 +- fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h | 5 +++-- fpdfsdk/javascript/JS_EventHandler.h | 3 ++- fpdfsdk/pdfwindow/PWL_ComboBox.cpp | 4 ++-- fpdfsdk/pdfwindow/PWL_ComboBox.h | 3 ++- fpdfsdk/pdfwindow/PWL_Edit.cpp | 2 +- fpdfsdk/pdfwindow/PWL_Edit.h | 3 ++- fpdfsdk/pdfwindow/PWL_ListBox.cpp | 6 +++--- fpdfsdk/pdfwindow/PWL_ListBox.h | 3 ++- 14 files changed, 42 insertions(+), 26 deletions(-) (limited to 'fpdfsdk') diff --git a/fpdfsdk/cpdfsdk_interform.cpp b/fpdfsdk/cpdfsdk_interform.cpp index b44002cef8..2d02a7ab93 100644 --- a/fpdfsdk/cpdfsdk_interform.cpp +++ b/fpdfsdk/cpdfsdk_interform.cpp @@ -351,8 +351,8 @@ bool CPDFSDK_InterForm::OnKeyStrokeCommit(CPDF_FormField* pFormField, fa.bModifier = m_pFormFillEnv->IsCTRLKeyDown(0); fa.bShift = m_pFormFillEnv->IsSHIFTKeyDown(0); fa.sValue = csValue; - pActionHandler->DoAction_FieldJavaScript(action, CPDF_AAction::KeyStroke, - m_pFormFillEnv, pFormField, fa); + pActionHandler->DoAction_FieldJavaScript( + action, CPDF_AAction::KeyStroke, m_pFormFillEnv.Get(), pFormField, fa); return fa.bRC; } @@ -371,8 +371,8 @@ bool CPDFSDK_InterForm::OnValidate(CPDF_FormField* pFormField, fa.bModifier = m_pFormFillEnv->IsCTRLKeyDown(0); fa.bShift = m_pFormFillEnv->IsSHIFTKeyDown(0); fa.sValue = csValue; - pActionHandler->DoAction_FieldJavaScript(action, CPDF_AAction::Validate, - m_pFormFillEnv, pFormField, fa); + pActionHandler->DoAction_FieldJavaScript( + action, CPDF_AAction::Validate, m_pFormFillEnv.Get(), pFormField, fa); return fa.bRC; } diff --git a/fpdfsdk/cpdfsdk_interform.h b/fpdfsdk/cpdfsdk_interform.h index 032399c84e..b613c731ba 100644 --- a/fpdfsdk/cpdfsdk_interform.h +++ b/fpdfsdk/cpdfsdk_interform.h @@ -13,6 +13,7 @@ #include "core/fpdfdoc/cpdf_action.h" #include "core/fpdfdoc/ipdf_formnotify.h" +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_basic.h" #include "core/fxge/fx_dib.h" #include "fpdfsdk/cpdfsdk_widget.h" @@ -35,7 +36,9 @@ class CPDFSDK_InterForm : public IPDF_FormNotify { ~CPDFSDK_InterForm() override; CPDF_InterForm* GetInterForm() const { return m_pInterForm.get(); } - CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { return m_pFormFillEnv; } + CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { + return m_pFormFillEnv.Get(); + } bool HighlightWidgets(); @@ -121,7 +124,7 @@ class CPDFSDK_InterForm : public IPDF_FormNotify { using CPDFSDK_WidgetMap = std::map; - CPDFSDK_FormFillEnvironment* m_pFormFillEnv; // Not owned. + CFX_UnownedPtr m_pFormFillEnv; std::unique_ptr m_pInterForm; CPDFSDK_WidgetMap m_Map; #ifdef PDF_ENABLE_XFA diff --git a/fpdfsdk/cpdfsdk_pageview.h b/fpdfsdk/cpdfsdk_pageview.h index bcd5177c1a..8bede08a5d 100644 --- a/fpdfsdk/cpdfsdk_pageview.h +++ b/fpdfsdk/cpdfsdk_pageview.h @@ -11,6 +11,7 @@ #include #include "core/fpdfapi/page/cpdf_page.h" +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_system.h" #include "fpdfsdk/cpdfsdk_annot.h" @@ -55,7 +56,10 @@ class CPDFSDK_PageView final : public CPDF_Page::View { CPDF_Page* GetPDFPage() const; CPDF_Document* GetPDFDocument(); - CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { return m_pFormFillEnv; } + CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { + return m_pFormFillEnv.Get(); + } + bool OnLButtonDown(const CFX_PointF& point, uint32_t nFlag); bool OnLButtonUp(const CFX_PointF& point, uint32_t nFlag); #ifdef PDF_ENABLE_XFA @@ -102,7 +106,7 @@ class CPDFSDK_PageView final : public CPDF_Page::View { UnderlyingPageType* const m_page; std::unique_ptr m_pAnnotList; std::vector m_SDKAnnotArray; - CPDFSDK_FormFillEnvironment* const m_pFormFillEnv; // Not owned. + CFX_UnownedPtr const m_pFormFillEnv; CPDFSDK_Annot::ObservedPtr m_pCaptureWidget; #ifndef PDF_ENABLE_XFA bool m_bOwnsPage; diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp index 454e2187a3..93ecab0d08 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp +++ b/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp @@ -358,6 +358,6 @@ bool CPDFXFA_Context::PutRequestURL(const CFX_WideString& wsURL, IFWL_AdapterTimerMgr* CPDFXFA_Context::GetTimerMgr() { CXFA_FWLAdapterTimerMgr* pAdapter = nullptr; if (m_pFormFillEnv) - pAdapter = new CXFA_FWLAdapterTimerMgr(m_pFormFillEnv); + pAdapter = new CXFA_FWLAdapterTimerMgr(m_pFormFillEnv.Get()); return pAdapter; } diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_context.h b/fpdfsdk/fpdfxfa/cpdfxfa_context.h index 8e6b2198af..690e1855e3 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_context.h +++ b/fpdfsdk/fpdfxfa/cpdfxfa_context.h @@ -10,6 +10,8 @@ #include #include +#include "core/fxcrt/cfx_unowned_ptr.h" +#include "core/fxcrt/fx_system.h" #include "fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h" #include "xfa/fxfa/cxfa_ffdoc.h" @@ -36,12 +38,14 @@ class CPDFXFA_Context : public IXFA_AppProvider { bool LoadXFADoc(); CPDF_Document* GetPDFDoc() { return m_pPDFDoc.get(); } CXFA_FFDoc* GetXFADoc() { return m_pXFADoc.get(); } - CXFA_FFDocView* GetXFADocView() { return m_pXFADocView; } + CXFA_FFDocView* GetXFADocView() { return m_pXFADocView.Get(); } XFA_DocType GetDocType() const { return m_iDocType; } v8::Isolate* GetJSERuntime() const; CXFA_FFApp* GetXFAApp() { return m_pXFAApp.get(); } - CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { return m_pFormFillEnv; } + CPDFSDK_FormFillEnvironment* GetFormFillEnv() const { + return m_pFormFillEnv.Get(); + } void SetFormFillEnv(CPDFSDK_FormFillEnvironment* pFormFillEnv); void DeletePage(int page_index); @@ -102,8 +106,8 @@ class CPDFXFA_Context : public IXFA_AppProvider { std::unique_ptr m_pPDFDoc; std::unique_ptr m_pXFADoc; - CPDFSDK_FormFillEnvironment* m_pFormFillEnv; // not owned. - CXFA_FFDocView* m_pXFADocView; // not owned. + CFX_UnownedPtr m_pFormFillEnv; + CFX_UnownedPtr m_pXFADocView; std::unique_ptr m_pXFAApp; std::unique_ptr m_pRuntime; std::vector m_XFAPageList; diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp index 4464f1c43a..1a27d9b015 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp +++ b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.cpp @@ -46,7 +46,7 @@ CPDFXFA_DocEnvironment::CPDFXFA_DocEnvironment(CPDFXFA_Context* pContext) CPDFXFA_DocEnvironment::~CPDFXFA_DocEnvironment() { if (m_pJSEventContext && m_pContext->GetFormFillEnv()) { m_pContext->GetFormFillEnv()->GetJSRuntime()->ReleaseEventContext( - m_pJSEventContext); + m_pJSEventContext.Get()); } } diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h index 8f1d238103..ec04e78db9 100644 --- a/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h +++ b/fpdfsdk/fpdfxfa/cpdfxfa_docenvironment.h @@ -8,6 +8,7 @@ #define FPDFSDK_FPDFXFA_CPDFXFA_DOCENVIRONMENT_H_ #include "core/fxcrt/cfx_retain_ptr.h" +#include "core/fxcrt/cfx_unowned_ptr.h" #include "public/fpdfview.h" #include "xfa/fxfa/fxfa.h" @@ -105,8 +106,8 @@ class CPDFXFA_DocEnvironment : public IXFA_DocEnvironment { FPDF_DWORD flag); void ToXFAContentFlags(CFX_WideString csSrcContent, FPDF_DWORD& flag); - CPDFXFA_Context* const m_pContext; // Not owned. - IJS_EventContext* m_pJSEventContext; // Not owned. + CFX_UnownedPtr const m_pContext; + CFX_UnownedPtr m_pJSEventContext; }; #endif // FPDFSDK_FPDFXFA_CPDFXFA_DOCENVIRONMENT_H_ diff --git a/fpdfsdk/javascript/JS_EventHandler.h b/fpdfsdk/javascript/JS_EventHandler.h index 1e70a358bc..5dcb70b4c8 100644 --- a/fpdfsdk/javascript/JS_EventHandler.h +++ b/fpdfsdk/javascript/JS_EventHandler.h @@ -7,6 +7,7 @@ #ifndef FPDFSDK_JAVASCRIPT_JS_EVENTHANDLER_H_ #define FPDFSDK_JAVASCRIPT_JS_EVENTHANDLER_H_ +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_string.h" #include "core/fxcrt/fx_system.h" #include "fpdfsdk/cpdfsdk_formfillenvironment.h" @@ -165,7 +166,7 @@ class CJS_EventHandler { JS_EVENT_T EventType() { return m_eEventType; } public: - CJS_EventContext* const m_pJSEventContext; // Not Owned. + CFX_UnownedPtr const m_pJSEventContext; JS_EVENT_T m_eEventType; bool m_bValid; diff --git a/fpdfsdk/pdfwindow/PWL_ComboBox.cpp b/fpdfsdk/pdfwindow/PWL_ComboBox.cpp index 9321e0f606..9f5ab97858 100644 --- a/fpdfsdk/pdfwindow/PWL_ComboBox.cpp +++ b/fpdfsdk/pdfwindow/PWL_ComboBox.cpp @@ -265,7 +265,7 @@ void CPWL_ComboBox::CreateEdit(const PWL_CREATEPARAM& cp) { return; m_pEdit = pdfium::MakeUnique(); - m_pEdit->AttachFFLData(m_pFormFiller); + m_pEdit->AttachFFLData(m_pFormFiller.Get()); PWL_CREATEPARAM ecp = cp; ecp.pParentWnd = this; @@ -306,7 +306,7 @@ void CPWL_ComboBox::CreateListBox(const PWL_CREATEPARAM& cp) { return; m_pList = pdfium::MakeUnique(); - m_pList->AttachFFLData(m_pFormFiller); + m_pList->AttachFFLData(m_pFormFiller.Get()); PWL_CREATEPARAM lcp = cp; lcp.pParentWnd = this; diff --git a/fpdfsdk/pdfwindow/PWL_ComboBox.h b/fpdfsdk/pdfwindow/PWL_ComboBox.h index a687eb8e94..19d9ab2a04 100644 --- a/fpdfsdk/pdfwindow/PWL_ComboBox.h +++ b/fpdfsdk/pdfwindow/PWL_ComboBox.h @@ -9,6 +9,7 @@ #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "fpdfsdk/pdfwindow/PWL_Edit.h" #include "fpdfsdk/pdfwindow/PWL_ListBox.h" #include "fpdfsdk/pdfwindow/PWL_Wnd.h" @@ -100,7 +101,7 @@ class CPWL_ComboBox : public CPWL_Wnd { int32_t m_nPopupWhere; int32_t m_nSelectItem; IPWL_Filler_Notify* m_pFillerNotify; - CFFL_FormFiller* m_pFormFiller; // Not owned. + CFX_UnownedPtr m_pFormFiller; }; #endif // FPDFSDK_PDFWINDOW_PWL_COMBOBOX_H_ diff --git a/fpdfsdk/pdfwindow/PWL_Edit.cpp b/fpdfsdk/pdfwindow/PWL_Edit.cpp index 90572746da..4e37e9bde4 100644 --- a/fpdfsdk/pdfwindow/PWL_Edit.cpp +++ b/fpdfsdk/pdfwindow/PWL_Edit.cpp @@ -398,7 +398,7 @@ void CPWL_Edit::DrawThisAppearance(CFX_RenderDevice* pDevice, CFX_SystemHandler* pSysHandler = GetSystemHandler(); CFX_Edit::DrawEdit(pDevice, pUser2Device, m_pEdit.get(), GetTextColor().ToFXColor(GetTransparency()), rcClip, - CFX_PointF(), pRange, pSysHandler, m_pFormFiller); + CFX_PointF(), pRange, pSysHandler, m_pFormFiller.Get()); } bool CPWL_Edit::OnLButtonDown(const CFX_PointF& point, uint32_t nFlag) { diff --git a/fpdfsdk/pdfwindow/PWL_Edit.h b/fpdfsdk/pdfwindow/PWL_Edit.h index 5e1a36610d..bd3799141a 100644 --- a/fpdfsdk/pdfwindow/PWL_Edit.h +++ b/fpdfsdk/pdfwindow/PWL_Edit.h @@ -9,6 +9,7 @@ #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_basic.h" #include "fpdfsdk/fxedit/fx_edit.h" #include "fpdfsdk/pdfwindow/PWL_EditCtrl.h" @@ -136,7 +137,7 @@ class CPWL_Edit : public CPWL_EditCtrl { IPWL_Filler_Notify* m_pFillerNotify; bool m_bFocus; CFX_FloatRect m_rcOldWindow; - CFFL_FormFiller* m_pFormFiller; // Not owned. + CFX_UnownedPtr m_pFormFiller; }; #endif // FPDFSDK_PDFWINDOW_PWL_EDIT_H_ diff --git a/fpdfsdk/pdfwindow/PWL_ListBox.cpp b/fpdfsdk/pdfwindow/PWL_ListBox.cpp index 8448204a74..b682959426 100644 --- a/fpdfsdk/pdfwindow/PWL_ListBox.cpp +++ b/fpdfsdk/pdfwindow/PWL_ListBox.cpp @@ -172,14 +172,14 @@ void CPWL_ListBox::DrawThisAppearance(CFX_RenderDevice* pDevice, if (pSysHandler && pSysHandler->IsSelectionImplemented()) { CFX_Edit::DrawEdit(pDevice, pUser2Device, m_pList->GetItemEdit(i), GetTextColor().ToFXColor(255), rcList, ptOffset, - nullptr, pSysHandler, m_pFormFiller); - pSysHandler->OutputSelectedRect(m_pFormFiller, rcItem); + nullptr, pSysHandler, m_pFormFiller.Get()); + pSysHandler->OutputSelectedRect(m_pFormFiller.Get(), rcItem); } else { CPWL_Utils::DrawFillRect(pDevice, pUser2Device, rcItem, ArgbEncode(255, 0, 51, 113)); CFX_Edit::DrawEdit(pDevice, pUser2Device, m_pList->GetItemEdit(i), ArgbEncode(255, 255, 255, 255), rcList, ptOffset, - nullptr, pSysHandler, m_pFormFiller); + nullptr, pSysHandler, m_pFormFiller.Get()); } } else { CFX_SystemHandler* pSysHandler = GetSystemHandler(); diff --git a/fpdfsdk/pdfwindow/PWL_ListBox.h b/fpdfsdk/pdfwindow/PWL_ListBox.h index 9f8f464efb..0de9c91195 100644 --- a/fpdfsdk/pdfwindow/PWL_ListBox.h +++ b/fpdfsdk/pdfwindow/PWL_ListBox.h @@ -9,6 +9,7 @@ #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "fpdfsdk/fxedit/fx_edit.h" #include "fpdfsdk/pdfwindow/PWL_Wnd.h" @@ -111,7 +112,7 @@ class CPWL_ListBox : public CPWL_Wnd { IPWL_Filler_Notify* m_pFillerNotify; private: - CFFL_FormFiller* m_pFormFiller; // Not owned. + CFX_UnownedPtr m_pFormFiller; }; #endif // FPDFSDK_PDFWINDOW_PWL_LISTBOX_H_ -- cgit v1.2.3