Create a special "destroyed" type rather than using null

When a QPDF is destroyed, changing indirect objects to direct nulls makes them effectively disappear silently when they sneak into other places. Instead, we should treat this as an error. Adding a destroyed object type makes this possible.

Create a special "destroyed" type rather than using null
When a QPDF is destroyed, changing indirect objects to direct nulls makes them effectively disappear silently when they sneak into other places. Instead, we should treat this as an error. Adding a destroyed object type makes this possible.
Jay Berkenbilt
1 parent 264e25f3
Showing 16 changed files with 164 additions and 47 deletions
ChangeLog
TODO
include/qpdf/Constants.h
include/qpdf/QPDFObjectHandle.hh
libqpdf/CMakeLists.txt
libqpdf/QPDF.cc
libqpdf/QPDFObjectHandle.cc
libqpdf/QPDFValueProxy.cc
libqpdf/QPDF_Destroyed.cc
libqpdf/QPDF_Reserved.cc
libqpdf/QPDF_Unresolved.cc
libqpdf/qpdf/QPDFValueProxy.hh
libqpdf/qpdf/QPDF_Destroyed.hh
manual/release-notes.rst
qpdf/qtest/qpdf/foreign-in-write.out
qpdf/test_driver.cc
+2022-09-08  Jay Berkenbilt  <ejb@ql.org>
+
+        * Add QPDFObjectHandle::isDestroyed() to test whether an indirect
+        object was from a QPDF that has been destroyed.
+
 2022-09-07  Jay Berkenbilt  <ejb@ql.org>
  
         * Add QPDFObjectHandle::getQPDF(), which returns a reference, as
@@ -812,9 +812,11 @@ Rejected Ideas
   QPDFObjectHandle because of indirect objects. This only pertains to
   direct objects, which are always "resolved" in QPDFObjectHandle.
  
-  If this is addressed, read comments in QPDFWriter.cc::enqueueObject
-  near the call to getOwningQPDF, comments in QPDFValueProxy::reset,
-  and comments in QPDF::~QPDF() near the line that assigns to null.
-  This will also affect test 92 in test_driver.cc. All these
-  references were from the release of qpdf 11 (in case they have moved
-  by such time as this might be resurrected).
+  If this is addressed, read comments in the following places:
+  * QPDFWriter.cc::enqueueObject near the call to getOwningQPDF
+  * QPDFValueProxy::reset and QPDFValueProxy::destroy
+  * QPDF::~QPDF()
+  * test 92 in test_driver.cc
+  * QPDFObjectHandle.hh near isDestroyed
+  All these references were from the release of qpdf 11 (in case they
+  have moved by such time as this might be resurrected).
@@ -123,6 +123,7 @@ enum qpdf_object_type_e {
     ot_inlineimage,
     /* Object types internal to qpdf */
     ot_unresolved,
+    ot_destroyed,
 };
  
 /* Write Parameters. See QPDFWriter.hh for details. */
@@ -391,6 +391,11 @@ class QPDFObjectHandle
     QPDF_DLL
     inline bool isIndirect() const;
  
+    // This returns true for indirect objects from a QPDF that has
+    // been destroyed.
+    QPDF_DLL
+    bool isDestroyed();
+
     // True for everything except array, dictionary, stream, word, and
     // inline image.
     QPDF_DLL
@@ -90,6 +90,7 @@ set(libqpdf_SOURCES
   QPDFXRefEntry.cc
   QPDF_Array.cc
   QPDF_Bool.cc
+  QPDF_Destroyed.cc
   QPDF_Dictionary.cc
   QPDF_InlineImage.cc
   QPDF_Integer.cc
@@ -249,22 +249,23 @@ QPDF::~QPDF()
     // std::shared_ptr objects will prevent the objects from being
     // deleted. Walk through all objects in the object cache, which is
     // those objects that we read from the file, and break all
-    // resolved indirect references by replacing them with direct null
-    // objects. At this point, obviously no one is still using the
-    // QPDF object, but we'll explicitly clear the xref table anyway
-    // just to prevent any possibility of resolve() succeeding. Note
+    // resolved indirect references by replacing them with an internal
+    // object type representing that they have been destroyed. Note
     // that we can't break references like this at any time when the
-    // QPDF object is active. This also causes all QPDFObjectHandle
-    // objects that are reachable from this object to become nulls and
+    // QPDF object is active. The call to reset also causes all
+    // QPDFObjectHandle objects that are reachable from this object to
     // release their association with this QPDF.
+
+    // At this point, obviously no one is still using the QPDF object,
+    // but we'll explicitly clear the xref table anyway just to
+    // prevent any possibility of resolve() succeeding.
     this->m->xref_table.clear();
     auto null_obj = QPDF_Null::create();
     for (auto const& iter: this->m->obj_cache) {
         iter.second.object->reset();
-        // If the issue discussed in QPDFValueProxy::reset were
-        // resolved, then this assignment to null_obj could be
-        // removed.
-        iter.second.object->assign(null_obj);
+        // It would be better if reset() could call destroy(), but it
+        // can't -- see comments in QPDFValueProxy::reset().
+        iter.second.object->destroy();
     }
 }
  
