From 6bdd824188bc9a2e6b24b5752a3170ce10185c1d Mon Sep 17 00:00:00 2001 From: Wei Li Date: Thu, 23 Mar 2017 09:45:04 -0700 Subject: Fix two CloneNonCycle issues CloneNonCycle() tries to detect cyclic object references without copying them. There are two issues: -- for elements in an array or a dictionary, they should be able to refer to the same object, which are not cyclic; -- for cyclic referenced elements in an array or a dictionary, do not clone the element at all. Having nullptr or as an element, like we did before, might cause crash when the element being accessed. BUG=chromium:701860 Change-Id: Id0304accde76ed06fa5ce640994c7628359600fb Reviewed-on: https://pdfium-review.googlesource.com/3156 Commit-Queue: dsinclair Reviewed-by: dsinclair --- core/fpdfapi/parser/cpdf_array.cpp | 7 +++-- core/fpdfapi/parser/cpdf_dictionary.cpp | 5 ++-- core/fpdfapi/parser/cpdf_object_unittest.cpp | 6 ++--- fpdfsdk/fpdfppo_embeddertest.cpp | 40 ++++++++++++++++++++++++++-- testing/resources/circular_viewer_ref.in | 24 +++++++++++++++++ testing/resources/circular_viewer_ref.pdf | 35 ++++++++++++++++++++++++ testing/resources/repeat_viewer_ref.in | 19 +++++++++++++ testing/resources/repeat_viewer_ref.pdf | 30 +++++++++++++++++++++ 8 files changed, 157 insertions(+), 9 deletions(-) create mode 100644 testing/resources/circular_viewer_ref.in create mode 100644 testing/resources/circular_viewer_ref.pdf create mode 100644 testing/resources/repeat_viewer_ref.in create mode 100644 testing/resources/repeat_viewer_ref.pdf diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp index bf8b51e604..f3c23f37be 100644 --- a/core/fpdfapi/parser/cpdf_array.cpp +++ b/core/fpdfapi/parser/cpdf_array.cpp @@ -57,8 +57,11 @@ std::unique_ptr CPDF_Array::CloneNonCyclic( pVisited->insert(this); auto pCopy = pdfium::MakeUnique(); for (const auto& pValue : m_Objects) { - if (!pdfium::ContainsKey(*pVisited, pValue.get())) - pCopy->m_Objects.push_back(pValue->CloneNonCyclic(bDirect, pVisited)); + if (!pdfium::ContainsKey(*pVisited, pValue.get())) { + std::set visited(*pVisited); + if (auto obj = pValue->CloneNonCyclic(bDirect, &visited)) + pCopy->m_Objects.push_back(std::move(obj)); + } } return std::move(pCopy); } diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp index 653ef45067..d4e4080d31 100644 --- a/core/fpdfapi/parser/cpdf_dictionary.cpp +++ b/core/fpdfapi/parser/cpdf_dictionary.cpp @@ -68,8 +68,9 @@ std::unique_ptr CPDF_Dictionary::CloneNonCyclic( auto pCopy = pdfium::MakeUnique(m_pPool); for (const auto& it : *this) { if (!pdfium::ContainsKey(*pVisited, it.second.get())) { - pCopy->m_Map.insert(std::make_pair( - it.first, it.second->CloneNonCyclic(bDirect, pVisited))); + std::set visited(*pVisited); + if (auto obj = it.second->CloneNonCyclic(bDirect, &visited)) + pCopy->m_Map.insert(std::make_pair(it.first, std::move(obj))); } } return std::move(pCopy); diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp index 9285993c52..b25d40a029 100644 --- a/core/fpdfapi/parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp @@ -761,7 +761,7 @@ TEST(PDFArrayTest, CloneDirectObject) { std::unique_ptr cloned_array = ToArray(std::move(cloned_array_object)); - ASSERT_EQ(1U, cloned_array->GetCount()); + ASSERT_EQ(0U, cloned_array->GetCount()); CPDF_Object* cloned_obj = cloned_array->GetObjectAt(0); EXPECT_FALSE(cloned_obj); } @@ -795,7 +795,7 @@ TEST(PDFDictionaryTest, CloneDirectObject) { std::unique_ptr cloned_dict = ToDictionary(std::move(cloned_dict_object)); - ASSERT_EQ(1U, cloned_dict->GetCount()); + ASSERT_EQ(0U, cloned_dict->GetCount()); CPDF_Object* cloned_obj = cloned_dict->GetObjectFor("foo"); EXPECT_FALSE(cloned_obj); } @@ -859,7 +859,7 @@ TEST(PDFObjectTest, CloneCheckLoop) { CPDF_Object* cloned_arr = cloned_dict->GetObjectFor("arr"); ASSERT_TRUE(cloned_arr); ASSERT_TRUE(cloned_arr->IsArray()); - EXPECT_EQ(1u, cloned_arr->AsArray()->GetCount()); + EXPECT_EQ(0U, cloned_arr->AsArray()->GetCount()); // Recursively referenced object is not cloned. EXPECT_EQ(nullptr, cloned_arr->AsArray()->GetObjectAt(0)); } diff --git a/fpdfsdk/fpdfppo_embeddertest.cpp b/fpdfsdk/fpdfppo_embeddertest.cpp index 0972316534..db39700ca1 100644 --- a/fpdfsdk/fpdfppo_embeddertest.cpp +++ b/fpdfsdk/fpdfppo_embeddertest.cpp @@ -15,6 +15,12 @@ namespace { class FPDFPPOEmbeddertest : public EmbedderTest {}; +int FakeBlockWriter(FPDF_FILEWRITE* pThis, + const void* pData, + unsigned long size) { + return size; +} + } // namespace TEST_F(FPDFPPOEmbeddertest, NoViewerPreferences) { @@ -36,13 +42,13 @@ TEST_F(FPDFPPOEmbeddertest, ViewerPreferences) { } TEST_F(FPDFPPOEmbeddertest, ImportPages) { - EXPECT_TRUE(OpenDocument("viewer_ref.pdf")); + ASSERT_TRUE(OpenDocument("viewer_ref.pdf")); FPDF_PAGE page = LoadPage(0); EXPECT_TRUE(page); FPDF_DOCUMENT output_doc = FPDF_CreateNewDocument(); - EXPECT_TRUE(output_doc); + ASSERT_TRUE(output_doc); EXPECT_TRUE(FPDF_CopyViewerPreferences(output_doc, document())); EXPECT_TRUE(FPDF_ImportPages(output_doc, document(), "1", 0)); EXPECT_EQ(1, FPDF_GetPageCount(output_doc)); @@ -51,6 +57,36 @@ TEST_F(FPDFPPOEmbeddertest, ImportPages) { UnloadPage(page); } +TEST_F(FPDFPPOEmbeddertest, BadRepeatViewerPref) { + ASSERT_TRUE(OpenDocument("repeat_viewer_ref.pdf")); + + FPDF_DOCUMENT output_doc = FPDF_CreateNewDocument(); + EXPECT_TRUE(output_doc); + EXPECT_TRUE(FPDF_CopyViewerPreferences(output_doc, document())); + + FPDF_FILEWRITE writer; + writer.version = 1; + writer.WriteBlock = FakeBlockWriter; + + EXPECT_TRUE(FPDF_SaveAsCopy(output_doc, &writer, 0)); + FPDF_CloseDocument(output_doc); +} + +TEST_F(FPDFPPOEmbeddertest, BadCircularViewerPref) { + ASSERT_TRUE(OpenDocument("circular_viewer_ref.pdf")); + + FPDF_DOCUMENT output_doc = FPDF_CreateNewDocument(); + EXPECT_TRUE(output_doc); + EXPECT_TRUE(FPDF_CopyViewerPreferences(output_doc, document())); + + FPDF_FILEWRITE writer; + writer.version = 1; + writer.WriteBlock = FakeBlockWriter; + + EXPECT_TRUE(FPDF_SaveAsCopy(output_doc, &writer, 0)); + FPDF_CloseDocument(output_doc); +} + TEST_F(FPDFPPOEmbeddertest, BadRanges) { EXPECT_TRUE(OpenDocument("viewer_ref.pdf")); diff --git a/testing/resources/circular_viewer_ref.in b/testing/resources/circular_viewer_ref.in new file mode 100644 index 0000000000..f791607e91 --- /dev/null +++ b/testing/resources/circular_viewer_ref.in @@ -0,0 +1,24 @@ +{{header}} +{{object 1 0}} +<> + >>>> +>> +endobj + +{{object 2 0}} +<> +endobj + +{{object 7 0}} +<> +endobj + +{{xref}} +trailer <> +{{startxref}} +%%EOF diff --git a/testing/resources/circular_viewer_ref.pdf b/testing/resources/circular_viewer_ref.pdf new file mode 100644 index 0000000000..2e5c4dd8ab --- /dev/null +++ b/testing/resources/circular_viewer_ref.pdf @@ -0,0 +1,35 @@ +%PDF-1.7 +% ò¤ô +1 0 obj +<> + >>>> +>> +endobj + +2 0 obj +<> +endobj + +7 0 obj +<> +endobj + +xref +0 8 +0000000000 65535 f +0000000015 00000 n +0000000157 00000 n +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000195 00000 n +trailer <> +startxref +291 +%%EOF diff --git a/testing/resources/repeat_viewer_ref.in b/testing/resources/repeat_viewer_ref.in new file mode 100644 index 0000000000..6b436e535f --- /dev/null +++ b/testing/resources/repeat_viewer_ref.in @@ -0,0 +1,19 @@ +{{header}} +{{object 1 0}} +<>/ViewerPreferences<< +<>/ViewerPreferences<< +<>/ViewerPreferences<<>> +endobj + +{{object 2 0}} +<> +endobj + +{{object 7 0}} +<> +endobj + +{{xref}} +trailer <> +{{startxref}} +%%EOF diff --git a/testing/resources/repeat_viewer_ref.pdf b/testing/resources/repeat_viewer_ref.pdf new file mode 100644 index 0000000000..3f4ee47724 --- /dev/null +++ b/testing/resources/repeat_viewer_ref.pdf @@ -0,0 +1,30 @@ +%PDF-1.7 +% ò¤ô +1 0 obj +<>/ViewerPreferences<< +<>/ViewerPreferences<< +<>/ViewerPreferences<<>> +endobj + +2 0 obj +<> +endobj + +7 0 obj +<> +endobj + +xref +0 8 +0000000000 65535 f +0000000015 00000 n +0000000183 00000 n +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000221 00000 n +trailer <> +startxref +317 +%%EOF -- cgit v1.2.3