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.

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  <ejb@ql.org>
+
+	* Add an extra check to the library to detect when foreign objects
+	are inserted directly (instead of using
+	<function>QPDF::copyForeignObject</function>) 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  <ejb@ql.org>
 
 	* 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<QPDFObjectHandle> 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<QPDF_Array*>(obj.getPointer())->setItem(n, item);
     }
     else
@@ -878,6 +879,10 @@ QPDFObjectHandle::setArrayFromVector(std::vector<QPDFObjectHandle> const& items)
 {
     if (isArray())
     {
+        for (auto const& item: items)
+        {
+            checkOwnership(item);
+        }
         dynamic_cast<QPDF_Array*>(obj.getPointer())->setFromVector(items);
     }
     else
@@ -906,6 +911,7 @@ QPDFObjectHandle::appendItem(QPDFObjectHandle const& item)
 {
     if (isArray())
     {
+        checkOwnership(item);
         dynamic_cast<QPDF_Array*>(obj.getPointer())->appendItem(item);
     }
     else
@@ -1283,6 +1289,7 @@ QPDFObjectHandle::replaceKey(std::string const& key,
 {
     if (isDictionary())
     {
+        checkOwnership(value);
         dynamic_cast<QPDF_Dictionary*>(
             obj.getPointer())->replaceKey(key, value);
     }
@@ -1313,6 +1320,7 @@ QPDFObjectHandle::replaceOrRemoveKey(std::string const& key,
 {
     if (isDictionary())
     {
+        checkOwnership(value);
         dynamic_cast<QPDF_Dictionary*>(
             obj.getPointer())->replaceOrRemoveKey(key, value);
     }
@@ -3270,6 +3278,20 @@ QPDFObjectHandle::isImage(bool exclude_imagemask)
 }
 
 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()
 {
     if (! isPageObject())
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.
          </para>
         </listitem>
+        <listitem>
+         <para>
+          Add an extra check to the library to detect when foreign
+          objects are inserted directly (instead of using
+          <function>QPDF::copyForeignObject</function>) 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.
+         </para>
+        </listitem>
        </itemizedlist>
       </listitem>
       <listitem>
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<PointerHolder<QPDF>>& 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<PointerHolder<QPDF> > page_heap;
     std::map<std::string, QPDF*> page_spec_qpdfs;
     std::map<std::string, ClosedFileInputSource*> 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<PointerHolder<QPDF>> 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)
     {