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 <tsepez@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>

11 files changed, 173 insertions, 19 deletions

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 <memory>
 
+#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<CPDF_Dictionary> m_pPropertiesDict;
   std::unique_ptr<CPDF_Dictionary> 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<CPDF_StreamAcc>& 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<CPDF_Object> 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<CPDF_Object> 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<CPDF_Reference>(
-                                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<CPDF_Object> pObject(
-      ParseIndirectObjectAt(m_pDocument, *pos, 0));
+      ParseIndirectObjectAt(m_pDocument.Get(), *pos, 0));
   if (!pObject)
     return false;
 
@@ -1397,7 +1396,7 @@ std::unique_ptr<CPDF_Dictionary> 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 <set>
 #include <vector>
 
+#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<CPDF_Document> 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_TransferFunc> CPDF_DocRenderData::GetTransferFunc(
     if (!pFuncs[0])
       return nullptr;
   }
-  auto pTransfer = pdfium::MakeRetain<CPDF_TransferFunc>(m_pPDFDoc);
+  auto pTransfer = pdfium::MakeRetain<CPDF_TransferFunc>(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<CPDF_Document> m_pPDFDoc;
   std::map<CPDF_Font*, CFX_RetainPtr<CPDF_Type3Cache>> m_Type3FaceMap;
   std::map<CPDF_Object*, CFX_RetainPtr<CPDF_TransferFunc>> m_TransferFuncMap;
 };
diff --git a/core/fpdfdoc/cpdf_filespec.h b/core/fpdfdoc/cpdf_filespec.h
index ea002f0e22..fb71b57186 100644
--- a/core/fpdfdoc/cpdf_filespec.h
+++ b/core/fpdfdoc/cpdf_filespec.h
@@ -8,6 +8,7 @@
 #define CORE_FPDFDOC_CPDF_FILESPEC_H_
 
 #include "core/fxcrt/cfx_string_pool_template.h"
+#include "core/fxcrt/cfx_unowned_ptr.h"
 #include "core/fxcrt/cfx_weak_ptr.h"
 #include "core/fxcrt/fx_string.h"
 
@@ -24,14 +25,14 @@ class CPDF_FileSpec {
   // Convert a pdf file name into platform dependent format.
   static CFX_WideString DecodeFileName(const CFX_WideStringC& filepath);
 
-  CPDF_Object* GetObj() const { return m_pObj; }
+  CPDF_Object* GetObj() const { return m_pObj.Get(); }
   bool GetFileName(CFX_WideString* wsFileName) const;
 
   // Set this file spec to refer to a file name (not a url).
   void SetFileName(const CFX_WideStringC& wsFileName);
 
  private:
-  CPDF_Object* const m_pObj;  // not owned.
+  CFX_UnownedPtr<CPDF_Object> const m_pObj;
 };
 
 #endif  // CORE_FPDFDOC_CPDF_FILESPEC_H_
