Merge pull request #901 from m-holger/jrrr

Refactor removal of reserved objects in QPDF::JSONReactor

3 files changed, 51 insertions, 57 deletions

diff --git a/include/qpdf/JSON.hh b/include/qpdf/JSON.hh
index 2906d85a..b368df99 100644
--- a/include/qpdf/JSON.hh
+++ b/include/qpdf/JSON.hh
@@ -54,6 +54,8 @@ class JSON
 {
   public:
     static int constexpr LATEST = 2;
+
+    QPDF_DLL
     JSON() = default;
 
     QPDF_DLL
@@ -369,7 +371,7 @@ class JSON
         }
         virtual ~JSON_dictionary() = default;
         virtual void write(Pipeline*, size_t depth) const;
-        std::map<std::string, std::shared_ptr<JSON_value>> members;
+        std::map<std::string, JSON> members;
         std::set<std::string> parsed_keys;
     };
     struct JSON_array: public JSON_value
@@ -380,7 +382,7 @@ class JSON
         }
         virtual ~JSON_array() = default;
         virtual void write(Pipeline*, size_t depth) const;
-        std::vector<std::shared_ptr<JSON_value>> elements;
+        std::vector<JSON> elements;
     };
     struct JSON_string: public JSON_value
     {
@@ -423,7 +425,7 @@ class JSON
         std::function<void(Pipeline*)> fn;
     };
 
-    JSON(std::shared_ptr<JSON_value>);
+    JSON(std::unique_ptr<JSON_value>);
 
     static bool checkSchemaInternal(
         JSON_value* this_v,
@@ -441,13 +443,13 @@ class JSON
         ~Members() = default;
 
       private:
-        Members(std::shared_ptr<JSON_value>);
+        Members(std::unique_ptr<JSON_value>);
         Members(Members const&) = delete;
 
-        std::shared_ptr<JSON_value> value;
+        std::unique_ptr<JSON_value> value;
         // start and end are only populated for objects created by parse
-        qpdf_offset_t start;
-        qpdf_offset_t end;
+        qpdf_offset_t start{0};
+        qpdf_offset_t end{0};
     };
 
     std::shared_ptr<Members> m;
diff --git a/libqpdf/JSON.cc b/libqpdf/JSON.cc
index 76db652b..cb60eabc 100644
--- a/libqpdf/JSON.cc
+++ b/libqpdf/JSON.cc
@@ -9,15 +9,13 @@
 #include <cstring>
 #include <stdexcept>
 
-JSON::Members::Members(std::shared_ptr<JSON_value> value) :
-    value(value),
-    start(0),
-    end(0)
+JSON::Members::Members(std::unique_ptr<JSON_value> value) :
+    value(std::move(value))
 {
 }
 
-JSON::JSON(std::shared_ptr<JSON_value> value) :
-    m(new Members(value))
+JSON::JSON(std::unique_ptr<JSON_value> value) :
+    m(new Members(std::move(value)))
 {
 }
 
@@ -278,7 +276,7 @@ JSON::encode_string(std::string const& str)
 JSON
 JSON::makeDictionary()
 {
-    return JSON(std::make_shared<JSON_dictionary>());
+    return JSON(std::make_unique<JSON_dictionary>());
 }
 
 JSON
@@ -286,7 +284,7 @@ JSON::addDictionaryMember(std::string const& key, JSON const& val)
 {
     if (auto* obj = dynamic_cast<JSON_dictionary*>(this->m->value.get())) {
         return obj->members[encode_string(key)] =
-                   val.m->value ? val.m->value : std::make_shared<JSON_null>();
+                   val.m->value ? val : makeNull();
     } else {
         throw std::runtime_error(
             "JSON::addDictionaryMember called on non-dictionary");
@@ -311,7 +309,7 @@ JSON::checkDictionaryKeySeen(std::string const& key)
 JSON
 JSON::makeArray()
 {
-    return JSON(std::make_shared<JSON_array>());
+    return JSON(std::make_unique<JSON_array>());
 }
 
 JSON
@@ -322,9 +320,9 @@ JSON::addArrayElement(JSON const& val)
         throw std::runtime_error("JSON::addArrayElement called on non-array");
     }
     if (val.m->value.get()) {
-        arr->elements.push_back(val.m->value);
+        arr->elements.push_back(val);
     } else {
-        arr->elements.push_back(std::make_shared<JSON_null>());
+        arr->elements.push_back(makeNull());
     }
     return arr->elements.back();
 }
