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.

9 files changed, 105 insertions, 25 deletions

diff --git a/libqpdf/CMakeLists.txt b/libqpdf/CMakeLists.txt
index 95024c64..3084813d 100644
--- a/libqpdf/CMakeLists.txt
+++ b/libqpdf/CMakeLists.txt
@@ -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
diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc
index 40390055..c66cfd5a 100644
--- a/libqpdf/QPDF.cc
+++ b/libqpdf/QPDF.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();
     }
 }
 
diff --git a/libqpdf/QPDFObjectHandle.cc b/libqpdf/QPDFObjectHandle.cc
index fa8aae7f..556e3a1e 100644
--- a/libqpdf/QPDFObjectHandle.cc
+++ b/libqpdf/QPDFObjectHandle.cc
@@ -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);
diff --git a/libqpdf/QPDFValueProxy.cc b/libqpdf/QPDFValueProxy.cc
index 461d8177..befaaf8b 100644
--- a/libqpdf/QPDFValueProxy.cc
+++ b/libqpdf/QPDFValueProxy.cc
@@ -1,6 +1,7 @@
 #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();
+}
diff --git a/libqpdf/QPDF_Destroyed.cc b/libqpdf/QPDF_Destroyed.cc
new file mode 100644
index 00000000..55308c9a
--- /dev/null
+++ b/libqpdf/QPDF_Destroyed.cc
@@ -0,0 +1,39 @@
+#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();
+}
diff --git a/libqpdf/QPDF_Reserved.cc b/libqpdf/QPDF_Reserved.cc
index 420cf765..9e795e57 100644
--- a/libqpdf/QPDF_Reserved.cc
+++ b/libqpdf/QPDF_Reserved.cc
@@ -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();
 }
diff --git a/libqpdf/QPDF_Unresolved.cc b/libqpdf/QPDF_Unresolved.cc
index 503e5b84..79553c1e 100644
--- a/libqpdf/QPDF_Unresolved.cc
+++ b/libqpdf/QPDF_Unresolved.cc
@@ -17,7 +17,7 @@ std::shared_ptr<QPDFValueProxy>
 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();
 }
diff --git a/libqpdf/qpdf/QPDFValueProxy.hh b/libqpdf/qpdf/QPDFValueProxy.hh
index ca8db544..8ddcacf7 100644
--- a/libqpdf/qpdf/QPDFValueProxy.hh
+++ b/libqpdf/qpdf/QPDFValueProxy.hh
@@ -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
     {
diff --git a/libqpdf/qpdf/QPDF_Destroyed.hh b/libqpdf/qpdf/QPDF_Destroyed.hh
new file mode 100644
index 00000000..78c2d601
--- /dev/null
+++ b/libqpdf/qpdf/QPDF_Destroyed.hh
@@ -0,0 +1,19 @@
+#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