diff --git a/core/fxcrt/cfx_unowned_ptr.h b/core/fxcrt/cfx_unowned_ptr.h
new file mode 100644
index 0000000000..11f4e8e38b
--- /dev/null
+++ b/core/fxcrt/cfx_unowned_ptr.h
@@ -0,0 +1,64 @@
+// Copyright 2017 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CORE_FXCRT_CFX_UNOWNED_PTR_H_
+#define CORE_FXCRT_CFX_UNOWNED_PTR_H_
+
+#include <functional>
+#include <memory>
+#include <type_traits>
+#include <utility>
+
+#include "core/fxcrt/fx_memory.h"
+
+template <class T>
+class CFX_UnownedPtr {
+ public:
+  CFX_UnownedPtr() {}
+  CFX_UnownedPtr(const CFX_UnownedPtr& that) : CFX_UnownedPtr(that.Get()) {}
+
+  template <typename U>
+  explicit CFX_UnownedPtr(U* pObj) : m_pObj(pObj) {}
+
+  // Deliberately implicit to allow returning nullptrs.
+  // NOLINTNEXTLINE(runtime/explicit)
+  CFX_UnownedPtr(std::nullptr_t ptr) {}
+
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+  ~CFX_UnownedPtr() {
+    if (m_pObj)
+      reinterpret_cast<volatile uint8_t*>(m_pObj)[0];
+  }
+#endif
+
+  CFX_UnownedPtr& operator=(T* that) {
+    m_pObj = that;
+    return *this;
+  }
+  CFX_UnownedPtr& operator=(const CFX_UnownedPtr& that) {
+    if (*this != that)
+      m_pObj = that.Get();
+    return *this;
+  }
+
+  bool operator==(const CFX_UnownedPtr& that) const {
+    return Get() == that.Get();
+  }
+  bool operator==(const T* that) const { return Get() == that; }
+  bool operator!=(const CFX_UnownedPtr& that) const { return !(*this == that); }
+  bool operator!=(const T* that) const { return !(*this == that); }
+  bool operator<(const CFX_UnownedPtr& that) const {
+    return std::less<T*>()(Get(), that.Get());
+  }
+
+  T* Get() const { return m_pObj; }
+  explicit operator bool() const { return !!m_pObj; }
+  T& operator*() const { return *m_pObj; }
+  T* operator->() const { return m_pObj; }
+
+ private:
+  T* m_pObj = nullptr;
+};
+
+#endif  // CORE_FXCRT_CFX_UNOWNED_PTR_H_
diff --git a/core/fxcrt/cfx_unowned_ptr_unittest.cpp b/core/fxcrt/cfx_unowned_ptr_unittest.cpp
new file mode 100644
index 0000000000..7d29faedf5
--- /dev/null
+++ b/core/fxcrt/cfx_unowned_ptr_unittest.cpp
@@ -0,0 +1,86 @@
+// Copyright 2017 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/fxcrt/cfx_unowned_ptr.h"
+
+#include <utility>
+#include <vector>
+
+#include "testing/fx_string_testhelpers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class Clink {
+ public:
+  CFX_UnownedPtr<Clink> next_ = nullptr;
+};
+
+void DeleteDangling() {
+  Clink* ptr1 = new Clink();
+  Clink* ptr2 = new Clink();
+  ptr2->next_ = ptr1;
+  delete ptr1;
+  delete ptr2;
+}
+
+}  // namespace
+
+TEST(fxcrt, UnownedPtrOk) {
+  Clink* ptr1 = new Clink();
+  Clink* ptr2 = new Clink();
+  ptr2->next_ = ptr1;
+  delete ptr2;
+  delete ptr1;
+}
+
+TEST(fxcrt, UnownedPtrNotOk) {
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+  EXPECT_DEATH(DeleteDangling(), "");
+#else
+  DeleteDangling();
+#endif
+}
+
+TEST(fxcrt, OperatorEQ) {
+  int foo;
+  CFX_UnownedPtr<int> ptr1;
+  EXPECT_TRUE(ptr1 == ptr1);
+
+  CFX_UnownedPtr<int> ptr2;
+  EXPECT_TRUE(ptr1 == ptr2);
+
+  CFX_UnownedPtr<int> ptr3(&foo);
+  EXPECT_TRUE(ptr3 == &foo);
+  EXPECT_FALSE(ptr1 == ptr3);
+
+  ptr1 = &foo;
+  EXPECT_TRUE(ptr1 == ptr3);
+}
+
+TEST(fxcrt, OperatorNE) {
+  int foo;
+  CFX_UnownedPtr<int> ptr1;
+  EXPECT_FALSE(ptr1 != ptr1);
+
+  CFX_UnownedPtr<int> ptr2;
+  EXPECT_FALSE(ptr1 != ptr2);
+
+  CFX_UnownedPtr<int> ptr3(&foo);
+  EXPECT_FALSE(ptr3 != &foo);
+  EXPECT_TRUE(ptr1 != ptr3);
+
+  ptr1 = &foo;
+  EXPECT_FALSE(ptr1 != ptr3);
+}
+
+TEST(fxcrt, OperatorLT) {
+  int foos[2];
+  CFX_UnownedPtr<int> ptr1(&foos[0]);
+  CFX_UnownedPtr<int> ptr2(&foos[1]);
+
+  EXPECT_FALSE(ptr1 < ptr1);
+  EXPECT_TRUE(ptr1 < ptr2);
+  EXPECT_FALSE(ptr2 < ptr1);
+}
diff --git a/core/fxge/fx_font.h b/core/fxge/fx_font.h
index ce91e27c42..946c713418 100644
--- a/core/fxge/fx_font.h
+++ b/core/fxge/fx_font.h
@@ -11,6 +11,7 @@
 #include <utility>
 #include <vector>
 
+#include "core/fxcrt/cfx_unowned_ptr.h"
 #include "core/fxcrt/fx_system.h"
 #include "core/fxge/cfx_substfont.h"
 #include "core/fxge/dib/cfx_dibitmap.h"
@@ -172,7 +173,7 @@ class CFX_Font {
   void ClearFaceCache();
 
   FXFT_Face m_Face;
-  mutable CFX_FaceCache* m_FaceCache;  // not owned.
+  mutable CFX_UnownedPtr<CFX_FaceCache> m_FaceCache;
   std::unique_ptr<CFX_SubstFont> m_pSubstFont;
   std::vector<uint8_t> m_pFontDataAllocation;
   uint8_t* m_pFontData;
diff --git a/core/fxge/ge/cfx_font.cpp b/core/fxge/ge/cfx_font.cpp
index 2ae34f11b9..18baed9d09 100644
--- a/core/fxge/ge/cfx_font.cpp
+++ b/core/fxge/ge/cfx_font.cpp
@@ -537,10 +537,9 @@ int CFX_Font::GetMaxAdvanceWidth() const {
 }
 
 CFX_FaceCache* CFX_Font::GetFaceCache() const {
-  if (!m_FaceCache) {
+  if (!m_FaceCache)
     m_FaceCache = CFX_GEModule::Get()->GetFontCache()->GetCachedFace(this);
-  }
-  return m_FaceCache;
+  return m_FaceCache.Get();
 }
 
 void CFX_Font::ClearFaceCache() {