@@ -332,43 +330,43 @@ JSON::addArrayElement(JSON const& val)
 JSON
 JSON::makeString(std::string const& utf8)
 {
-    return JSON(std::make_shared<JSON_string>(utf8));
+    return JSON(std::make_unique<JSON_string>(utf8));
 }
 
 JSON
 JSON::makeInt(long long int value)
 {
-    return JSON(std::make_shared<JSON_number>(value));
+    return JSON(std::make_unique<JSON_number>(value));
 }
 
 JSON
 JSON::makeReal(double value)
 {
-    return JSON(std::make_shared<JSON_number>(value));
+    return JSON(std::make_unique<JSON_number>(value));
 }
 
 JSON
 JSON::makeNumber(std::string const& encoded)
 {
-    return JSON(std::make_shared<JSON_number>(encoded));
+    return JSON(std::make_unique<JSON_number>(encoded));
 }
 
 JSON
 JSON::makeBool(bool value)
 {
-    return JSON(std::make_shared<JSON_bool>(value));
+    return JSON(std::make_unique<JSON_bool>(value));
 }
 
 JSON
 JSON::makeNull()
 {
-    return JSON(std::make_shared<JSON_null>());
+    return JSON(std::make_unique<JSON_null>());
 }
 
 JSON
 JSON::makeBlob(std::function<void(Pipeline*)> fn)
 {
-    return JSON(std::make_shared<JSON_blob>(fn));
+    return JSON(std::make_unique<JSON_blob>(fn));
 }
 
 bool
