Check inputs for CPDFSDK_CustomAccess::ReadBlockAtOffset().
To be consistent with other IFX_SeekableReadStream::ReadBlockAtOffset()
implementation. Do the same checks in similar code that reads from
FPDF_FILEACCESS.
With these checks, CPDF_Parser::ParseAndAppendCrossRefSubsectionData()
will consistently fail for xref tables with 0 entries. Check for this
case and make the parser succeed. This deals with the xref table quirk
more gracefully and avoids rebuilds. Update test expectations as a
result of this new behavior.
BUG=pdfium:1197
Change-Id: Id9900161c3453c37b51d501cb8746618388214a9
Reviewed-on: https://pdfium-review.googlesource.com/c/47551
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp
index ed4eeb1..64fdd6c 100644
--- a/core/fpdfapi/parser/cpdf_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_parser.cpp
@@ -418,6 +418,9 @@
uint32_t start_objnum,
uint32_t count,
std::vector<CrossRefObjData>* out_objects) {
+ if (!count)
+ return true;
+
// Each entry shall be exactly 20 byte.
// A sample entry looks like:
// "0000000000 00007 f\r\n"
diff --git a/fpdfsdk/cpdfsdk_customaccess.cpp b/fpdfsdk/cpdfsdk_customaccess.cpp
index b679858..fcaede5 100644
--- a/fpdfsdk/cpdfsdk_customaccess.cpp
+++ b/fpdfsdk/cpdfsdk_customaccess.cpp
@@ -20,16 +20,12 @@
bool CPDFSDK_CustomAccess::ReadBlockAtOffset(void* buffer,
FX_FILESIZE offset,
size_t size) {
- if (offset < 0)
+ if (!buffer || offset < 0 || !size)
return false;
FX_SAFE_FILESIZE new_pos = pdfium::base::checked_cast<FX_FILESIZE>(size);
new_pos += offset;
- if (!new_pos.IsValid())
- return false;
- if (new_pos.ValueOrDie() > static_cast<FX_FILESIZE>(m_pFileAccess->m_FileLen))
- return false;
-
- return !!m_pFileAccess->m_GetBlock(m_pFileAccess->m_Param, offset,
- static_cast<uint8_t*>(buffer), size);
+ return new_pos.IsValid() && new_pos.ValueOrDie() <= GetSize() &&
+ m_pFileAccess->m_GetBlock(m_pFileAccess->m_Param, offset,
+ static_cast<uint8_t*>(buffer), size);
}
diff --git a/fpdfsdk/fpdf_dataavail.cpp b/fpdfsdk/fpdf_dataavail.cpp
index 2b567dd..990a968 100644
--- a/fpdfsdk/fpdf_dataavail.cpp
+++ b/fpdfsdk/fpdf_dataavail.cpp
@@ -76,8 +76,14 @@
bool ReadBlockAtOffset(void* buffer,
FX_FILESIZE offset,
size_t size) override {
- return !!m_pFileAccess->m_GetBlock(m_pFileAccess->m_Param, offset,
- static_cast<uint8_t*>(buffer), size);
+ if (!buffer || offset < 0 || !size)
+ return false;
+
+ FX_SAFE_FILESIZE new_pos = pdfium::base::checked_cast<FX_FILESIZE>(size);
+ new_pos += offset;
+ return new_pos.IsValid() && new_pos.ValueOrDie() <= GetSize() &&
+ m_pFileAccess->m_GetBlock(m_pFileAccess->m_Param, offset,
+ static_cast<uint8_t*>(buffer), size);
}
private:
diff --git a/fpdfsdk/fpdf_view_embeddertest.cpp b/fpdfsdk/fpdf_view_embeddertest.cpp
index 9de00c5..5b6be41 100644
--- a/fpdfsdk/fpdf_view_embeddertest.cpp
+++ b/fpdfsdk/fpdf_view_embeddertest.cpp
@@ -686,18 +686,16 @@
}
// Related to https://crbug.com/pdfium/1197
-// TODO(thestig): FPDF_DocumentHasValidCrossReferenceTable() should return true
-// in this test.
TEST_F(FPDFViewEmbedderTest, LoadDocumentWithEmptyXRefConsistently) {
ASSERT_TRUE(OpenDocument("empty_xref.pdf"));
- EXPECT_FALSE(FPDF_DocumentHasValidCrossReferenceTable(document()));
+ EXPECT_TRUE(FPDF_DocumentHasValidCrossReferenceTable(document()));
std::string file_path;
ASSERT_TRUE(PathService::GetTestFilePath("empty_xref.pdf", &file_path));
{
ScopedFPDFDocument doc(FPDF_LoadDocument(file_path.c_str(), ""));
ASSERT_TRUE(doc);
- EXPECT_FALSE(FPDF_DocumentHasValidCrossReferenceTable(doc.get()));
+ EXPECT_TRUE(FPDF_DocumentHasValidCrossReferenceTable(doc.get()));
}
{
size_t file_length = 0;
@@ -707,6 +705,6 @@
ScopedFPDFDocument doc(
FPDF_LoadMemDocument(file_contents.get(), file_length, ""));
ASSERT_TRUE(doc);
- EXPECT_FALSE(FPDF_DocumentHasValidCrossReferenceTable(doc.get()));
+ EXPECT_TRUE(FPDF_DocumentHasValidCrossReferenceTable(doc.get()));
}
}
diff --git a/public/fpdfview.h b/public/fpdfview.h
index 05e94f9..0ba50e2 100644
--- a/public/fpdfview.h
+++ b/public/fpdfview.h
@@ -333,6 +333,7 @@
// A function pointer for getting a block of data from a specific position.
// Position is specified by byte offset from the beginning of the file.
+ // The pointer to the buffer is never NULL and the size is never 0.
// The position and size will never go out of range of the file length.
// It may be possible for FPDFSDK to call this function multiple times for
// the same position.
diff --git a/testing/embedder_test.cpp b/testing/embedder_test.cpp
index a94b7ef..e038aeb 100644
--- a/testing/embedder_test.cpp
+++ b/testing/embedder_test.cpp
@@ -568,12 +568,15 @@
unsigned char* buf,
unsigned long size) {
std::string* new_file = static_cast<std::string*>(param);
- if (!new_file || pos + size < pos)
+ if (!new_file || pos + size < pos) {
+ NOTREACHED();
return 0;
+ }
- unsigned long file_size = new_file->size();
- if (pos + size > file_size)
+ if (pos + size > new_file->size()) {
+ NOTREACHED();
return 0;
+ }
memcpy(buf, new_file->data() + pos, size);
return 1;
diff --git a/testing/fake_file_access.cpp b/testing/fake_file_access.cpp
index 9e0e6db..290a10d 100644
--- a/testing/fake_file_access.cpp
+++ b/testing/fake_file_access.cpp
@@ -99,8 +99,12 @@
int FakeFileAccess::GetBlock(unsigned long position,
unsigned char* pBuf,
unsigned long size) {
+ if (!pBuf || !size)
+ return false;
+
if (!IsDataAvail(static_cast<size_t>(position), static_cast<size_t>(size)))
return false;
+
return file_access_->m_GetBlock(file_access_->m_Param, position, pBuf, size);
}
diff --git a/testing/test_support.cpp b/testing/test_support.cpp
index 15309ad..03d243d 100644
--- a/testing/test_support.cpp
+++ b/testing/test_support.cpp
@@ -217,8 +217,10 @@
unsigned char* pBuf,
unsigned long size) {
TestLoader* pLoader = static_cast<TestLoader*>(param);
- if (pos + size < pos || pos + size > pLoader->m_Len)
+ if (pos + size < pos || pos + size > pLoader->m_Len) {
+ NOTREACHED();
return 0;
+ }
memcpy(pBuf, pLoader->m_pBuf + pos, size);
return 1;
