From 9b28933647f0a3ed535dd488c4526b75d1c10fc6 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Thu, 4 Nov 2021 11:55:36 -0400 Subject: [PATCH] Check object ownership when adding When adding a QPDFObjectHandle to an array or dictionary, if possible, check if the new object belongs to the same QPDF. This makes it much easier to find incorrect code than waiting for the situation to be detected when the file is written. --- ChangeLog | 8 ++++++++ include/qpdf/QPDFObjectHandle.hh | 1 + libqpdf/QPDFObjectHandle.cc | 22 ++++++++++++++++++++++ manual/qpdf-manual.xml | 10 ++++++++++ qpdf/qpdf.cc | 10 ++++++---- qpdf/qpdf.testcov | 1 + qpdf/qtest/qpdf/foreign-in-write.out | 1 + qpdf/test_driver.cc | 12 ++++++++++++ 8 files changed, 61 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 24bbc569..91301ae9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2021-11-04 Jay Berkenbilt + + * Add an extra check to the library to detect when foreign objects + are inserted directly (instead of using + QPDF::copyForeignObject) at the time of + insertion rather than when the file is written. Catching the error + sooner makes it much easier to locate the incorrect code. + 2021-11-03 Jay Berkenbilt * Bug fix: make overlay/underlay work on a page with no resource diff --git a/include/qpdf/QPDFObjectHandle.hh b/include/qpdf/QPDFObjectHandle.hh index e57179b5..2f2b3ebc 100644 --- a/include/qpdf/QPDFObjectHandle.hh +++ b/include/qpdf/QPDFObjectHandle.hh @@ -1336,6 +1336,7 @@ class QPDFObjectHandle std::vector arrayOrStreamToStreamArray( std::string const& description, std::string& all_description); static void warn(QPDF*, QPDFExc const&); + void checkOwnership(QPDFObjectHandle const&) const; bool initialized; diff --git a/libqpdf/QPDFObjectHandle.cc b/libqpdf/QPDFObjectHandle.cc index 27740f09..af862bd4 100644 --- a/libqpdf/QPDFObjectHandle.cc +++ b/libqpdf/QPDFObjectHandle.cc @@ -864,6 +864,7 @@ QPDFObjectHandle::setArrayItem(int n, QPDFObjectHandle const& item) { if (isArray()) { + checkOwnership(item); dynamic_cast(obj.getPointer())->setItem(n, item); } else @@ -878,6 +879,10 @@ QPDFObjectHandle::setArrayFromVector(std::vector const& items) { if (isArray()) { + for (auto const& item: items) + { + checkOwnership(item); + } dynamic_cast(obj.getPointer())->setFromVector(items); } else @@ -906,6 +911,7 @@ QPDFObjectHandle::appendItem(QPDFObjectHandle const& item) { if (isArray()) { + checkOwnership(item); dynamic_cast(obj.getPointer())->appendItem(item); } else @@ -1283,6 +1289,7 @@ QPDFObjectHandle::replaceKey(std::string const& key, { if (isDictionary()) { + checkOwnership(value); dynamic_cast( obj.getPointer())->replaceKey(key, value); } @@ -1313,6 +1320,7 @@ QPDFObjectHandle::replaceOrRemoveKey(std::string const& key, { if (isDictionary()) { + checkOwnership(value); dynamic_cast( obj.getPointer())->replaceOrRemoveKey(key, value); } @@ -3269,6 +3277,20 @@ QPDFObjectHandle::isImage(bool exclude_imagemask) dict.getKey("/ImageMask").getBoolValue())))); } +void +QPDFObjectHandle::checkOwnership(QPDFObjectHandle const& item) const +{ + if ((this->qpdf != nullptr) && + (item.qpdf != nullptr) && + (this->qpdf != item.qpdf)) + { + QTC::TC("qpdf", "QPDFObjectHandle check ownership"); + throw std::logic_error( + "Attempting to add an object from a different QPDF." + " Use QPDF::copyForeignObject to add objects from another file."); + } +} + void QPDFObjectHandle::assertPageObject() { diff --git a/manual/qpdf-manual.xml b/manual/qpdf-manual.xml index 7cd22459..026d6039 100644 --- a/manual/qpdf-manual.xml +++ b/manual/qpdf-manual.xml @@ -5104,6 +5104,16 @@ print "\n"; receive warnings on certain recoverable conditions. + + + Add an extra check to the library to detect when foreign + objects are inserted directly (instead of using + QPDF::copyForeignObject) at the time of + insertion rather than when the file is written. Catching the + error sooner makes it much easier to locate the incorrect + code. + + diff --git a/qpdf/qpdf.cc b/qpdf/qpdf.cc index e1cd62da..9438886e 100644 --- a/qpdf/qpdf.cc +++ b/qpdf/qpdf.cc @@ -5705,7 +5705,9 @@ static QPDFObjectHandle added_page(QPDF& pdf, QPDFPageObjectHelper page) return added_page(pdf, page.getObjectHandle()); } -static void handle_page_specs(QPDF& pdf, Options& o, bool& warnings) +static void handle_page_specs( + QPDF& pdf, Options& o, bool& warnings, + std::vector>& page_heap) { // Parse all page specifications and translate them into lists of // actual pages. @@ -5757,7 +5759,6 @@ static void handle_page_specs(QPDF& pdf, Options& o, bool& warnings) } // Create a QPDF object for each file that we may take pages from. - std::vector > page_heap; std::map page_spec_qpdfs; std::map page_spec_cfis; page_spec_qpdfs[o.infilename] = &pdf; @@ -6009,7 +6010,7 @@ static void handle_page_specs(QPDF& pdf, Options& o, bool& warnings) pdf.warn( QPDFExc(qpdf_e_damaged_pdf, pdf.getFilename(), "", 0, "Exception caught while fixing copied" - " annotations. This may be a qpdf bug." + + " annotations. This may be a qpdf bug. " + std::string("Exception: ") + e.what())); } } @@ -6649,9 +6650,10 @@ int realmain(int argc, char* argv[]) } } bool other_warnings = false; + std::vector> page_heap; if (! o.page_specs.empty()) { - handle_page_specs(pdf, o, other_warnings); + handle_page_specs(pdf, o, other_warnings, page_heap); } if (! o.rotations.empty()) { diff --git a/qpdf/qpdf.testcov b/qpdf/qpdf.testcov index 68a32d43..8f888209 100644 --- a/qpdf/qpdf.testcov +++ b/qpdf/qpdf.testcov @@ -597,3 +597,4 @@ QPDFWriter exclude from object stream 0 check unclosed --pages 1 QPDF_pages findPage not found 0 qpdf overlay page with no resources 0 +QPDFObjectHandle check ownership 0 diff --git a/qpdf/qtest/qpdf/foreign-in-write.out b/qpdf/qtest/qpdf/foreign-in-write.out index 14293aa9..a829577d 100644 --- a/qpdf/qtest/qpdf/foreign-in-write.out +++ b/qpdf/qtest/qpdf/foreign-in-write.out @@ -1,2 +1,3 @@ logic error: QPDFObjectHandle from different QPDF found while writing. Use QPDF::copyForeignObject to add objects from another file. +logic error: Attempting to add an object from a different QPDF. Use QPDF::copyForeignObject to add objects from another file. test 29 done diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc index 9a21b82c..1d995bb7 100644 --- a/qpdf/test_driver.cc +++ b/qpdf/test_driver.cc @@ -1274,6 +1274,18 @@ void runtest(int n, char const* filename1, char const* arg2) { std::cout << "logic error: " << e.what() << std::endl; } + + // Detect adding a foreign object + auto root1 = pdf.getRoot(); + auto root2 = other.getRoot(); + try + { + root1.replaceKey("/Oops", root2); + } + catch (std::logic_error const& e) + { + std::cout << "logic error: " << e.what() << std::endl; + } } else if (n == 30) {