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

@@ -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

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;
 

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);
     }
@@ -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()
 {

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>

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;
@@ -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())
         {

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

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

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)
     {