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
diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp
index 9e60aaa..e425cfc 100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -516,18 +516,18 @@
}
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 @@
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<int>(index + i);
- }
- }
+ CPDF_Reference* pKid = ToReference(pKidList->GetObjectAt(i));
+ if (pKid && pKid->GetRefObjNum() == objnum)
+ return static_cast<int>(*index + i);
}
}
@@ -585,8 +582,15 @@
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<int>(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 6545548..3848ad6 100644
--- a/core/fpdfapi/parser/cpdf_document.h
+++ b/core/fpdfapi/parser/cpdf_document.h
@@ -106,9 +106,9 @@
// 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<CPDF_Object> ParseIndirectObject(uint32_t objnum) override;
void LoadDocInternal();
diff --git a/fpdfsdk/fpdfdoc_embeddertest.cpp b/fpdfsdk/fpdfdoc_embeddertest.cpp
index 5441bb3..3666687 100644
--- a/fpdfsdk/fpdfdoc_embeddertest.cpp
+++ b/fpdfsdk/fpdfdoc_embeddertest.cpp
@@ -68,6 +68,16 @@
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<unsigned long>(-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 0000000..c21df24
--- /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 <</F1 15 0 R>>
+ >>
+ /Contents [21 0 R]
+ /MediaBox [0 0 612 792]
+>>
+endobj
+% Page number 1.
+{{object 4 0}} <<
+ /Type /Page
+ /Parent 2 0 R
+ /Resources <<
+ /Font <</F1 15 0 R>>
+ >>
+ /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) <</Fail [10 /FitH]>>
+ (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 <</D [999 0 R /XYZ 0 0 -200]>>
+>>
+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 0000000..fb01c57
--- /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 <</F1 15 0 R>>
+ >>
+ /Contents [21 0 R]
+ /MediaBox [0 0 612 792]
+>>
+endobj
+% Page number 1.
+4 0 obj <<
+ /Type /Page
+ /Parent 2 0 R
+ /Resources <<
+ /Font <</F1 15 0 R>>
+ >>
+ /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) <</Fail [10 /FitH]>>
+ (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 <</D [999 0 R /XYZ 0 0 -200]>>
+>>
+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
