Disable copying/assigning to QPDF objects, add QPDF::create()

Jay Berkenbilt
1 parent da0b0e40
Showing 9 changed files with 42 additions and 8 deletions
ChangeLog
TODO
fuzz/qpdf_fuzzer.cc
include/qpdf/QPDF.hh
libqpdf/QPDF.cc
libqpdf/QPDFJob.cc
libqpdf/qpdf-c.cc
manual/release-notes.rst
qpdf/test_driver.cc
+2022-09-02  Jay Berkenbilt  <ejb@ql.org>
+
+        * Add new QPDF::create() factory method that returns
+        std::shared_ptr<QPDF>.
+
+        * Prevent copying/assigning to QPDF objects in the API. It has
+        never been safe to do this, but the API wasn't preventing it.
+
 2022-09-01  Jay Berkenbilt  <ejb@ql.org>
         * Remove QPDFObject.hh from include/qpdf. The only reason to
@@ -4,8 +4,6 @@ Next
 Before Release:
-* Consider a transition symbol for deprecation of stack-allocated QPDF
-  objects and also make QPDF non-copiable.
 * Evaluate issues tagged with `next`
 * Stay on top of https://github.com/pikepdf/pikepdf/pull/315
 * Consider whether otherwise unreferenced object streams should be
@@ -55,7 +55,7 @@ FuzzHelper::getQpdf()
 {
     auto is = std::shared_ptr<InputSource>(
         new BufferInputSource("fuzz input", &this->input_buffer));
-    auto qpdf = std::make_shared<QPDF>();
+    auto qpdf = QPDF::create();
     qpdf->processInputSource(is);
     return qpdf;
 }
@@ -64,6 +64,9 @@ class QPDF
     QPDF_DLL
     ~QPDF();
+    QPDF_DLL
+    static std::shared_ptr<QPDF> create();
+
     // Associate a file with a QPDF object and do initial parsing of
     // the file.  PDF objects are not read until they are needed.  A
     // QPDF object may be associated with only one file in its
@@ -929,6 +932,15 @@ class QPDF
     static bool test_json_validators();
   private:
+    // It has never been safe to copy QPDF objects as there is code in
+    // the library that assumes there are no copies of a QPDF object.
+    // Copying QPDF objects was not prevented by the API until qpdf
+    // 11. If you have been copying QPDF objects, use
+    // std::shared_ptr<QPDF> instead. From qpdf 11, you can use
+    // QPDF::create to create them.
+    QPDF(QPDF const&) = delete;
+    QPDF& operator=(QPDF const&) = delete;
+
     static std::string const qpdf_version;
     class ObjCache
@@ -264,6 +264,12 @@ QPDF::~QPDF()
     }
 }
+std::shared_ptr<QPDF>
+QPDF::create()
+{
+    return std::make_shared<QPDF>();
+}
+
 void
 QPDF::processFile(char const* filename, char const* password)
 {
@@ -1981,7 +1981,7 @@ QPDFJob::doProcessOnce(
     bool used_for_input,
     bool main_input)
 {
-    auto pdf = std::make_shared<QPDF>();
+    auto pdf = QPDF::create();
     setQPDFOptions(*pdf);
     if (empty) {
         pdf->emptyPDF();
@@ -148,7 +148,7 @@ qpdf_init()
 {
     QTC::TC("qpdf", "qpdf-c called qpdf_init");
     qpdf_data qpdf = new _qpdf_data();
-    qpdf->qpdf = std::make_shared<QPDF>();
+    qpdf->qpdf = QPDF::create();
     return qpdf;
 }
@@ -139,11 +139,18 @@ For a detailed list of changes, please see the file
       - See :ref:`breaking-crypto-api` for specific details, and see
         :ref:`weak-crypto` for a general discussion.
-    - QPDFObjectHandle::warnIfPossible no longer takes an optional
+    - ``QPDFObjectHandle::warnIfPossible`` no longer takes an optional
       argument to throw an exception if there is no description. If
       there is no description, it writes to the default logger's error
       stream.
+    - ``QPDF`` objects can no longer be copied or assigned to. It has
+      never been safe to do this because of assumptions made by
+      library code. Now it is prevented by the API. If you run into
+      trouble, use ``QPDF::create()`` to create ``QPDF`` shared
+      pointers (or create them in some other way if you need backward
+      compatibility with older qpdf versions).
+
   - CLI Enhancements
     - ``qpdf --list-attachments --verbose`` include some additional
@@ -196,6 +203,9 @@ For a detailed list of changes, please see the file
       generally not happen for correct code, but at least the
       situation is detectible now.
+    - Add new factory method ``QPDF::create()`` that returns a
+      ``std::shared_ptr<QPDF>``.
+
     - Add new ``Pipeline`` methods to reduce the amount of casting that is
       needed:
@@ -3262,12 +3262,12 @@ static void
 test_92(QPDF& pdf, char const* arg2)
 {
     // Exercise indirect objects owned by destroyed QPDF object.
-    QPDF* qpdf = new QPDF();
+    auto qpdf = QPDF::create();
     qpdf->emptyPDF();
     auto root = qpdf->getRoot();
     assert(root.getOwningQPDF() != nullptr);
     assert(root.isIndirect());
-    delete qpdf;
+    qpdf = nullptr;
     assert(root.getOwningQPDF() == nullptr);
     assert(!root.isIndirect());
 }