@@ -252,8 +252,10 @@ void
 QPDFObjectHandle::reset()
 {
     // Recursively remove association with any QPDF object. This
-    // method may only be called during final destruction. See
-    // comments in QPDF::~QPDF().
+    // method may only be called during final destruction.
+    // QPDF::~QPDF() calls it for indirect objects using the object
+    // pointer itself, so we don't do that here. Other objects call it
+    // through this method.
     if (!isIndirect()) {
         this->obj->reset();
     }
@@ -352,6 +354,12 @@ QPDFObjectHandle::asString()
 }
  
 bool
+QPDFObjectHandle::isDestroyed()
+{
+    return dereference() && (obj->getTypeCode() == ::ot_destroyed);
+}
+
+bool
 QPDFObjectHandle::isBool()
 {
     return dereference() && (obj->getTypeCode() == ::ot_boolean);
 #include <qpdf/QPDFValueProxy.hh>
  
 #include <qpdf/QPDF.hh>
+#include <qpdf/QPDF_Destroyed.hh>
  
 void
 QPDFValueProxy::doResolve()
@@ -8,3 +9,10 @@ QPDFValueProxy::doResolve()
     auto og = value->og;
     QPDF::Resolver::resolve(value->qpdf, og);
 }
+
+void
+QPDFValueProxy::destroy()
+{
+    // See comments in reset() for why this isn't part of reset.
+    value = QPDF_Destroyed::getInstance();
+}
+#include <qpdf/QPDF_Destroyed.hh>
+
+#include <stdexcept>
+
+QPDF_Destroyed::QPDF_Destroyed() :
+    QPDFValue(::ot_destroyed, "destroyed")
+{
+}
+
+std::shared_ptr<QPDFValue>
+QPDF_Destroyed::getInstance()
+{
+    static std::shared_ptr<QPDFValue> instance(new QPDF_Destroyed());
+    return instance;
+}
+
+std::shared_ptr<QPDFValueProxy>
+QPDF_Destroyed::shallowCopy()
+{
+    throw std::logic_error(
+        "attempted to shallow copy QPDFObjectHandle from destroyed QPDF");
+    return nullptr;
+}
+
+std::string
+QPDF_Destroyed::unparse()
+{
+    throw std::logic_error(
+        "attempted to unparse a QPDFObjectHandle from a destroyed QPDF");
+    return "";
+}
+
+JSON
+QPDF_Destroyed::getJSON(int json_version)
+{
+    throw std::logic_error(
+        "attempted to get JSON from a QPDFObjectHandle from a destroyed QPDF");
+    return JSON::makeNull();
+}
@@ -31,6 +31,6 @@ JSON
 QPDF_Reserved::getJSON(int json_version)
 {
     throw std::logic_error(
-        "QPDFObjectHandle: attempting to unparse a reserved object");
+        "QPDFObjectHandle: attempting to get JSON from a reserved object");
     return JSON::makeNull();
 }
@@ -17,7 +17,7 @@ std::shared_ptr&lt;QPDFValueProxy&gt;
 QPDF_Unresolved::shallowCopy()
 {
     throw std::logic_error(
-        "attempted to shallow copy unresolved QPDFObjectHandle");
+        "attempted to shallow copy an unresolved QPDFObjectHandle");
     return nullptr;
 }
  
@@ -32,5 +32,7 @@ QPDF_Unresolved::unparse()
 JSON
 QPDF_Unresolved::getJSON(int json_version)
 {
+    throw std::logic_error(
+        "attempted to get JSON from an unresolved QPDFObjectHandle");
     return JSON::makeNull();
 }
@@ -114,21 +114,23 @@ class QPDFValueProxy
     {
         value->reset();
         // It would be better if, rather than clearing value->qpdf and
-        // value->og, we completely replaced value with a null object.
-        // However, at the time of the release of qpdf 11, this causes
-        // test failures and would likely break a lot of code since it
-        // possible for a direct object that recursively contains no
-        // indirect objects to be copied into multiple QPDF objects.
-        // For that reason, we have to break the association with the
-        // owning QPDF but not otherwise mutate the object. For
-        // indirect objects, QPDF::~QPDF replaces the object with
-        // null, which clears circular references. If this code were
-        // able to do the null replacement, that code would not have
-        // to.
+        // value->og, we completely replaced value with
+        // QPDF_Destroyed. However, at the time of the release of qpdf
+        // 11, this causes test failures and would likely break a lot
+        // of code since it possible for a direct object that
+        // recursively contains no indirect objects to be copied into
+        // multiple QPDF objects. For that reason, we have to break
+        // the association with the owning QPDF but not otherwise
+        // mutate the object. For indirect objects, QPDF::~QPDF
+        // replaces indirect objects with QPDF_Destroyed, which clears
+        // circular references. If this code were able to do that,
+        // that code would not have to.
         value->qpdf = nullptr;
         value->og = QPDFObjGen();
     }
  
