From dba61da1bfb7e4d74c723f369d1c017df9707a14 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Thu, 8 Sep 2022 08:03:57 -0400 Subject: 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. --- libqpdf/CMakeLists.txt | 1 + libqpdf/QPDF.cc | 21 +++++++++++---------- libqpdf/QPDFObjectHandle.cc | 12 ++++++++++-- libqpdf/QPDFValueProxy.cc | 8 ++++++++ libqpdf/QPDF_Destroyed.cc | 39 +++++++++++++++++++++++++++++++++++++++ libqpdf/QPDF_Reserved.cc | 2 +- libqpdf/QPDF_Unresolved.cc | 4 +++- libqpdf/qpdf/QPDFValueProxy.hh | 24 +++++++++++++----------- libqpdf/qpdf/QPDF_Destroyed.hh | 19 +++++++++++++++++++ 9 files changed, 105 insertions(+), 25 deletions(-) create mode 100644 libqpdf/QPDF_Destroyed.cc create mode 100644 libqpdf/qpdf/QPDF_Destroyed.hh (limited to 'libqpdf') 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(); } @@ -351,6 +353,12 @@ QPDFObjectHandle::asString() return dereference() ? obj->as() : nullptr; } +bool +QPDFObjectHandle::isDestroyed() +{ + return dereference() && (obj->getTypeCode() == ::ot_destroyed); +} + bool QPDFObjectHandle::isBool() { 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 #include +#include 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 + +#include + +QPDF_Destroyed::QPDF_Destroyed() : + QPDFValue(::ot_destroyed, "destroyed") +{ +} + +std::shared_ptr +QPDF_Destroyed::getInstance() +{ + static std::shared_ptr instance(new QPDF_Destroyed()); + return instance; +} + +std::shared_ptr +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 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 + +class QPDF_Destroyed: public QPDFValue +{ + public: + virtual ~QPDF_Destroyed() = default; + virtual std::shared_ptr shallowCopy(); + virtual std::string unparse(); + virtual JSON getJSON(int json_version); + static std::shared_ptr getInstance(); + + private: + QPDF_Destroyed(); +}; + +#endif // QPDF_DESTROYED_HH -- cgit v1.2.3-54-g00ecf