Fix crash when ColorSpace references itself directly.

Also fixes any problems with cycles between colorspaces.

Past fixes have solved problems with CPDF_DocPageData::GetColorSpace()
calling itself and CPDF_DocPageData::GetColorSpace() calling
CPDF_ColorSpace::Load() and vice versa. They have not solved
CPDF_ColorSpace::Load() calling itself.

This CL repurposes the |pVisited| set to ensure CPDF_ColorSpace::Load()
does not try to load a colorspace as a dependency of itself and
creates |pVisitedLocal| to ensure CPDF_DocPageData::GetColorSpace()
does not create a similar circular dependency not involving
CPDF_ColorSpace::Load().

Bug: chromium:828206
Change-Id: Ib2d0ec494be169135607f3651e0f70627b26ebd7
Reviewed-on: https://pdfium-review.googlesource.com/29810
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Henrique Nakashima <hnakashima@chromium.org>

6 files changed, 55 insertions, 7 deletions

diff --git a/BUILD.gn b/BUILD.gn
index 36ee859264..a51e3f4609 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -2980,6 +2980,7 @@ test("pdfium_unittests") {
 test("pdfium_embeddertests") {
   sources = [
     "core/fpdfapi/edit/cpdf_creator_embeddertest.cpp",
+    "core/fpdfapi/page/cpdf_docpagedata_embeddertest.cpp",
     "core/fpdfapi/page/cpdf_stitchfunc_embeddertest.cpp",
     "core/fpdfapi/parser/cpdf_parser_embeddertest.cpp",
     "core/fpdfapi/parser/cpdf_security_handler_embeddertest.cpp",
diff --git a/core/fpdfapi/page/cpdf_colorspace.cpp b/core/fpdfapi/page/cpdf_colorspace.cpp
index e1dd598a63..528c6ea3cf 100644
--- a/core/fpdfapi/page/cpdf_colorspace.cpp
+++ b/core/fpdfapi/page/cpdf_colorspace.cpp
@@ -400,6 +400,11 @@ std::unique_ptr<CPDF_ColorSpace> CPDF_ColorSpace::Load(
   if (!pObj)
     return nullptr;
 
+  if (pdfium::ContainsKey(*pVisited, pObj))
+    return nullptr;
+
+  pdfium::ScopedSetInsertion<CPDF_Object*> insertion(pVisited, pObj);
+
   if (pObj->IsName()) {
     return std::unique_ptr<CPDF_ColorSpace>(
         ColorspaceFromName(pObj->GetString()));
diff --git a/core/fpdfapi/page/cpdf_docpagedata.cpp b/core/fpdfapi/page/cpdf_docpagedata.cpp
index 0d2a9189b6..03db315912 100644
--- a/core/fpdfapi/page/cpdf_docpagedata.cpp
+++ b/core/fpdfapi/page/cpdf_docpagedata.cpp
@@ -219,13 +219,22 @@ CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceGuarded(
     CPDF_Object* pCSObj,
     const CPDF_Dictionary* pResources,
     std::set<CPDF_Object*>* pVisited) {
+  std::set<CPDF_Object*> visitedLocal;
+  return GetColorSpaceInternal(pCSObj, pResources, pVisited, &visitedLocal);
+}
+
+CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceInternal(
+    CPDF_Object* pCSObj,
+    const CPDF_Dictionary* pResources,
+    std::set<CPDF_Object*>* pVisited,
+    std::set<CPDF_Object*>* pVisitedInternal) {
   if (!pCSObj)
     return nullptr;
 
-  if (pdfium::ContainsKey(*pVisited, pCSObj))
+  if (pdfium::ContainsKey(*pVisitedInternal, pCSObj))
     return nullptr;
 
-  pdfium::ScopedSetInsertion<CPDF_Object*> insertion(pVisited, pCSObj);
+  pdfium::ScopedSetInsertion<CPDF_Object*> insertion(pVisitedInternal, pCSObj);
 
   if (pCSObj->IsName()) {
     ByteString name = pCSObj->GetString();
@@ -233,8 +242,8 @@ CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceGuarded(
     if (!pCS && pResources) {
       CPDF_Dictionary* pList = pResources->GetDictFor("ColorSpace");
       if (pList) {
-        return GetColorSpaceGuarded(pList->GetDirectObjectFor(name), nullptr,
-                                    pVisited);
+        return GetColorSpaceInternal(pList->GetDirectObjectFor(name), nullptr,
+                                     pVisited, pVisitedInternal);
       }
     }
     if (!pCS || !pResources)
@@ -259,7 +268,8 @@ CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceGuarded(
     if (!pDefaultCS)
       return pCS;
 
-    return GetColorSpaceGuarded(pDefaultCS, nullptr, pVisited);
+    return GetColorSpaceInternal(pDefaultCS, nullptr, pVisited,
+                                 pVisitedInternal);
   }
 
   CPDF_Array* pArray = pCSObj->AsArray();
@@ -267,8 +277,8 @@ CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceGuarded(
     return nullptr;
 
   if (pArray->GetCount() == 1) {
-    return GetColorSpaceGuarded(pArray->GetDirectObjectAt(0), pResources,
-                                pVisited);
+    return GetColorSpaceInternal(pArray->GetDirectObjectAt(0), pResources,
+                                 pVisited, pVisitedInternal);
   }
 
   CPDF_CountedColorSpace* csData = nullptr;
diff --git a/core/fpdfapi/page/cpdf_docpagedata.h b/core/fpdfapi/page/cpdf_docpagedata.h
index 02107aa2bb..5508f568be 100644
--- a/core/fpdfapi/page/cpdf_docpagedata.h
+++ b/core/fpdfapi/page/cpdf_docpagedata.h
@@ -39,8 +39,13 @@ class CPDF_DocPageData {
                              CPDF_FontEncoding* pEncoding);
   void ReleaseFont(const CPDF_Dictionary* pFontDict);
 
+  // Loads a colorspace.
   CPDF_ColorSpace* GetColorSpace(CPDF_Object* pCSObj,
                                  const CPDF_Dictionary* pResources);
+
+  // Loads a colorspace in a context that might be while loading another
+  // colorspace. |pVisited| is passed recursively to avoid circular calls
+  // involving CPDF_ColorSpace::Load().
   CPDF_ColorSpace* GetColorSpaceGuarded(CPDF_Object* pCSObj,
                                         const CPDF_Dictionary* pResources,
                                         std::set<CPDF_Object*>* pVisited);
@@ -68,6 +73,17 @@ class CPDF_DocPageData {
  private:
   using CPDF_CountedFont = CPDF_CountedObject<CPDF_Font>;
 
+  // Loads a colorspace in a context that might be while loading another
+  // colorspace, or even in a recursive call from this method itself. |pVisited|
+  // is passed recursively to avoid circular calls involving
+  // CPDF_ColorSpace::Load() and |pVisitedInternal| is also passed recursively
+  // to avoid circular calls with this method calling itself.
+  CPDF_ColorSpace* GetColorSpaceInternal(
+      CPDF_Object* pCSObj,
+      const CPDF_Dictionary* pResources,
+      std::set<CPDF_Object*>* pVisited,
+      std::set<CPDF_Object*>* pVisitedInternal);
+
   bool m_bForceClear;
   UnownedPtr<CPDF_Document> const m_pPDFDoc;
   std::map<ByteString, CPDF_Stream*> m_HashProfileMap;
diff --git a/core/fpdfapi/page/cpdf_docpagedata_embeddertest.cpp b/core/fpdfapi/page/cpdf_docpagedata_embeddertest.cpp
new file mode 100644
index 0000000000..5120b21c56
--- /dev/null
+++ b/core/fpdfapi/page/cpdf_docpagedata_embeddertest.cpp
@@ -0,0 +1,16 @@
+// Copyright 2018 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 "testing/embedder_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class CPDF_DocPageDataEmbeddertest : public EmbedderTest {};
+
+TEST_F(CPDF_DocPageDataEmbeddertest, Crasher_828206) {
+  EXPECT_TRUE(OpenDocument("bug_828206.pdf"));
+  FPDF_PAGE page = LoadPage(0);
+  ASSERT_TRUE(page);
+  RenderLoadedPage(page);
+  UnloadPage(page);
+}
diff --git a/testing/resources/bug_828206.pdf b/testing/resources/bug_828206.pdf
new file mode 100644
index 0000000000..7015f812bd
--- /dev/null
+++ b/testing/resources/bug_828206.pdf