+    void destroy();
+
     bool
     isUnresolved() const
     {
+#ifndef QPDF_DESTROYED_HH
+#define QPDF_DESTROYED_HH
+
+#include <qpdf/QPDFValue.hh>
+
+class QPDF_Destroyed: public QPDFValue
+{
+  public:
+    virtual ~QPDF_Destroyed() = default;
+    virtual std::shared_ptr<QPDFValueProxy> shallowCopy();
+    virtual std::string unparse();
+    virtual JSON getJSON(int json_version);
+    static std::shared_ptr<QPDFValue> getInstance();
+
+  private:
+    QPDF_Destroyed();
+};
+
+#endif // QPDF_DESTROYED_HH
@@ -202,11 +202,13 @@ For a detailed list of changes, please see the file
       ``replaceKeyAndGetOld``, a ``null`` object if the object was not
       previously there.
  
-    - The method ``QPDFObjectHandle::getOwningQPDF`` now returns a
-      null pointer (rather than an invalid pointer) if the owning
-      ``QPDF`` object has been destroyed. This situation should
-      generally not happen for correct code, but at least the
-      situation is detectible now.
+    - It is now possible to detect when a ``QPDFObjectHandle`` is an
+      indirect object that belongs to a ``QPDF`` that has been
+      destroyed. Any attempt to unparse this type of
+      ``QPDFObjectHandle`` will throw a logic error. You can detect
+      this by calling the new ``QPDFObjectHandle::isDestroyed``
+      method. Also the ``QPDFObjectHandle::getOwningQPDF`` method now
+      returns a null pointer rather than an invalid pointer.
  
     - The method ``QPDFObjectHandle::getQPDF`` returns a ``QPDF&``
       (rather than a ``QPDF*``) and is an alternative to
 logic error: QPDFObjectHandle from different QPDF found while writing.  Use QPDF::copyForeignObject to add objects from another file.
+logic error: attempted to unparse a QPDFObjectHandle from a destroyed QPDF
 logic error: Attempting to add an object from a different QPDF. Use QPDF::copyForeignObject to add objects from another file.
 test 29 done
@@ -1135,8 +1135,8 @@ test_29(QPDF&amp; pdf, char const* arg2)
 {
     // Detect mixed objects in QPDFWriter
     assert(arg2 != 0);
-    QPDF other;
-    other.processFile(arg2);
+    auto other = QPDF::create();
+    other->processFile(arg2);
     // We need to create a QPDF with mixed ownership to exercise
     // QPDFWriter's ownership check. To do this, we have to sneak the
     // foreign object inside an ownerless direct object to avoid
@@ -1146,10 +1146,25 @@ test_29(QPDF&amp; pdf, char const* arg2)
     // explicitly change the ownership to the wrong value.
     auto dict = QPDFObjectHandle::newDictionary();
     dict.replaceKey("/QTest", pdf.getTrailer().getKey("/QTest"));
-    other.getTrailer().replaceKey("/QTest", dict);
+    other->getTrailer().replaceKey("/QTest", dict);
  
     try {
-        QPDFWriter w(other, "a.pdf");
+        QPDFWriter w(*other, "a.pdf");
+        w.write();
+        std::cout << "oops -- didn't throw" << std::endl;
+    } catch (std::logic_error const& e) {
+        std::cout << "logic error: " << e.what() << std::endl;
+    }
+
+    // Make sure deleting the other source doesn't prevent detection.
+    auto other2 = QPDF::create();
+    other2->emptyPDF();
+    dict = QPDFObjectHandle::newDictionary();
+    dict.replaceKey("/QTest", other2->getRoot());
+    other->getTrailer().replaceKey("/QTest", dict);
+    other2 = nullptr;
+    try {
+        QPDFWriter w(*other, "a.pdf");
         w.write();
         std::cout << "oops -- didn't throw" << std::endl;
     } catch (std::logic_error const& e) {
@@ -1158,7 +1173,7 @@ test_29(QPDF&amp; pdf, char const* arg2)
  
     // Detect adding a foreign object
     auto root1 = pdf.getRoot();
-    auto root2 = other.getRoot();
+    auto root2 = other->getRoot();
     try {
         root1.replaceKey("/Oops", root2);
     } catch (std::logic_error const& e) {
@@ -3300,14 +3315,20 @@ test_92(QPDF&amp; pdf, char const* arg2)
     check(resources);
     check(contents);
     check(contents_dict);
-    // Objects that were originally indirect should be null.
+    // Objects that were originally indirect should be destroyed.
     // Otherwise, they should have retained their old values. See
     // comments in QPDFValueProxy::reset for why this is the case.
-    assert(root.isNull());
-    assert(page1.isNull());
-    assert(contents.isNull());
-    assert(!resources.isNull());
-    assert(!contents_dict.isNull());
+    assert(root.isDestroyed());
+    assert(page1.isDestroyed());
+    assert(contents.isDestroyed());
+    assert(resources.isDictionary());
+    assert(contents_dict.isDictionary());
+    try {
+        root.unparse();
+        assert(false);
+    } catch (std::logic_error&) {
+        // Expected
+    }
 }
  
 static void