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 --- core/fpdfapi/page/cpdf_contentmarkitem.cpp | 2 +- core/fpdfapi/page/cpdf_contentmarkitem.h | 3 ++- core/fpdfapi/parser/cpdf_parser.cpp | 15 +++++++-------- core/fpdfapi/parser/cpdf_parser.h | 3 ++- core/fpdfapi/render/cpdf_docrenderdata.cpp | 2 +- core/fpdfapi/render/cpdf_docrenderdata.h | 4 +++- 6 files changed, 16 insertions(+), 13 deletions(-) (limited to 'core/fpdfapi') diff --git a/core/fpdfapi/page/cpdf_contentmarkitem.cpp b/core/fpdfapi/page/cpdf_contentmarkitem.cpp index 2c370141e2..de720b39fc 100644 --- a/core/fpdfapi/page/cpdf_contentmarkitem.cpp +++ b/core/fpdfapi/page/cpdf_contentmarkitem.cpp @@ -26,7 +26,7 @@ CPDF_ContentMarkItem::~CPDF_ContentMarkItem() {} CPDF_Dictionary* CPDF_ContentMarkItem::GetParam() const { switch (m_ParamType) { case PropertiesDict: - return m_pPropertiesDict; + return m_pPropertiesDict.Get(); case DirectDict: return m_pDirectDict.get(); case None: diff --git a/core/fpdfapi/page/cpdf_contentmarkitem.h b/core/fpdfapi/page/cpdf_contentmarkitem.h index ed2737111b..afd2833691 100644 --- a/core/fpdfapi/page/cpdf_contentmarkitem.h +++ b/core/fpdfapi/page/cpdf_contentmarkitem.h @@ -9,6 +9,7 @@ #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_memory.h" #include "core/fxcrt/fx_string.h" #include "core/fxcrt/fx_system.h" @@ -37,7 +38,7 @@ class CPDF_ContentMarkItem { private: CFX_ByteString m_MarkName; ParamType m_ParamType; - CPDF_Dictionary* m_pPropertiesDict; // not owned. + CFX_UnownedPtr m_pPropertiesDict; std::unique_ptr m_pDirectDict; }; diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp index a3e5dba40a..54b8ea0aa3 100644 --- a/core/fpdfapi/parser/cpdf_parser.cpp +++ b/core/fpdfapi/parser/cpdf_parser.cpp @@ -52,8 +52,7 @@ int32_t GetStreamFirst(const CFX_RetainPtr& pObjStream) { } // namespace CPDF_Parser::CPDF_Parser() - : m_pDocument(nullptr), - m_bHasParsed(false), + : m_bHasParsed(false), m_bXRefStream(false), m_bVersionUpdated(false), m_FileVersion(0), @@ -719,8 +718,8 @@ bool CPDF_Parser::RebuildCrossRef() { last_obj = start_pos; FX_FILESIZE obj_end = 0; std::unique_ptr pObject = - ParseIndirectObjectAtByStrict(m_pDocument, obj_pos, objnum, - &obj_end); + ParseIndirectObjectAtByStrict(m_pDocument.Get(), obj_pos, + objnum, &obj_end); if (CPDF_Stream* pStream = ToStream(pObject.get())) { if (CPDF_Dictionary* pDict = pStream->GetDict()) { if ((pDict->KeyExist("Type")) && @@ -779,7 +778,7 @@ bool CPDF_Parser::RebuildCrossRef() { m_pSyntax->SetPos(pos + i - m_pSyntax->m_HeaderOffset); std::unique_ptr pObj = - m_pSyntax->GetObject(m_pDocument, 0, 0, true); + m_pSyntax->GetObject(m_pDocument.Get(), 0, 0, true); if (pObj) { if (pObj->IsDictionary() || pObj->AsStream()) { CPDF_Stream* pStream = pObj->AsStream(); @@ -800,7 +799,7 @@ bool CPDF_Parser::RebuildCrossRef() { pElement ? pElement->GetObjNum() : 0; if (dwObjNum) { m_pTrailer->SetNewFor( - key, m_pDocument, dwObjNum); + key, m_pDocument.Get(), dwObjNum); } else { m_pTrailer->SetFor(key, pElement->Clone()); } @@ -920,7 +919,7 @@ bool CPDF_Parser::RebuildCrossRef() { bool CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, bool bMainXRef) { std::unique_ptr pObject( - ParseIndirectObjectAt(m_pDocument, *pos, 0)); + ParseIndirectObjectAt(m_pDocument.Get(), *pos, 0)); if (!pObject) return false; @@ -1397,7 +1396,7 @@ std::unique_ptr CPDF_Parser::LoadTrailerV4() { if (m_pSyntax->GetKeyword() != "trailer") return nullptr; - return ToDictionary(m_pSyntax->GetObject(m_pDocument, 0, 0, true)); + return ToDictionary(m_pSyntax->GetObject(m_pDocument.Get(), 0, 0, true)); } uint32_t CPDF_Parser::GetPermissions() const { diff --git a/core/fpdfapi/parser/cpdf_parser.h b/core/fpdfapi/parser/cpdf_parser.h index 7ae2d4627e..c444c99f28 100644 --- a/core/fpdfapi/parser/cpdf_parser.h +++ b/core/fpdfapi/parser/cpdf_parser.h @@ -12,6 +12,7 @@ #include #include +#include "core/fxcrt/cfx_unowned_ptr.h" #include "core/fxcrt/fx_basic.h" class CPDF_Array; @@ -150,7 +151,7 @@ class CPDF_Parser { // the objects. bool VerifyCrossRefV4(); - CPDF_Document* m_pDocument; // not owned + CFX_UnownedPtr m_pDocument; bool m_bHasParsed; bool m_bXRefStream; bool m_bVersionUpdated; diff --git a/core/fpdfapi/render/cpdf_docrenderdata.cpp b/core/fpdfapi/render/cpdf_docrenderdata.cpp index 2d0bca4497..d03824b59e 100644 --- a/core/fpdfapi/render/cpdf_docrenderdata.cpp +++ b/core/fpdfapi/render/cpdf_docrenderdata.cpp @@ -88,7 +88,7 @@ CFX_RetainPtr CPDF_DocRenderData::GetTransferFunc( if (!pFuncs[0]) return nullptr; } - auto pTransfer = pdfium::MakeRetain(m_pPDFDoc); + auto pTransfer = pdfium::MakeRetain(m_pPDFDoc.Get()); m_TransferFuncMap[pObj] = pTransfer; float input; diff --git a/core/fpdfapi/render/cpdf_docrenderdata.h b/core/fpdfapi/render/cpdf_docrenderdata.h index 7952f95991..949a079126 100644 --- a/core/fpdfapi/render/cpdf_docrenderdata.h +++ b/core/fpdfapi/render/cpdf_docrenderdata.h @@ -11,6 +11,8 @@ #include "core/fpdfapi/page/cpdf_countedobject.h" #include "core/fpdfapi/render/cpdf_transferfunc.h" +#include "core/fxcrt/cfx_unowned_ptr.h" +#include "core/fxcrt/fx_system.h" class CPDF_Document; class CPDF_Font; @@ -32,7 +34,7 @@ class CPDF_DocRenderData { void Clear(bool bRelease); private: - CPDF_Document* m_pPDFDoc; // Not Owned + CFX_UnownedPtr m_pPDFDoc; std::map> m_Type3FaceMap; std::map> m_TransferFuncMap; }; -- cgit v1.2.3