From b92ec18fdccd196035e02f3232c0b730637ac815 Mon Sep 17 00:00:00 2001 From: Henrique Nakashima Date: Wed, 11 Apr 2018 21:55:49 +0000 Subject: 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 Commit-Queue: Henrique Nakashima --- BUILD.gn | 1 + core/fpdfapi/page/cpdf_colorspace.cpp | 5 +++++ core/fpdfapi/page/cpdf_docpagedata.cpp | 24 +++++++++++++++------ core/fpdfapi/page/cpdf_docpagedata.h | 16 ++++++++++++++ .../fpdfapi/page/cpdf_docpagedata_embeddertest.cpp | 16 ++++++++++++++ testing/resources/bug_828206.pdf | Bin 0 -> 238 bytes 6 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 core/fpdfapi/page/cpdf_docpagedata_embeddertest.cpp create mode 100644 testing/resources/bug_828206.pdf 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::Load( if (!pObj) return nullptr; + if (pdfium::ContainsKey(*pVisited, pObj)) + return nullptr; + + pdfium::ScopedSetInsertion insertion(pVisited, pObj); + if (pObj->IsName()) { return std::unique_ptr( 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* pVisited) { + std::set visitedLocal; + return GetColorSpaceInternal(pCSObj, pResources, pVisited, &visitedLocal); +} + +CPDF_ColorSpace* CPDF_DocPageData::GetColorSpaceInternal( + CPDF_Object* pCSObj, + const CPDF_Dictionary* pResources, + std::set* pVisited, + std::set* pVisitedInternal) { if (!pCSObj) return nullptr; - if (pdfium::ContainsKey(*pVisited, pCSObj)) + if (pdfium::ContainsKey(*pVisitedInternal, pCSObj)) return nullptr; - pdfium::ScopedSetInsertion insertion(pVisited, pCSObj); + pdfium::ScopedSetInsertion 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* pVisited); @@ -68,6 +73,17 @@ class CPDF_DocPageData { private: using CPDF_CountedFont = CPDF_CountedObject; + // 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* pVisited, + std::set* pVisitedInternal); + bool m_bForceClear; UnownedPtr const m_pPDFDoc; std::map 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 Binary files /dev/null and b/testing/resources/bug_828206.pdf differ -- cgit v1.2.3