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 <key, nullptr> 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 <dsinclair@chromium.org>
Reviewed-by: dsinclair <dsinclair@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp
index bf8b51e..f3c23f3 100644
--- a/core/fpdfapi/parser/cpdf_array.cpp
+++ b/core/fpdfapi/parser/cpdf_array.cpp
@@ -57,8 +57,11 @@
pVisited->insert(this);
auto pCopy = pdfium::MakeUnique<CPDF_Array>();
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<const CPDF_Object*> 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 653ef45..d4e4080 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -68,8 +68,9 @@
auto pCopy = pdfium::MakeUnique<CPDF_Dictionary>(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<const CPDF_Object*> 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 9285993..b25d40a 100644
--- a/core/fpdfapi/parser/cpdf_object_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp
@@ -761,7 +761,7 @@
std::unique_ptr<CPDF_Array> 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 @@
std::unique_ptr<CPDF_Dictionary> 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 @@
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 0972316..db39700 100644
--- a/fpdfsdk/fpdfppo_embeddertest.cpp
+++ b/fpdfsdk/fpdfppo_embeddertest.cpp
@@ -15,6 +15,12 @@
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, 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 @@
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 0000000..f791607
--- /dev/null
+++ b/testing/resources/circular_viewer_ref.in
@@ -0,0 +1,24 @@
+{{header}}
+{{object 1 0}}
+<</Names 1 0 R
+ /ViewerPreferences<<
+ /Names 1 0 R
+ /ViewerPreferences<<
+ /Names 1 0 R
+ /ViewerPreferences<<>>
+ >>>>
+>>
+endobj
+
+{{object 2 0}}
+<</Names[(0) 7 0 R]>>
+endobj
+
+{{object 7 0}}
+<</JS(this.print\({bUI:true,bSilent:false,bShrinkToFit:true}\);)/S/JavaScript>>
+endobj
+
+{{xref}}
+trailer <</Root 1 0 R>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/circular_viewer_ref.pdf b/testing/resources/circular_viewer_ref.pdf
new file mode 100644
index 0000000..2e5c4dd
--- /dev/null
+++ b/testing/resources/circular_viewer_ref.pdf
@@ -0,0 +1,35 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj
+<</Names 1 0 R
+ /ViewerPreferences<<
+ /Names 1 0 R
+ /ViewerPreferences<<
+ /Names 1 0 R
+ /ViewerPreferences<<>>
+ >>>>
+>>
+endobj
+
+2 0 obj
+<</Names[(0) 7 0 R]>>
+endobj
+
+7 0 obj
+<</JS(this.print\({bUI:true,bSilent:false,bShrinkToFit:true}\);)/S/JavaScript>>
+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 <</Root 1 0 R>>
+startxref
+291
+%%EOF
diff --git a/testing/resources/repeat_viewer_ref.in b/testing/resources/repeat_viewer_ref.in
new file mode 100644
index 0000000..6b436e5
--- /dev/null
+++ b/testing/resources/repeat_viewer_ref.in
@@ -0,0 +1,19 @@
+{{header}}
+{{object 1 0}}
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<>>
+endobj
+
+{{object 2 0}}
+<</Names[(0) 7 0 R]>>
+endobj
+
+{{object 7 0}}
+<</JS(this.print\({bUI:true,bSilent:false,bShrinkToFit:true}\);)/S/JavaScript>>
+endobj
+
+{{xref}}
+trailer <</Root 1 0 R>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/repeat_viewer_ref.pdf b/testing/resources/repeat_viewer_ref.pdf
new file mode 100644
index 0000000..3f4ee47
--- /dev/null
+++ b/testing/resources/repeat_viewer_ref.pdf
@@ -0,0 +1,30 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<
+<</Names<</JavaScript 2 0 R>>/ViewerPreferences<<>>
+endobj
+
+2 0 obj
+<</Names[(0) 7 0 R]>>
+endobj
+
+7 0 obj
+<</JS(this.print\({bUI:true,bSilent:false,bShrinkToFit:true}\);)/S/JavaScript>>
+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 <</Root 1 0 R>>
+startxref
+317
+%%EOF