Merge pull request #1242 from m-holger/fuzz

Tighten page tree checks

fuzz/CMakeLists.txt

@@ -127,7 +127,9 @@ set(CORPUS_OTHER
   69977b.fuzz
   69977c.fuzz
   70055.fuzz
-  4599089157701632.fuzz
+  70245.fuzz
+  70306.fuzz
+  4826608268017664.fuzz
 )
 
 set(CORPUS_DIR ${CMAKE_CURRENT_BINARY_DIR}/qpdf_corpus)

fuzz/json_fuzzer.cc

@@ -33,6 +33,7 @@ FuzzHelper::doChecks()
         std::cerr << "runtime_error parsing json: " << e.what() << std::endl;
     }
     QPDF q;
+    q.setMaxWarnings(1000);
     Buffer buf(const_cast<unsigned char*>(data), size);
     auto is = std::make_shared<BufferInputSource>("json", &buf);
     q.createFromJSON(is);

fuzz/qtest/fuzz.test

@@ -21,7 +21,7 @@ my @fuzzers = (
     ['pngpredictor' => 1],
     ['runlength' => 6],
     ['tiffpredictor' => 2],
-    ['qpdf' => 70],             # increment when adding new files
+    ['qpdf' => 72],             # increment when adding new files
     );
 
 my $n_tests = 0;

include/qpdf/QPDF.hh

@@ -1515,6 +1515,7 @@ class QPDF
         std::set<QPDFObjGen> resolving;
         QPDFObjectHandle trailer;
         std::vector<QPDFObjectHandle> all_pages;
+        bool invalid_page_found{false};
         std::map<QPDFObjGen, int> pageobj_to_pages_pos;
         bool pushed_inherited_attributes_to_pages{false};
         bool ever_pushed_inherited_attributes_to_pages{false};

libqpdf/QPDF_json.cc

@@ -233,12 +233,13 @@ provide_data(std::shared_ptr<InputSource> is, qpdf_offset_t start, qpdf_offset_t
 class QPDF::JSONReactor: public JSON::Reactor
 {
   public:
-    JSONReactor(QPDF& pdf, std::shared_ptr<InputSource> is, bool must_be_complete) :
+    JSONReactor(QPDF& pdf, std::shared_ptr<InputSource> is, bool must_be_complete, int max_warnings) :
         pdf(pdf),
         is(is),
         must_be_complete(must_be_complete),
         descr(std::make_shared<QPDFValue::Description>(
-            QPDFValue::JSON_Descr(std::make_shared<std::string>(is->getName()), "")))
+            QPDFValue::JSON_Descr(std::make_shared<std::string>(is->getName()), ""))),
+        max_warnings(max_warnings)
     {
         for (auto& oc: pdf.m->obj_cache) {
             if (oc.second.object->getTypeCode() == ::ot_reserved) {
@@ -291,7 +292,8 @@ class QPDF::JSONReactor: public JSON::Reactor
     std::shared_ptr<InputSource> is;
     bool must_be_complete{true};
     std::shared_ptr<QPDFValue::Description> descr;
-    bool errors{false};
+    int errors{0};
+    int max_warnings{0};
     bool saw_qpdf{false};
     bool saw_qpdf_meta{false};
     bool saw_objects{false};
@@ -314,18 +316,21 @@ class QPDF::JSONReactor: public JSON::Reactor
 void
 QPDF::JSONReactor::error(qpdf_offset_t offset, std::string const& msg)
 {
-    this->errors = true;
+    ++errors;
     std::string object = this->cur_object;
     if (is->getName() != pdf.getFilename()) {
         object += " from " + is->getName();
     }
     this->pdf.warn(qpdf_e_json, object, offset, msg);
+    if (max_warnings > 0 && errors >= max_warnings) {
+        throw std::runtime_error("errors found in JSON");
+    }
 }
 
 bool
 QPDF::JSONReactor::anyErrors() const
 {
-    return this->errors;
+    return errors > 0;
 }
 
 void
@@ -820,7 +825,7 @@ QPDF::updateFromJSON(std::shared_ptr<InputSource> is)
 void
 QPDF::importJSON(std::shared_ptr<InputSource> is, bool must_be_complete)
 {
-    JSONReactor reactor(*this, is, must_be_complete);
+    JSONReactor reactor(*this, is, must_be_complete, m->max_warnings);
     try {
         JSON::parse(*is, &reactor);
     } catch (std::runtime_error& e) {

libqpdf/QPDF_pages.cc

@@ -40,7 +40,7 @@ std::vector<QPDFObjectHandle> const&
 QPDF::getAllPages()
 {
     // Note that pushInheritedAttributesToPage may also be used to initialize m->all_pages.
-    if (m->all_pages.empty()) {
+    if (m->all_pages.empty() && !m->invalid_page_found) {
         m->ever_called_get_all_pages = true;
         QPDFObjGen::set visited;
         QPDFObjGen::set seen;
@@ -66,9 +66,15 @@ QPDF::getAllPages()
             getRoot().replaceKey("/Pages", pages);
         }
         seen.clear();
-        if (pages.hasKey("/Kids")) {
+        if (!pages.hasKey("/Kids")) {
             // Ensure we actually found a /Pages object.
-            getAllPagesInternal(pages, visited, seen, false);
+            throw QPDFExc(
+                qpdf_e_pages, m->file->getName(), "", 0, "root of pages tree has no /Kids array");
+        }
+        getAllPagesInternal(pages, visited, seen, false);
+        if (m->invalid_page_found) {
+            flattenPagesTree();
+            m->invalid_page_found = false;
         }
     }
     return m->all_pages;
@@ -100,6 +106,7 @@ QPDF::getAllPagesInternal(
         auto kid = kids.getArrayItem(i);
         if (!kid.isDictionary()) {
             kid.warnIfPossible("Pages tree includes non-dictionary object; ignoring");
+            m->invalid_page_found = true;
             continue;
         }
         if (kid.hasKey("/Kids")) {
@@ -181,7 +188,11 @@ QPDF::flattenPagesTree()
     pages.replaceKey("/Kids", QPDFObjectHandle::newArray(m->all_pages));
     // /Count has not changed
     if (pages.getKey("/Count").getUIntValue() != len) {
-        throw std::runtime_error("/Count is wrong after flattening pages tree");
+        if (m->invalid_page_found && pages.getKey("/Count").getUIntValue() > len) {
+            pages.replaceKey("/Count", QPDFObjectHandle::newInteger(toI(len)));
+        } else {
+            throw std::runtime_error("/Count is wrong after flattening pages tree");
+        }
     }
 }