Allow ProcessCrossRefV5Entry() to overwrite existing entries
Currently in CPDF_Parser::ProcessCrossRefV5Entry(), if there already
exists an entry for a given object number, ProcessCrossRefV5Entry() will
never overwrite the existing object. Remove this check so newer cross
reference V5 entries take precedence when cross references parsing
starts with the oldest entry. Keep the check to prevent overwriting when
parsing starts with the newest cross reference.
With the issue fixed, remove the suppression for the bug_1484283.pdf
test case. The only other touch up is to add an expectation image for
bug_1484283.pdf rendering in Windows GDI mode. The output is not a pixel
perfect match with the existing expectation files.
Bug: chromium:1484283
Change-Id: Ib06474c2110f50468ae57fbdc945b39431f0b907
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/112932
Reviewed-by: Nigi <nigi@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp
index 6aef205..2a3e07e 100644
--- a/core/fpdfapi/parser/cpdf_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_parser.cpp
@@ -416,12 +416,17 @@
std::move(m_CrossRefTable));
}
+ // Traverse the xref data structures from oldest to newest. So entries from
+ // later iterations should overwrite existing entries.
for (size_t i = 0; i < xref_list.size(); ++i) {
if (xref_list[i] > 0 && !LoadCrossRefV4(xref_list[i], false))
return false;
- if (xref_stream_list[i] > 0 && !LoadCrossRefV5(&xref_stream_list[i], false))
+ if (xref_stream_list[i] > 0 &&
+ !LoadCrossRefV5(&xref_stream_list[i], /*is_main_xref=*/false,
+ /*overwrite_existing=*/true)) {
return false;
+ }
if (i == 0 && !VerifyCrossRefV4())
return false;
@@ -484,15 +489,23 @@
std::move(m_CrossRefTable));
}
- if (xref_stream_list[0] > 0 && !LoadCrossRefV5(&xref_stream_list[0], false))
+ if (xref_stream_list[0] > 0 &&
+ !LoadCrossRefV5(&xref_stream_list[0], /*is_main_xref=*/false,
+ /*overwrite_existing=*/true)) {
return false;
+ }
+ // Traverse the xref data structures from oldest to newest. So entries from
+ // later iterations should overwrite existing entries.
for (size_t i = 1; i < xref_list.size(); ++i) {
if (xref_list[i] > 0 && !LoadCrossRefV4(xref_list[i], false))
return false;
- if (xref_stream_list[i] > 0 && !LoadCrossRefV5(&xref_stream_list[i], false))
+ if (xref_stream_list[i] > 0 &&
+ !LoadCrossRefV5(&xref_stream_list[i], /*is_main_xref=*/false,
+ /*overwrite_existing=*/true)) {
return false;
+ }
}
return true;
}
@@ -649,14 +662,20 @@
}
bool CPDF_Parser::LoadAllCrossRefV5(FX_FILESIZE xref_offset) {
- if (!LoadCrossRefV5(&xref_offset, true))
+ if (!LoadCrossRefV5(&xref_offset, /*is_main_xref=*/true,
+ /*overwrite_existing=*/false)) {
return false;
+ }
+ // Traverse the xref objects from newest to older. So entries from later
+ // iterations should not overwrite existing entries.
std::set<FX_FILESIZE> seen_xref_offset;
while (xref_offset > 0) {
seen_xref_offset.insert(xref_offset);
- if (!LoadCrossRefV5(&xref_offset, false))
+ if (!LoadCrossRefV5(&xref_offset, /*is_main_xref=*/false,
+ /*overwrite_existing=*/false)) {
return false;
+ }
// Check for circular references.
if (pdfium::Contains(seen_xref_offset, xref_offset))
@@ -748,7 +767,9 @@
return GetTrailer() && !m_CrossRefTable->objects_info().empty();
}
-bool CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, bool bMainXRef) {
+bool CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos,
+ bool is_main_xref,
+ bool overwrite_existing) {
RetainPtr<const CPDF_Stream> pStream =
ToStream(ParseIndirectObjectAt(*pos, 0));
if (!pStream || !pStream->GetObjNum()) {
@@ -769,7 +790,7 @@
auto new_cross_ref_table = std::make_unique<CPDF_CrossRefTable>(
/*trailer=*/ToDictionary(pDict->Clone()),
/*trailer_object_number=*/pStream->GetObjNum());
- if (bMainXRef) {
+ if (is_main_xref) {
m_CrossRefTable = std::move(new_cross_ref_table);
m_CrossRefTable->SetObjectMapSize(size);
} else {
@@ -835,7 +856,7 @@
}
ProcessCrossRefV5Entry(seg_span.subspan(i * total_width, total_width),
- field_widths, obj_num);
+ field_widths, obj_num, overwrite_existing);
}
segindex += index.obj_count;
@@ -846,7 +867,8 @@
void CPDF_Parser::ProcessCrossRefV5Entry(
pdfium::span<const uint8_t> entry_span,
pdfium::span<const uint32_t> field_widths,
- uint32_t obj_num) {
+ uint32_t obj_num,
+ bool overwrite_existing) {
DCHECK_GE(field_widths.size(), kMinFieldCount);
ObjectType type;
if (field_widths[0]) {
@@ -870,8 +892,9 @@
return;
}
- if (existing_type != ObjectType::kFree)
+ if (!overwrite_existing && existing_type != ObjectType::kFree) {
return;
+ }
if (type == ObjectType::kFree) {
m_CrossRefTable->SetFree(obj_num);
@@ -1096,7 +1119,8 @@
m_LastXRefOffset = m_pLinearized->GetLastXRefOffset();
FX_FILESIZE dwFirstXRefOffset = m_LastXRefOffset;
bool bLoadV4 = LoadCrossRefV4(dwFirstXRefOffset, false);
- if (!bLoadV4 && !LoadCrossRefV5(&dwFirstXRefOffset, true)) {
+ if (!bLoadV4 && !LoadCrossRefV5(&dwFirstXRefOffset, /*is_main_xref=*/true,
+ /*overwrite_existing=*/false)) {
if (!RebuildCrossRef())
return FORMAT_ERROR;
@@ -1162,14 +1186,20 @@
bool CPDF_Parser::LoadLinearizedAllCrossRefV5(FX_FILESIZE main_xref_offset) {
FX_FILESIZE xref_offset = main_xref_offset;
- if (!LoadCrossRefV5(&xref_offset, false))
+ if (!LoadCrossRefV5(&xref_offset, /*is_main_xref=*/false,
+ /*overwrite_existing=*/false)) {
return false;
+ }
+ // Traverse the xref objects from newest to older. So entries from later
+ // iterations should not overwrite existing entries.
std::set<FX_FILESIZE> seen_xref_offset;
while (xref_offset) {
seen_xref_offset.insert(xref_offset);
- if (!LoadCrossRefV5(&xref_offset, false))
+ if (!LoadCrossRefV5(&xref_offset, /*is_main_xref=*/false,
+ /*overwrite_existing=*/false)) {
return false;
+ }
// Check for circular references.
if (pdfium::Contains(seen_xref_offset, xref_offset))
diff --git a/core/fpdfapi/parser/cpdf_parser.h b/core/fpdfapi/parser/cpdf_parser.h
index ea8409a..abda2c5 100644
--- a/core/fpdfapi/parser/cpdf_parser.h
+++ b/core/fpdfapi/parser/cpdf_parser.h
@@ -148,10 +148,13 @@
bool LoadAllCrossRefV4(FX_FILESIZE xref_offset);
bool LoadAllCrossRefV5(FX_FILESIZE xref_offset);
- bool LoadCrossRefV5(FX_FILESIZE* pos, bool bMainXRef);
+ bool LoadCrossRefV5(FX_FILESIZE* pos,
+ bool is_main_xref,
+ bool overwrite_existing);
void ProcessCrossRefV5Entry(pdfium::span<const uint8_t> entry_span,
pdfium::span<const uint32_t> field_widths,
- uint32_t obj_num);
+ uint32_t obj_num,
+ bool overwrite_existing);
RetainPtr<CPDF_Dictionary> LoadTrailerV4();
Error SetEncryptHandler();
void ReleaseEncryptHandler();
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS
index 28c98ff..96c84d12 100644
--- a/testing/SUPPRESSIONS
+++ b/testing/SUPPRESSIONS
@@ -639,9 +639,6 @@
# TODO(pdfium:1457): Remove after associated bug is fixed
bug_1457.in * * * *
-# TODO(chromium:1484283): Remove after associated bug is fixed
-bug_1484283.pdf * * * *
-
# TODO(pdfium:1519): Remove after associated bug is fixed
bug_1519.in * * * *
diff --git a/testing/resources/pixel/bug_1484283_expected_gdi.pdf.0.png b/testing/resources/pixel/bug_1484283_expected_gdi.pdf.0.png
new file mode 100644
index 0000000..18a1d04
--- /dev/null
+++ b/testing/resources/pixel/bug_1484283_expected_gdi.pdf.0.png
Binary files differ