@@ -504,11 +502,11 @@ JSON::checkSchemaInternal(
     }
 
     if (sch_dict && (!pattern_key.empty())) {
-        auto pattern_schema = sch_dict->members[pattern_key].get();
+        auto pattern_schema = sch_dict->members[pattern_key].m->value.get();
         for (auto const& iter: this_dict->members) {
             std::string const& key = iter.first;
             checkSchemaInternal(
-                this_dict->members[key].get(),
+                this_dict->members[key].m->value.get(),
                 pattern_schema,
                 flags,
                 errors,
@@ -519,8 +517,8 @@ JSON::checkSchemaInternal(
             std::string const& key = iter.first;
             if (this_dict->members.count(key)) {
                 checkSchemaInternal(
-                    this_dict->members[key].get(),
-                    iter.second.get(),
+                    this_dict->members[key].m->value.get(),
+                    iter.second.m->value.get(),
                     flags,
                     errors,
                     prefix + "." + key);
@@ -557,8 +555,8 @@ JSON::checkSchemaInternal(
                 int i = 0;
                 for (auto const& element: this_arr->elements) {
                     checkSchemaInternal(
-                        element.get(),
-                        sch_arr->elements.at(0).get(),
+                        element.m->value.get(),
+                        sch_arr->elements.at(0).m->value.get(),
                         flags,
                         errors,
                         prefix + "." + std::to_string(i));
@@ -568,7 +566,7 @@ JSON::checkSchemaInternal(
                 QTC::TC("libtests", "JSON schema array for single item");
                 checkSchemaInternal(
                     this_v,
-                    sch_arr->elements.at(0).get(),
+                    sch_arr->elements.at(0).m->value.get(),
                     flags,
                     errors,
                     prefix);
@@ -587,8 +585,8 @@ JSON::checkSchemaInternal(
             size_t i = 0;
             for (auto const& element: this_arr->elements) {
                 checkSchemaInternal(
-                    element.get(),
-                    sch_arr->elements.at(i).get(),
+                    element.m->value.get(),
+                    sch_arr->elements.at(i).m->value.get(),
                     flags,
                     errors,
                     prefix + "." + std::to_string(i));
diff --git a/libqpdf/QPDF_json.cc b/libqpdf/QPDF_json.cc
index f6bb279c..563e8e29 100644
--- a/libqpdf/QPDF_json.cc
+++ b/libqpdf/QPDF_json.cc
@@ -6,6 +6,7 @@
 #include <qpdf/QIntC.hh>
 #include <qpdf/QPDFObject_private.hh>
 #include <qpdf/QPDFValue.hh>
+#include <qpdf/QPDF_Null.hh>
 #include <qpdf/QTC.hh>
 #include <qpdf/QUtil.hh>
 #include <algorithm>
@@ -234,6 +235,11 @@ class QPDF::JSONReactor: public JSON::Reactor
         descr(std::make_shared<QPDFValue::Description>(QPDFValue::JSON_Descr(
             std::make_shared<std::string>(is->getName()), "")))
     {
+        for (auto& oc: pdf.m->obj_cache) {
+            if (oc.second.object->getTypeCode() == ::ot_reserved) {
+                reserved.insert(oc.first);
+            }
+        }
     }
     virtual ~JSONReactor() = default;
     virtual void dictionaryStart() override;
@@ -265,7 +271,6 @@ class QPDF::JSONReactor: public JSON::Reactor
     void setObjectDescription(QPDFObjectHandle& oh, JSON const& value);
     QPDFObjectHandle makeObject(JSON const& value);
     void error(qpdf_offset_t offset, std::string const& message);
-    QPDFObjectHandle reserveObject(int obj, int gen);
     void replaceObject(
         QPDFObjectHandle to_replace,
         QPDFObjectHandle replacement,
@@ -416,27 +421,17 @@ QPDF::JSONReactor::containerEnd(JSON const& value)
             object_stack.pop_back();
         }
     } else if ((state == st_top) && (from_state == st_qpdf)) {
-        for (auto const& og: this->reserved) {
-            // Handle dangling indirect object references which the
-            // PDF spec says to treat as nulls. It's tempting to make
-            // this an error, but that would be wrong since valid
-            // input files may have these.
-            QTC::TC("qpdf", "QPDF_json non-trivial null reserved");
-            this->pdf.replaceObject(og, QPDFObjectHandle::newNull());
+        // Handle dangling indirect object references which the PDF spec says to
+        // treat as nulls. It's tempting to make this an error, but that would
+        // be wrong since valid input files may have these.
+        for (auto& oc: pdf.m->obj_cache) {
+            if (oc.second.object->getTypeCode() == ::ot_reserved &&
+                reserved.count(oc.first) == 0) {
+                QTC::TC("qpdf", "QPDF_json non-trivial null reserved");
+                pdf.updateCache(oc.first, QPDF_Null::create(), -1, -1);
+            }
         }
-        this->reserved.clear();
-    }
-}
-
-QPDFObjectHandle
-QPDF::JSONReactor::reserveObject(int obj, int gen)
-{
-    QPDFObjGen og(obj, gen);
-    auto oh = pdf.reserveObjectIfNotExists(og);
-    if (oh.isReserved()) {
-        this->reserved.insert(og);
     }
-    return oh;
 }
 
 void
@@ -446,7 +441,6 @@ QPDF::JSONReactor::replaceObject(
     JSON const& value)
 {
     auto og = to_replace.getObjGen();
-    this->reserved.erase(og);
     this->pdf.replaceObject(og, replacement);
     auto oh = pdf.getObject(og);
     setObjectDescription(oh, value);
@@ -564,7 +558,7 @@ QPDF::JSONReactor::dictionaryItem(std::string const& key, JSON const& value)
             this->cur_object = "trailer";
         } else if (is_obj_key(key, obj, gen)) {
             this->cur_object = key;
-            auto oh = reserveObject(obj, gen);
+            auto oh = pdf.reserveObjectIfNotExists(QPDFObjGen(obj, gen));
             object_stack.push_back(oh);
             nestedState(key, value, st_object_top);
         } else {
@@ -763,7 +757,7 @@ QPDF::JSONReactor::makeObject(JSON const& value)
         int gen = 0;
         std::string str;
         if (is_indirect_object(str_v, obj, gen)) {
-            result = reserveObject(obj, gen);
+            result = pdf.reserveObjectIfNotExists(QPDFObjGen(obj, gen));
         } else if (is_unicode_string(str_v, str)) {
             result = QPDFObjectHandle::newUnicodeString(str);
         } else if (is_binary_string(str_v, str)) {