Fix insertions into empty name trees.
When a name tree is empty, CPDF_NameTree::AddValueAndName() tries to
find a leaf node to insert into and fails. Instead, just insert into the
root node of the name tree. Add a unit test for this case.
This fixes FPDFDoc_AddAttachment() for the case where the EmbeddedFiles
name tree is empty. Fix the known to be broken
AddAttachmentsToFileWithNoAttachments embedder test case and do further
testing.
Bug: pdfium:1502
Change-Id: Iab189ce612070e0ef268ac6e01a1cbcce96c8d2b
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/67990
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfdoc/cpdf_nametree.cpp b/core/fpdfdoc/cpdf_nametree.cpp
index b232540..80bb5de 100644
--- a/core/fpdfdoc/cpdf_nametree.cpp
+++ b/core/fpdfdoc/cpdf_nametree.cpp
@@ -310,7 +310,7 @@
m_pRoot.Reset(pNames->GetDictFor(category));
}
-CPDF_NameTree::~CPDF_NameTree() {}
+CPDF_NameTree::~CPDF_NameTree() = default;
size_t CPDF_NameTree::GetCount() const {
return m_pRoot ? CountNamesInternal(m_pRoot.Get(), 0) : 0;
@@ -324,10 +324,18 @@
size_t nIndex = 0;
CPDF_Array* pFind = nullptr;
int nFindIndex = -1;
- // Fail if the tree already contains this name or if the tree is too deep.
- if (SearchNameNodeByName(m_pRoot.Get(), name, 0, &nIndex, &pFind,
- &nFindIndex)) {
- return false;
+ // Handle the corner case where the root node is empty. i.e. No kids and no
+ // names. In which case, just insert into it and skip all the searches.
+ CPDF_Array* pNames = m_pRoot->GetArrayFor("Names");
+ if (pNames && pNames->IsEmpty() && !m_pRoot->GetArrayFor("Kids"))
+ pFind = pNames;
+
+ if (!pFind) {
+ // Fail if the tree already contains this name or if the tree is too deep.
+ if (SearchNameNodeByName(m_pRoot.Get(), name, 0, &nIndex, &pFind,
+ &nFindIndex)) {
+ return false;
+ }
}
// If the returned |pFind| is a nullptr, then |name| is smaller than all
diff --git a/core/fpdfdoc/cpdf_nametree_unittest.cpp b/core/fpdfdoc/cpdf_nametree_unittest.cpp
index 595a7f1..ae90980 100644
--- a/core/fpdfdoc/cpdf_nametree_unittest.cpp
+++ b/core/fpdfdoc/cpdf_nametree_unittest.cpp
@@ -109,7 +109,6 @@
AddNameKeyValue(pNames, "7.txt", 777);
CPDF_NameTree nameTree(pRootDict.Get());
- pNames = nameTree.GetRootForTest()->GetArrayFor("Names");
// Insert a name that already exists in the names array.
EXPECT_FALSE(
@@ -135,6 +134,40 @@
CheckNameKeyValue(pNames, 4, "9.txt", 999);
}
+TEST(cpdf_nametree, AddIntoEmptyNames) {
+ // Set up a name tree with an empty Names array.
+ auto pRootDict = pdfium::MakeRetain<CPDF_Dictionary>();
+ CPDF_Array* pNames = pRootDict->SetNewFor<CPDF_Array>("Names");
+
+ CPDF_NameTree name_tree(pRootDict.Get());
+
+ // Insert a name should work.
+ EXPECT_TRUE(name_tree.AddValueAndName(pdfium::MakeRetain<CPDF_Number>(111),
+ L"2.txt"));
+
+ // Insert a name that already exists in the names array.
+ EXPECT_FALSE(name_tree.AddValueAndName(pdfium::MakeRetain<CPDF_Number>(111),
+ L"2.txt"));
+
+ // Insert in the beginning of the names array.
+ EXPECT_TRUE(name_tree.AddValueAndName(pdfium::MakeRetain<CPDF_Number>(111),
+ L"1.txt"));
+
+ // Insert in the middle of the names array.
+ EXPECT_TRUE(name_tree.AddValueAndName(pdfium::MakeRetain<CPDF_Number>(555),
+ L"5.txt"));
+
+ // Insert at the end of the names array.
+ EXPECT_TRUE(name_tree.AddValueAndName(pdfium::MakeRetain<CPDF_Number>(999),
+ L"9.txt"));
+
+ // Check that the names array has the expected key-value pairs.
+ CheckNameKeyValue(pNames, 0, "1.txt", 111);
+ CheckNameKeyValue(pNames, 1, "2.txt", 111);
+ CheckNameKeyValue(pNames, 2, "5.txt", 555);
+ CheckNameKeyValue(pNames, 3, "9.txt", 999);
+}
+
TEST(cpdf_nametree, AddIntoKids) {
// Set up a name tree with five nodes of three levels.
auto pRootDict = pdfium::MakeRetain<CPDF_Dictionary>();
diff --git a/fpdfsdk/fpdf_attachment_embeddertest.cpp b/fpdfsdk/fpdf_attachment_embeddertest.cpp
index 675a468..08f00dc 100644
--- a/fpdfsdk/fpdf_attachment_embeddertest.cpp
+++ b/fpdfsdk/fpdf_attachment_embeddertest.cpp
@@ -122,6 +122,7 @@
constexpr char kContents1[] = "Hello!";
EXPECT_TRUE(FPDFAttachment_SetFile(attachment, document(), kContents1,
strlen(kContents1)));
+ EXPECT_EQ(3, FPDFDoc_GetAttachmentCount(document()));
// Verify the name of the new attachment (i.e. the first attachment).
attachment = FPDFDoc_GetAttachment(document(), 0);
@@ -249,9 +250,55 @@
ScopedFPDFWideString file_name = GetFPDFWideString(L"0.txt");
FPDF_ATTACHMENT attachment =
FPDFDoc_AddAttachment(document(), file_name.get());
- // TODO(crbug.com/pdfium/1502): This should return true. Fix the bug and do
- // additional verifications here.
- EXPECT_FALSE(attachment);
+ ASSERT_TRUE(attachment);
+
+ // Set the new attachment's file.
+ constexpr char kContents1[] = "Hello!";
+ EXPECT_TRUE(FPDFAttachment_SetFile(attachment, document(), kContents1,
+ strlen(kContents1)));
+ EXPECT_EQ(1, FPDFDoc_GetAttachmentCount(document()));
+
+ // Verify the name of the new attachment (i.e. the first attachment).
+ attachment = FPDFDoc_GetAttachment(document(), 0);
+ ASSERT_TRUE(attachment);
+ unsigned long length_bytes = FPDFAttachment_GetName(attachment, nullptr, 0);
+ ASSERT_EQ(12u, length_bytes);
+ std::vector<FPDF_WCHAR> buf = GetFPDFWideStringBuffer(length_bytes);
+ EXPECT_EQ(12u, FPDFAttachment_GetName(attachment, buf.data(), length_bytes));
+ EXPECT_EQ(L"0.txt", GetPlatformWString(buf.data()));
+
+ // Verify the content of the new attachment (i.e. the first attachment).
+ length_bytes = FPDFAttachment_GetFile(attachment, nullptr, 0);
+ std::vector<char> content_buf(length_bytes);
+ ASSERT_EQ(
+ 6u, FPDFAttachment_GetFile(attachment, content_buf.data(), length_bytes));
+ EXPECT_EQ(std::string(kContents1), std::string(content_buf.data(), 6));
+
+ // Add an attachment to the end of the embedded file list and set its file.
+ file_name = GetFPDFWideString(L"z.txt");
+ attachment = FPDFDoc_AddAttachment(document(), file_name.get());
+ ASSERT_TRUE(attachment);
+ constexpr char kContents2[] = "World!";
+ EXPECT_TRUE(FPDFAttachment_SetFile(attachment, document(), kContents2,
+ strlen(kContents2)));
+ EXPECT_EQ(2, FPDFDoc_GetAttachmentCount(document()));
+
+ // Verify the name of the new attachment (i.e. the second attachment).
+ attachment = FPDFDoc_GetAttachment(document(), 1);
+ ASSERT_TRUE(attachment);
+ length_bytes = FPDFAttachment_GetName(attachment, nullptr, 0);
+ ASSERT_EQ(12u, length_bytes);
+ buf = GetFPDFWideStringBuffer(length_bytes);
+ EXPECT_EQ(12u, FPDFAttachment_GetName(attachment, buf.data(), length_bytes));
+ EXPECT_EQ(L"z.txt", GetPlatformWString(buf.data()));
+
+ // Verify the content of the new attachment (i.e. the second attachment).
+ length_bytes = FPDFAttachment_GetFile(attachment, nullptr, 0);
+ content_buf.clear();
+ content_buf.resize(length_bytes);
+ ASSERT_EQ(
+ 6u, FPDFAttachment_GetFile(attachment, content_buf.data(), length_bytes));
+ EXPECT_EQ(std::string(kContents2), std::string(content_buf.data(), 6));
}
TEST_F(FPDFAttachmentEmbedderTest, DeleteAttachment) {
