Beware of negative offset values in trailers.
Handle them as if they were omitted entirely (e.g. zero).
Bug: chromium:1324503, chromium:1324189
Change-Id: Icd3d28793d4168274b0b5a8829069afd3e2ddbea
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/93615
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp b/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
index c52deeb..90b0f6c 100644
--- a/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
+++ b/core/fpdfapi/parser/cpdf_cross_ref_avail.cpp
@@ -152,13 +152,13 @@
const int32_t xrefpos =
GetDirectInteger(trailer.Get(), kPrevCrossRefFieldKey);
- if (xrefpos &&
+ if (xrefpos > 0 &&
pdfium::base::IsValueInRangeForNumericType<FX_FILESIZE>(xrefpos))
AddCrossRefForCheck(static_cast<FX_FILESIZE>(xrefpos));
const int32_t stream_xref_offset =
GetDirectInteger(trailer.Get(), kPrevCrossRefStreamOffsetFieldKey);
- if (stream_xref_offset &&
+ if (stream_xref_offset > 0 &&
pdfium::base::IsValueInRangeForNumericType<FX_FILESIZE>(
stream_xref_offset))
AddCrossRefForCheck(static_cast<FX_FILESIZE>(stream_xref_offset));
@@ -188,7 +188,7 @@
if (trailer->GetNameFor(kTypeFieldKey) == kXRefKeyword) {
const int32_t xrefpos = trailer->GetIntegerFor(kPrevCrossRefFieldKey);
- if (xrefpos &&
+ if (xrefpos > 0 &&
pdfium::base::IsValueInRangeForNumericType<FX_FILESIZE>(xrefpos)) {
AddCrossRefForCheck(static_cast<FX_FILESIZE>(xrefpos));
}
diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp
index 611029f..f78bc8b 100644
--- a/core/fpdfapi/parser/cpdf_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_parser.cpp
@@ -360,23 +360,23 @@
if (xrefsize > 0 && xrefsize <= kMaxXRefSize)
ShrinkObjectMap(xrefsize);
- std::vector<FX_FILESIZE> xref_stream_list{
- GetDirectInteger(GetTrailer(), "XRefStm")};
+ FX_FILESIZE xref_stm = GetDirectInteger(GetTrailer(), "XRefStm");
+ std::vector<FX_FILESIZE> xref_stream_list{xref_stm};
std::vector<FX_FILESIZE> xref_list{xref_offset};
std::set<FX_FILESIZE> seen_xref_offset{xref_offset};
// When the trailer doesn't have Prev entry or Prev entry value is not
// numerical, GetDirectInteger() returns 0. Loading will end.
xref_offset = GetDirectInteger(GetTrailer(), "Prev");
- while (xref_offset) {
+ while (xref_offset > 0) {
// Check for circular references.
if (pdfium::Contains(seen_xref_offset, xref_offset))
return false;
seen_xref_offset.insert(xref_offset);
+ xref_list.insert(xref_list.begin(), xref_offset);
// SLOW ...
- xref_list.insert(xref_list.begin(), xref_offset);
LoadCrossRefV4(xref_offset, true);
RetainPtr<CPDF_Dictionary> pDict(LoadTrailerV4());
@@ -384,21 +384,20 @@
return false;
xref_offset = GetDirectInteger(pDict.Get(), "Prev");
+ xref_stm = pDict->GetIntegerFor("XRefStm");
+ xref_stream_list.insert(xref_stream_list.begin(), xref_stm);
// SLOW ...
- xref_stream_list.insert(xref_stream_list.begin(),
- pDict->GetIntegerFor("XRefStm"));
-
m_CrossRefTable = CPDF_CrossRefTable::MergeUp(
std::make_unique<CPDF_CrossRefTable>(std::move(pDict)),
std::move(m_CrossRefTable));
}
for (size_t i = 0; i < xref_list.size(); ++i) {
- if (!LoadCrossRefV4(xref_list[i], false))
+ if (xref_list[i] > 0 && !LoadCrossRefV4(xref_list[i], false))
return false;
- if (xref_stream_list[i] && !LoadCrossRefV5(&xref_stream_list[i], false))
+ if (xref_stream_list[i] > 0 && !LoadCrossRefV5(&xref_stream_list[i], false))
return false;
if (i == 0 && !VerifyCrossRefV4())
@@ -422,8 +421,8 @@
// Read /XRefStm from the first-page trailer. No need to read /Prev for the
// first-page trailer, as the caller already did that and passed it in as
// |main_xref_offset|.
- std::vector<FX_FILESIZE> xref_stream_list{
- GetDirectInteger(GetTrailer(), "XRefStm")};
+ FX_FILESIZE xref_stm = GetDirectInteger(GetTrailer(), "XRefStm");
+ std::vector<FX_FILESIZE> xref_stream_list{xref_stm};
std::vector<FX_FILESIZE> xref_list{main_xref_offset};
std::set<FX_FILESIZE> seen_xref_offset{main_xref_offset};
@@ -435,15 +434,15 @@
// Now GetTrailer() returns the merged trailer, where /Prev is from the
// main-trailer.
FX_FILESIZE xref_offset = GetDirectInteger(GetTrailer(), "Prev");
- while (xref_offset) {
+ while (xref_offset > 0) {
// Check for circular references.
if (pdfium::Contains(seen_xref_offset, xref_offset))
return false;
seen_xref_offset.insert(xref_offset);
+ xref_list.insert(xref_list.begin(), xref_offset);
// SLOW ...
- xref_list.insert(xref_list.begin(), xref_offset);
LoadCrossRefV4(xref_offset, true);
RetainPtr<CPDF_Dictionary> pDict(LoadTrailerV4());
@@ -451,24 +450,23 @@
return false;
xref_offset = GetDirectInteger(pDict.Get(), "Prev");
+ xref_stm = pDict->GetIntegerFor("XRefStm");
+ xref_stream_list.insert(xref_stream_list.begin(), xref_stm);
// SLOW ...
- xref_stream_list.insert(xref_stream_list.begin(),
- pDict->GetIntegerFor("XRefStm"));
-
m_CrossRefTable = CPDF_CrossRefTable::MergeUp(
std::make_unique<CPDF_CrossRefTable>(std::move(pDict)),
std::move(m_CrossRefTable));
}
- if (xref_stream_list[0] && !LoadCrossRefV5(&xref_stream_list[0], false))
+ if (xref_stream_list[0] > 0 && !LoadCrossRefV5(&xref_stream_list[0], false))
return false;
for (size_t i = 1; i < xref_list.size(); ++i) {
- if (!LoadCrossRefV4(xref_list[i], false))
+ if (xref_list[i] > 0 && !LoadCrossRefV4(xref_list[i], false))
return false;
- if (xref_stream_list[i] && !LoadCrossRefV5(&xref_stream_list[i], false))
+ if (xref_stream_list[i] > 0 && !LoadCrossRefV5(&xref_stream_list[i], false))
return false;
}
return true;
@@ -633,7 +631,7 @@
return false;
std::set<FX_FILESIZE> seen_xref_offset;
- while (xref_offset) {
+ while (xref_offset > 0) {
seen_xref_offset.insert(xref_offset);
if (!LoadCrossRefV5(&xref_offset, false))
return false;
@@ -730,11 +728,16 @@
return false;
const CPDF_Dictionary* pDict = pStream->GetDict();
- *pos = pDict->GetIntegerFor("Prev");
+ int32_t prev = pDict->GetIntegerFor("Prev");
+ if (prev < 0)
+ return false;
+
int32_t size = pDict->GetIntegerFor("Size");
if (size < 0)
return false;
+ *pos = prev;
+
RetainPtr<CPDF_Dictionary> pNewTrailer = ToDictionary(pDict->Clone());
if (bMainXRef) {
m_CrossRefTable =
diff --git a/fpdfsdk/fpdf_dataavail_embeddertest.cpp b/fpdfsdk/fpdf_dataavail_embeddertest.cpp
index df966b5..a5b9c47 100644
--- a/fpdfsdk/fpdf_dataavail_embeddertest.cpp
+++ b/fpdfsdk/fpdf_dataavail_embeddertest.cpp
@@ -389,3 +389,17 @@
EXPECT_EQ(PDF_DATA_NOTAVAIL,
FPDFAvail_IsPageAvail(avail(), -1, loader.hints()));
}
+
+TEST_F(FPDFDataAvailEmbedderTest, Bug_1324189) {
+ // Test passes if it doesn't crash.
+ TestAsyncLoader loader("bug_1324189.pdf");
+ CreateAvail(loader.file_avail(), loader.file_access());
+ ASSERT_EQ(PDF_DATA_NOTAVAIL, FPDFAvail_IsDocAvail(avail(), loader.hints()));
+}
+
+TEST_F(FPDFDataAvailEmbedderTest, Bug_1324503) {
+ // Test passes if it doesn't crash.
+ TestAsyncLoader loader("bug_1324503.pdf");
+ CreateAvail(loader.file_avail(), loader.file_access());
+ ASSERT_EQ(PDF_DATA_NOTAVAIL, FPDFAvail_IsDocAvail(avail(), loader.hints()));
+}
diff --git a/testing/resources/bug_1324189.in b/testing/resources/bug_1324189.in
new file mode 100644
index 0000000..45fceba
--- /dev/null
+++ b/testing/resources/bug_1324189.in
@@ -0,0 +1,16 @@
+{{header}}
+{{object 1 0}} <
+ /Type /Catalog
+>>
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
+{{xref}}
+trailer <<
+ /Root 1 0 R
+ /Prev -200
+ {{trailersize}}
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/bug_1324189.pdf b/testing/resources/bug_1324189.pdf
new file mode 100644
index 0000000..9652398
--- /dev/null
+++ b/testing/resources/bug_1324189.pdf
@@ -0,0 +1,28 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj <
+ /Type /Catalog
+>>
+xref
+0 2
+0000000000 65535 f
+0000000015 00000 n
+trailer <<
+ /Root 1 0 R
+ /Size 2
+>>
+startxref
+45
+%%EOF
+xref
+0 2
+0000000000 65535 f
+0000000015 00000 n
+trailer <<
+ /Root 1 0 R
+ /Prev -200
+ /Size 2
+>>
+startxref
+151
+%%EOF
diff --git a/testing/resources/bug_1324503.in b/testing/resources/bug_1324503.in
new file mode 100644
index 0000000..a46d142
--- /dev/null
+++ b/testing/resources/bug_1324503.in
@@ -0,0 +1,16 @@
+{{header}}
+{{object 1 0}} <
+ /Type /Catalog
+>>
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
+{{xref}}
+trailer <<
+ /Root 1 0 R
+ /XRefStm -1
+ {{trailersize}}
+>>
+{{startxref}}
+%%EOF
diff --git a/testing/resources/bug_1324503.pdf b/testing/resources/bug_1324503.pdf
new file mode 100644
index 0000000..2ce7425
--- /dev/null
+++ b/testing/resources/bug_1324503.pdf
@@ -0,0 +1,28 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj <
+ /Type /Catalog
+>>
+xref
+0 2
+0000000000 65535 f
+0000000015 00000 n
+trailer <<
+ /Root 1 0 R
+ /Size 2
+>>
+startxref
+45
+%%EOF
+xref
+0 2
+0000000000 65535 f
+0000000015 00000 n
+trailer <<
+ /Root 1 0 R
+ /XRefStm -1
+ /Size 2
+>>
+startxref
+151
+%%EOF
