From 3fe931553d7c602f4f89644d5973000cf9f01be3 Mon Sep 17 00:00:00 2001
From: Lei Zhang <thestig@chromium.org>
Date: Thu, 23 Apr 2026 09:56:15 -0700
Subject: [PATCH] Fix page/pages dictionary confusion in CPDF_Document::TraversePDFPages()

TraversePDFPages() checks for the lack of /Kids in a dictionary and
assumes that dictionary is a page dictionary. Instead of blindly making
this assumption, call GetNodeType() and reject the dictionary if it is a
pages dictionary.

With this check in place, update testing/resources/redirect.pdf to have
a valid page so the associate embedder test passes. For this test, the
primary purpose is to test PDF redirection, not PDF parsing.

For CXFASimpleParserEmbedderTest.Bug216, update the test expectation, as
the purpose of this test is to make sure the input does not cause a
crash, not to make sure whether the PDF loads successfully or not.

For DocumentTest.PagesWithoutKids, the existing test expectations
matched the existing incorrect TraversePDFPages() behavior. Update it to
match the new behavior and explain why that is correct.

Bug: 504516117
Change-Id: I76722d0d1aff21b167af85a0c72a226380bec17c
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/146770
Reviewed-by: Andy Phan <andyphan@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
---

diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp
index 01fa811..68d5fa8 100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -269,6 +269,9 @@
     if (*nPagesToGo != 1) {
       return nullptr;
     }
+    if (GetNodeType(pPages) == NodeType::kBranch) {
+      return nullptr;
+    }
     page_list_[iPage] = pPages->GetObjNum();
     return pPages;
   }
diff --git a/core/fpdfapi/parser/cpdf_document_unittest.cpp b/core/fpdfapi/parser/cpdf_document_unittest.cpp
index 27a5a16..64d7fc1 100644
--- a/core/fpdfapi/parser/cpdf_document_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_document_unittest.cpp
@@ -299,11 +299,10 @@
 TEST_F(DocumentTest, PagesWithoutKids) {
   // Set up a document with Pages dict without kids, and Count = 3
   auto doc = std::make_unique<CPDF_TestDocPagesWithoutKids>();
-  EXPECT_TRUE(doc->GetPageDictionary(0));
-  // Test GetPage does not fetch pages out of range
-  for (int i = 1; i < 5; i++) {
+  // Since there are no Page nodes in the Page Tree, GetPageDictionary() will
+  // never return non-null.
+  for (int i = 0; i < 5; i++) {
     EXPECT_FALSE(doc->GetPageDictionary(i));
   }
-
-  EXPECT_TRUE(doc->GetPageDictionary(0));
+  EXPECT_FALSE(doc->GetPageDictionary(0));
 }
diff --git a/testing/resources/redirect.in b/testing/resources/redirect.in
new file mode 100644
index 0000000..efa5e76
--- /dev/null
+++ b/testing/resources/redirect.in
@@ -0,0 +1,29 @@
+{{header}}
+{{object 1 0}} <<
+  /Type /Catalog
+  /Pages 2 0 R
+  /OpenAction 4 0 R
+>>
+endobj
+{{object 2 0}} <<
+  /Type /Pages
+  /MediaBox [0 0 200 200]
+  /Count 1
+  /Kids [3 0 R]
+>>
+endobj
+{{object 3 0}} <<
+  /Type /Page
+  /Parent 2 0 R
+>>
+endobj
+{{object 4 0}} <<
+  /Type /Action
+  /S /URI
+  /URI (http://evilzone.org)
+>>
+endobj
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
diff --git a/testing/resources/redirect.pdf b/testing/resources/redirect.pdf
index 517bd5a..cbc3f01 100644
--- a/testing/resources/redirect.pdf
+++ b/testing/resources/redirect.pdf
@@ -1,22 +1,40 @@
-%PDF-1.7
-trailer
-<<
-/Root 1 0 R
->>
-1 0 obj
-<<
-/Type /Catalog
-/Pages 2 0 R
-/OpenAction 2 0 R
->>
-endobj
-
-2 0 obj
-<<
-/Type /Action
-/S /URI
-/URI (http://evilzone.org) // URL HERE
->>
-endobj
-
-%%EOF
+%PDF-1.7
+%���
+1 0 obj <<
+  /Type /Catalog
+  /Pages 2 0 R
+  /OpenAction 4 0 R
+>>
+endobj
+2 0 obj <<
+  /Type /Pages
+  /MediaBox [0 0 200 200]
+  /Count 1
+  /Kids [3 0 R]
+>>
+endobj
+3 0 obj <<
+  /Type /Page
+  /Parent 2 0 R
+>>
+endobj
+4 0 obj <<
+  /Type /Action
+  /S /URI
+  /URI (http://evilzone.org)
+>>
+endobj
+xref
+0 5
+0000000000 65535 f 
+0000000015 00000 n 
+0000000088 00000 n 
+0000000177 00000 n 
+0000000228 00000 n 
+trailer <<
+  /Root 1 0 R
+  /Size 5
+>>
+startxref
+304
+%%EOF
diff --git a/xfa/fxfa/parser/cxfa_document_builder_embeddertest.cpp b/xfa/fxfa/parser/cxfa_document_builder_embeddertest.cpp
index becb519..b02a3d5 100644
--- a/xfa/fxfa/parser/cxfa_document_builder_embeddertest.cpp
+++ b/xfa/fxfa/parser/cxfa_document_builder_embeddertest.cpp
@@ -5,12 +5,13 @@
 #include "testing/embedder_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-class CXFASimpleParserEmbedderTest : public EmbedderTest {};
+using CXFASimpleParserEmbedderTest = EmbedderTest;
 
+// Should not crash.
 TEST_F(CXFASimpleParserEmbedderTest, Bug216) {
   ASSERT_TRUE(OpenDocument("bug_216.pdf"));
   ScopedPage page = LoadScopedPage(0);
-  EXPECT_TRUE(page);
+  EXPECT_FALSE(page);
 }
 
 TEST_F(CXFASimpleParserEmbedderTest, Bug709793) {

diff --git a/core/fpdfapi/parser/cpdf_document_unittest.cpp b/core/fpdfapi/parser/cpdf_document_unittest.cpp
index 27a5a16..64d7fc1 100644
--- a/core/fpdfapi/parser/cpdf_document_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_document_unittest.cpp
@@ -299,11 +299,10 @@
 TEST_F(DocumentTest, PagesWithoutKids) {
   // Set up a document with Pages dict without kids, and Count = 3
   auto doc = std::make_unique<CPDF_TestDocPagesWithoutKids>();
-  EXPECT_TRUE(doc->GetPageDictionary(0));
-  // Test GetPage does not fetch pages out of range
-  for (int i = 1; i < 5; i++) {
+  // Since there are no Page nodes in the Page Tree, GetPageDictionary() will
+  // never return non-null.
+  for (int i = 0; i < 5; i++) {
     EXPECT_FALSE(doc->GetPageDictionary(i));
   }
-
-  EXPECT_TRUE(doc->GetPageDictionary(0));
+  EXPECT_FALSE(doc->GetPageDictionary(0));
 }