From e507dc5004184ae3f8fd1cd19b723b4be69a46da Mon Sep 17 00:00:00 2001 From: tsepez Date: Wed, 18 Jan 2017 10:24:35 -0800 Subject: Bad indexing in CPDF_Document::FindPageIndex when page tree corrupt. Moving to std::vector from the more forgiving CFX_ArrayTemplate revealed the dubious page tree traversal, which depends on the correctness of the /Count entries to properly summarize the total descendants under a given node. The only "correct" thing to do is to throw away these counts as parsed, and re-compute them, perhaps in CountPages(). But I'm not willing to do that since it may break unknown documents in the wild. Pass out-params as pointers while we're at it. BUG=680376 Review-Url: https://codereview.chromium.org/2636403003 --- core/fpdfapi/parser/cpdf_document.cpp | 38 +++++---- core/fpdfapi/parser/cpdf_document.h | 4 +- fpdfsdk/fpdfdoc_embeddertest.cpp | 10 +++ testing/resources/bug_680376.in | 130 ++++++++++++++++++++++++++++ testing/resources/bug_680376.pdf | 156 ++++++++++++++++++++++++++++++++++ 5 files changed, 319 insertions(+), 19 deletions(-) create mode 100644 testing/resources/bug_680376.in create mode 100644 testing/resources/bug_680376.pdf diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp index 9e60aaa882..e425cfc328 100644 --- a/core/fpdfapi/parser/cpdf_document.cpp +++ b/core/fpdfapi/parser/cpdf_document.cpp @@ -516,18 +516,18 @@ void CPDF_Document::SetPageObjNum(int iPage, uint32_t objNum) { } int CPDF_Document::FindPageIndex(CPDF_Dictionary* pNode, - uint32_t& skip_count, + uint32_t* skip_count, uint32_t objnum, - int& index, + int* index, int level) { if (!pNode->KeyExist("Kids")) { if (objnum == pNode->GetObjNum()) - return index; + return *index; - if (skip_count) - skip_count--; + if (*skip_count) + (*skip_count)--; - index++; + (*index)++; return -1; } @@ -539,20 +539,17 @@ int CPDF_Document::FindPageIndex(CPDF_Dictionary* pNode, return -1; size_t count = pNode->GetIntegerFor("Count"); - if (count <= skip_count) { - skip_count -= count; - index += count; + if (count <= *skip_count) { + (*skip_count) -= count; + (*index) += count; return -1; } if (count && count == pKidList->GetCount()) { for (size_t i = 0; i < count; i++) { - if (CPDF_Reference* pKid = ToReference(pKidList->GetObjectAt(i))) { - if (pKid->GetRefObjNum() == objnum) { - m_PageList[index + i] = objnum; - return static_cast(index + i); - } - } + CPDF_Reference* pKid = ToReference(pKidList->GetObjectAt(i)); + if (pKid && pKid->GetRefObjNum() == objnum) + return static_cast(*index + i); } } @@ -585,8 +582,15 @@ int CPDF_Document::GetPageIndex(uint32_t objnum) { if (!pPages) return -1; - int index = 0; - return FindPageIndex(pPages, skip_count, objnum, index); + int start_index = 0; + int found_index = FindPageIndex(pPages, &skip_count, objnum, &start_index); + + // Corrupt page tree may yield out-of-range results. + if (found_index < 0 || found_index >= pdfium::CollectionSize(m_PageList)) + return -1; + + m_PageList[found_index] = objnum; + return found_index; } int CPDF_Document::GetPageCount() const { diff --git a/core/fpdfapi/parser/cpdf_document.h b/core/fpdfapi/parser/cpdf_document.h index 65455480a4..3848ad6220 100644 --- a/core/fpdfapi/parser/cpdf_document.h +++ b/core/fpdfapi/parser/cpdf_document.h @@ -106,9 +106,9 @@ class CPDF_Document : public CPDF_IndirectObjectHolder { // When this method is called, m_pTreeTraversal[level] exists. CPDF_Dictionary* TraversePDFPages(int iPage, int* nPagesToGo, size_t level); int FindPageIndex(CPDF_Dictionary* pNode, - uint32_t& skip_count, + uint32_t* skip_count, uint32_t objnum, - int& index, + int* index, int level = 0); std::unique_ptr ParseIndirectObject(uint32_t objnum) override; void LoadDocInternal(); diff --git a/fpdfsdk/fpdfdoc_embeddertest.cpp b/fpdfsdk/fpdfdoc_embeddertest.cpp index 5441bb3ecf..3666687d4d 100644 --- a/fpdfsdk/fpdfdoc_embeddertest.cpp +++ b/fpdfsdk/fpdfdoc_embeddertest.cpp @@ -68,6 +68,16 @@ TEST_F(FPDFDocEmbeddertest, DestGetLocationInPage) { EXPECT_EQ(1, zoom); } +TEST_F(FPDFDocEmbeddertest, BUG_680376) { + EXPECT_TRUE(OpenDocument("bug_680376.pdf")); + + // Page number directly in item from Dests NameTree. + FPDF_DEST dest = FPDF_GetNamedDestByName(document(), "First"); + EXPECT_TRUE(dest); + EXPECT_EQ(static_cast(-1), + FPDFDest_GetPageIndex(document(), dest)); +} + TEST_F(FPDFDocEmbeddertest, ActionGetFilePath) { EXPECT_TRUE(OpenDocument("launch_action.pdf")); diff --git a/testing/resources/bug_680376.in b/testing/resources/bug_680376.in new file mode 100644 index 0000000000..c21df245fb --- /dev/null +++ b/testing/resources/bug_680376.in @@ -0,0 +1,130 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /Pages 2 0 R + /Names << + /Dests 10 0 R + >> + /Dests 14 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 4 + /Kids [ + 5 0 R + 6 0 R + ] +>> +endobj +% Page number 0. +{{object 3 0}} << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /Contents [21 0 R] + /MediaBox [0 0 612 792] +>> +endobj +% Page number 1. +{{object 4 0}} << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /Contents [22 0 R] + /MediaBox [0 0 612 792] +>> +endobj +% Tree node with bad Count, duplicated kids. +{{object 5 0}} << + /Type /Pages + /Parent 2 0 R + /Count 2 + /Kids [ + 3 0 R + 3 0 R + 3 0 R + 3 0 R + ] +>> +endobj +% tree node with actual kids +{{object 6 0}} << + /Type /Pages + /Count 2 + /Kids [ + 3 0 R + 4 0 R + ] +>> +% Root of Dests NameTree +{{object 10 0}} << + /Kids [ + 11 0 R + 12 0 R + ] +>> +endobj +% Left child for Dests NameTree +{{object 11 0}} << + /Names [ + (First) [4 0 R] + ] +>> +endobj +% Right child for Dests NameTree +{{object 12 0}} << + /Names [ + (WrongKey) <> + (WrongType) /NameNotAllowedHere + ] +>> +endobj +% Old-style top-level Dests dictionary. Note that FirstAlternate +% intentionally references non-exisstant page 11 and LastAlternate +% intentionally references non-existant object 999. +{{object 14 0}} << + /FirstAlternate [11 /XYZ 200 400 800] + /LastAlternate <> +>> +endobj +% Font resource. +{{object 15 0}} << + /Type /Font + /Subtype /Type1 + /BaseFont /Arial +>> +endobj +% Content for page 0. +{{object 21 0}} << + /Length 0 +>> +stream +BT +/F1 20 Tf +100 600 TD (Page1)Tj +ET +endstream +endobj +% Content for page 1. +{{object 22 0}} << + /Length 0 +>> +stream +BT +/F1 20 Tf +100 600 TD (Page2)Tj +ET +endstream +endobj +{{xref}} +trailer << + /Size 6 + /Root 1 0 R +>> +{{startxref}} +%%EOF diff --git a/testing/resources/bug_680376.pdf b/testing/resources/bug_680376.pdf new file mode 100644 index 0000000000..fb01c57613 --- /dev/null +++ b/testing/resources/bug_680376.pdf @@ -0,0 +1,156 @@ +%PDF-1.7 +% ò¤ô +1 0 obj << + /Type /Catalog + /Pages 2 0 R + /Names << + /Dests 10 0 R + >> + /Dests 14 0 R +>> +endobj +2 0 obj << + /Type /Pages + /Count 4 + /Kids [ + 5 0 R + 6 0 R + ] +>> +endobj +% Page number 0. +3 0 obj << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /Contents [21 0 R] + /MediaBox [0 0 612 792] +>> +endobj +% Page number 1. +4 0 obj << + /Type /Page + /Parent 2 0 R + /Resources << + /Font <> + >> + /Contents [22 0 R] + /MediaBox [0 0 612 792] +>> +endobj +% Tree node with bad Count, duplicated kids. +5 0 obj << + /Type /Pages + /Parent 2 0 R + /Count 2 + /Kids [ + 3 0 R + 3 0 R + 3 0 R + 3 0 R + ] +>> +endobj +% tree node with actual kids +6 0 obj << + /Type /Pages + /Count 2 + /Kids [ + 3 0 R + 4 0 R + ] +>> +% Root of Dests NameTree +10 0 obj << + /Kids [ + 11 0 R + 12 0 R + ] +>> +endobj +% Left child for Dests NameTree +11 0 obj << + /Names [ + (First) [4 0 R] + ] +>> +endobj +% Right child for Dests NameTree +12 0 obj << + /Names [ + (WrongKey) <> + (WrongType) /NameNotAllowedHere + ] +>> +endobj +% Old-style top-level Dests dictionary. Note that FirstAlternate +% intentionally references non-exisstant page 11 and LastAlternate +% intentionally references non-existant object 999. +14 0 obj << + /FirstAlternate [11 /XYZ 200 400 800] + /LastAlternate <> +>> +endobj +% Font resource. +15 0 obj << + /Type /Font + /Subtype /Type1 + /BaseFont /Arial +>> +endobj +% Content for page 0. +21 0 obj << + /Length 0 +>> +stream +BT +/F1 20 Tf +100 600 TD (Page1)Tj +ET +endstream +endobj +% Content for page 1. +22 0 obj << + /Length 0 +>> +stream +BT +/F1 20 Tf +100 600 TD (Page2)Tj +ET +endstream +endobj +xref +0 23 +0000000000 65535 f +0000000015 00000 n +0000000119 00000 n +0000000217 00000 n +0000000378 00000 n +0000000568 00000 n +0000000714 00000 n +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000813 00000 n +0000000903 00000 n +0000000993 00000 n +0000000000 65535 f +0000001287 00000 n +0000001415 00000 n +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000000000 65535 f +0000001510 00000 n +0000001620 00000 n +trailer << + /Size 6 + /Root 1 0 R +>> +startxref +1708 +%%EOF -- cgit v1.2.3