diff options
author | Jay Berkenbilt <ejb@ql.org> | 2022-09-08 17:06:15 +0200 |
---|---|---|
committer | Jay Berkenbilt <ejb@ql.org> | 2022-09-08 17:06:15 +0200 |
commit | c7a4967d10fb9688f235baa8e57a1fb578f5387d (patch) | |
tree | b14956a2371befbd30b2a43583c390bc170ccd3d | |
parent | dba61da1bfb7e4d74c723f369d1c017df9707a14 (diff) | |
download | qpdf-c7a4967d10fb9688f235baa8e57a1fb578f5387d.tar.zst |
Change reset to disconnect and clarify comments
I decided that it's actually fine to copy a direct object to another
QPDF. Even if we eventually prevent a QPDFObject from having multiple
parents, this could happen if an object is moved.
-rw-r--r-- | TODO | 11 | ||||
-rw-r--r-- | include/qpdf/QPDFObjectHandle.hh | 13 | ||||
-rw-r--r-- | libqpdf/QPDF.cc | 10 | ||||
-rw-r--r-- | libqpdf/QPDFObjectHandle.cc | 4 | ||||
-rw-r--r-- | libqpdf/QPDFValueProxy.cc | 1 | ||||
-rw-r--r-- | libqpdf/QPDFWriter.cc | 14 | ||||
-rw-r--r-- | libqpdf/QPDF_Array.cc | 4 | ||||
-rw-r--r-- | libqpdf/QPDF_Dictionary.cc | 4 | ||||
-rw-r--r-- | libqpdf/QPDF_Stream.cc | 4 | ||||
-rw-r--r-- | libqpdf/SparseOHArray.cc | 4 | ||||
-rw-r--r-- | libqpdf/qpdf/QPDFValue.hh | 2 | ||||
-rw-r--r-- | libqpdf/qpdf/QPDFValueProxy.hh | 25 | ||||
-rw-r--r-- | libqpdf/qpdf/QPDF_Array.hh | 2 | ||||
-rw-r--r-- | libqpdf/qpdf/QPDF_Dictionary.hh | 2 | ||||
-rw-r--r-- | libqpdf/qpdf/QPDF_Stream.hh | 2 | ||||
-rw-r--r-- | libqpdf/qpdf/SparseOHArray.hh | 2 | ||||
-rw-r--r-- | qpdf/test_driver.cc | 4 |
17 files changed, 44 insertions, 64 deletions
@@ -785,7 +785,7 @@ Rejected Ideas and too much toil for library users to be worth the small benefit of not having to call resetObjGen in QPDF's destructor. -* Fix Multiple Direct Object Owner Issue +* Fix Multiple Direct Object Parent Issue These are some ideas I had before m-holger's changes to split QPDFValue from QPDFObject. These notes were written prior to the @@ -811,12 +811,3 @@ Rejected Ideas Note that arrays and dictionaries still need to contain QPDFObjectHandle because of indirect objects. This only pertains to direct objects, which are always "resolved" in QPDFObjectHandle. - - 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). diff --git a/include/qpdf/QPDFObjectHandle.hh b/include/qpdf/QPDFObjectHandle.hh index a4768f97..07fe3ad5 100644 --- a/include/qpdf/QPDFObjectHandle.hh +++ b/include/qpdf/QPDFObjectHandle.hh @@ -392,7 +392,8 @@ class QPDFObjectHandle inline bool isIndirect() const; // This returns true for indirect objects from a QPDF that has - // been destroyed. + // been destroyed. Trying unparse such an object will throw a + // logic_error. QPDF_DLL bool isDestroyed(); @@ -1540,8 +1541,8 @@ class QPDFObjectHandle friend class ObjAccessor; // Provide access to specific classes for recursive - // reset(). - class Resetter + // disconnected(). + class DisconnectAccess { friend class QPDF_Dictionary; friend class QPDF_Stream; @@ -1549,9 +1550,9 @@ class QPDFObjectHandle private: static void - reset(QPDFObjectHandle& o) + disconnect(QPDFObjectHandle& o) { - o.reset(); + o.disconnect(); } }; friend class Resetter; @@ -1653,7 +1654,7 @@ class QPDFObjectHandle bool first_level_only, bool stop_at_streams); void shallowCopyInternal(QPDFObjectHandle& oh, bool first_level_only); - void reset(); + void disconnect(); void setParsedOffset(qpdf_offset_t offset); void parseContentStream_internal( std::string const& description, ParserCallbacks* callbacks); diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc index c66cfd5a..1b1ee784 100644 --- a/libqpdf/QPDF.cc +++ b/libqpdf/QPDF.cc @@ -252,9 +252,11 @@ QPDF::~QPDF() // 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. The call to reset also causes all + // QPDF object is active. The call to reset also causes all direct // QPDFObjectHandle objects that are reachable from this object to - // release their association with this QPDF. + // release their association with this QPDF. Direct objects are + // not destroyed since they can be moved to other QPDF objects + // safely. // At this point, obviously no one is still using the QPDF object, // but we'll explicitly clear the xref table anyway just to @@ -262,9 +264,7 @@ QPDF::~QPDF() this->m->xref_table.clear(); auto null_obj = QPDF_Null::create(); for (auto const& iter: this->m->obj_cache) { - iter.second.object->reset(); - // It would be better if reset() could call destroy(), but it - // can't -- see comments in QPDFValueProxy::reset(). + iter.second.object->disconnect(); iter.second.object->destroy(); } } diff --git a/libqpdf/QPDFObjectHandle.cc b/libqpdf/QPDFObjectHandle.cc index 556e3a1e..d31af881 100644 --- a/libqpdf/QPDFObjectHandle.cc +++ b/libqpdf/QPDFObjectHandle.cc @@ -249,7 +249,7 @@ QPDFObjectHandle::operator!=(QPDFObjectHandle const& rhs) const } void -QPDFObjectHandle::reset() +QPDFObjectHandle::disconnect() { // Recursively remove association with any QPDF object. This // method may only be called during final destruction. @@ -257,7 +257,7 @@ QPDFObjectHandle::reset() // pointer itself, so we don't do that here. Other objects call it // through this method. if (!isIndirect()) { - this->obj->reset(); + this->obj->disconnect(); } } diff --git a/libqpdf/QPDFValueProxy.cc b/libqpdf/QPDFValueProxy.cc index befaaf8b..c315128c 100644 --- a/libqpdf/QPDFValueProxy.cc +++ b/libqpdf/QPDFValueProxy.cc @@ -13,6 +13,5 @@ QPDFValueProxy::doResolve() void QPDFValueProxy::destroy() { - // See comments in reset() for why this isn't part of reset. value = QPDF_Destroyed::getInstance(); } diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index 83785465..aad536b0 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -1198,14 +1198,12 @@ void QPDFWriter::enqueueObject(QPDFObjectHandle object) { if (object.isIndirect()) { - // This owner check should really be done for all objects, not - // just indirect objects. As of the time of the release of - // qpdf 11, it is known that there are cases of direct objects - // from other files getting copied into multiple QPDF objects. - // This definitely happens in the page splitting code. If we - // were to implement strong checks to prevent objects from - // having multiple owners, once that was complete phased in, - // this check could be moved outside the if statement. + // This owner check can only be done for indirect objects. It + // is possible for a direct object to have an owning QPDF that + // is from another file if a direct QPDFObjectHandle from one + // file was insert into another file without copying. Doing + // that is safe even if the original QPDF gets destroyed, + // which just disconnects the QPDFObjectHandle from its owner. if (object.getOwningQPDF() != &(this->m->pdf)) { QTC::TC("qpdf", "QPDFWriter foreign object"); throw std::logic_error( diff --git a/libqpdf/QPDF_Array.cc b/libqpdf/QPDF_Array.cc index 010efd1b..6d60b789 100644 --- a/libqpdf/QPDF_Array.cc +++ b/libqpdf/QPDF_Array.cc @@ -35,9 +35,9 @@ QPDF_Array::shallowCopy() } void -QPDF_Array::reset() +QPDF_Array::disconnect() { - elements.reset(); + elements.disconnect(); } std::string diff --git a/libqpdf/QPDF_Dictionary.cc b/libqpdf/QPDF_Dictionary.cc index 00d9f098..bf694949 100644 --- a/libqpdf/QPDF_Dictionary.cc +++ b/libqpdf/QPDF_Dictionary.cc @@ -22,10 +22,10 @@ QPDF_Dictionary::shallowCopy() } void -QPDF_Dictionary::reset() +QPDF_Dictionary::disconnect() { for (auto& iter: this->items) { - QPDFObjectHandle::Resetter::reset(iter.second); + QPDFObjectHandle::DisconnectAccess::disconnect(iter.second); } } diff --git a/libqpdf/QPDF_Stream.cc b/libqpdf/QPDF_Stream.cc index dd927dfb..83610e94 100644 --- a/libqpdf/QPDF_Stream.cc +++ b/libqpdf/QPDF_Stream.cc @@ -168,10 +168,10 @@ QPDF_Stream::getFilterOnWrite() const } void -QPDF_Stream::reset() +QPDF_Stream::disconnect() { this->stream_provider = nullptr; - QPDFObjectHandle::Resetter::reset(this->stream_dict); + QPDFObjectHandle::DisconnectAccess::disconnect(this->stream_dict); } void diff --git a/libqpdf/SparseOHArray.cc b/libqpdf/SparseOHArray.cc index a5a8e4e8..6ff02f6c 100644 --- a/libqpdf/SparseOHArray.cc +++ b/libqpdf/SparseOHArray.cc @@ -49,10 +49,10 @@ SparseOHArray::remove_last() } void -SparseOHArray::reset() +SparseOHArray::disconnect() { for (auto& iter: this->elements) { - QPDFObjectHandle::Resetter::reset(iter.second); + QPDFObjectHandle::DisconnectAccess::disconnect(iter.second); } } diff --git a/libqpdf/qpdf/QPDFValue.hh b/libqpdf/qpdf/QPDFValue.hh index 71078ec8..dbde018c 100644 --- a/libqpdf/qpdf/QPDFValue.hh +++ b/libqpdf/qpdf/QPDFValue.hh @@ -64,7 +64,7 @@ class QPDFValue return og; } virtual void - reset() + disconnect() { } diff --git a/libqpdf/qpdf/QPDFValueProxy.hh b/libqpdf/qpdf/QPDFValueProxy.hh index 8ddcacf7..ff3f80b6 100644 --- a/libqpdf/qpdf/QPDFValueProxy.hh +++ b/libqpdf/qpdf/QPDFValueProxy.hh @@ -102,33 +102,24 @@ class QPDFValueProxy o->value->og = og; } - // The following two methods are for use by class QPDF only void setObjGen(QPDF* qpdf, QPDFObjGen const& og) { + // Intended for use by the QPDF class value->qpdf = qpdf; value->og = og; } void - reset() - { - value->reset(); - // It would be better if, rather than clearing value->qpdf and - // 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. + disconnect() + { + // Disconnect an object from its owning QPDF. This is called + // by QPDF's destructor. + value->disconnect(); value->qpdf = nullptr; value->og = QPDFObjGen(); } - + // Mark an object as destroyed. Used by QPDF's destructor for its + // indirect objects. void destroy(); bool diff --git a/libqpdf/qpdf/QPDF_Array.hh b/libqpdf/qpdf/QPDF_Array.hh index 33dd1881..8fb227da 100644 --- a/libqpdf/qpdf/QPDF_Array.hh +++ b/libqpdf/qpdf/QPDF_Array.hh @@ -17,7 +17,7 @@ class QPDF_Array: public QPDFValue virtual std::shared_ptr<QPDFValueProxy> shallowCopy(); virtual std::string unparse(); virtual JSON getJSON(int json_version); - virtual void reset(); + virtual void disconnect(); int getNItems() const; QPDFObjectHandle getItem(int n) const; diff --git a/libqpdf/qpdf/QPDF_Dictionary.hh b/libqpdf/qpdf/QPDF_Dictionary.hh index 5f3e54fa..96985fe7 100644 --- a/libqpdf/qpdf/QPDF_Dictionary.hh +++ b/libqpdf/qpdf/QPDF_Dictionary.hh @@ -17,7 +17,7 @@ class QPDF_Dictionary: public QPDFValue virtual std::shared_ptr<QPDFValueProxy> shallowCopy(); virtual std::string unparse(); virtual JSON getJSON(int json_version); - virtual void reset(); + virtual void disconnect(); // hasKey() and getKeys() treat keys with null values as if they // aren't there. getKey() returns null for the value of a diff --git a/libqpdf/qpdf/QPDF_Stream.hh b/libqpdf/qpdf/QPDF_Stream.hh index c448697d..bf7dbca7 100644 --- a/libqpdf/qpdf/QPDF_Stream.hh +++ b/libqpdf/qpdf/QPDF_Stream.hh @@ -27,7 +27,7 @@ class QPDF_Stream: public QPDFValue virtual std::string unparse(); virtual JSON getJSON(int json_version); virtual void setDescription(QPDF*, std::string const&); - virtual void reset(); + virtual void disconnect(); QPDFObjectHandle getDict() const; bool isDataModified() const; void setFilterOnWrite(bool); diff --git a/libqpdf/qpdf/SparseOHArray.hh b/libqpdf/qpdf/SparseOHArray.hh index 4ebe762c..440c0b2d 100644 --- a/libqpdf/qpdf/SparseOHArray.hh +++ b/libqpdf/qpdf/SparseOHArray.hh @@ -15,7 +15,7 @@ class SparseOHArray void setAt(size_t idx, QPDFObjectHandle oh); void erase(size_t idx); void insert(size_t idx, QPDFObjectHandle oh); - void reset(); + void disconnect(); typedef std::unordered_map<size_t, QPDFObjectHandle>::const_iterator const_iterator; diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc index 96d001d6..3e74c106 100644 --- a/qpdf/test_driver.cc +++ b/qpdf/test_driver.cc @@ -3316,8 +3316,8 @@ test_92(QPDF& pdf, char const* arg2) check(contents); check(contents_dict); // 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. + // Otherwise, they should have retained their old values but just + // lost their connection to the owning QPDF. assert(root.isDestroyed()); assert(page1.isDestroyed()); assert(contents.isDestroyed()); |