Refactor `limits_error` handling for improved clarity and consistency

- Introduce `limits_error` method in `QPDFParser` for centralized limit-related error handling. - Enhance warnings and error messages with detailed limit identifiers (e.g., `parser-max-nesting`). - Refactor limit checks to improve maintainability and ensure uniformity in error reporting. - Update tests and output to reflect adjusted error handling approach.

Refactor `limits_error` handling for improved clarity and consistency
- Introduce `limits_error` method in `QPDFParser` for centralized limit-related error handling. - Enhance warnings and error messages with detailed limit identifiers (e.g., `parser-max-nesting`). - Refactor limit checks to improve maintainability and ensure uniformity in error reporting. - Update tests and output to reflect adjusted error handling approach.
m-holger
1 parent 27e14333
Showing 4 changed files with 37 additions and 27 deletions
libqpdf/QPDFParser.cc
libqpdf/qpdf/QPDFParser.hh
qpdf/qtest/qpdf/issue-146.out
qpdf/qtest/qpdf/issue-202.out
@@ -437,18 +437,16 @@ QPDFParser::parseRemainder(bool content_stream)
         case QPDFTokenizer::tt_array_open:
         case QPDFTokenizer::tt_dict_open:
             if (stack.size() > max_nesting) {
-                global::Limits::error();
-                warn("limits error: ignoring excessively deeply nested data structure");
-                return {};
-            } else {
-                b_contents = false;
-                stack.emplace_back(
-                    input,
-                    (tokenizer.getType() == QPDFTokenizer::tt_array_open) ? st_array
-                                                                          : st_dictionary_key);
-                frame = &stack.back();
-                continue;
+                limits_error(
+                    "parser-max-nesting", "ignoring excessively deeply nested data structure");
             }
+            b_contents = false;
+            stack.emplace_back(
+                input,
+                (tokenizer.getType() == QPDFTokenizer::tt_array_open) ? st_array
+                                                                      : st_dictionary_key);
+            frame = &stack.back();
+            continue;
  
         case QPDFTokenizer::tt_bool:
             addScalar<QPDF_Bool>(tokenizer.getValue() == "true");
@@ -588,10 +586,10 @@ void
 QPDFParser::addScalar(Args&&... args)
 {
     auto limit = Limits::parser_max_container_size(bad_count || sanity_checks);
-    if (frame->olist.size() > limit || frame->dict.size() > limit) {
+    if (frame->olist.size() >= limit || frame->dict.size() >= limit) {
         // Stop adding scalars. We are going to abort when the close token or a bad token is
         // encountered.
-        max_bad_count = 0;
+        max_bad_count = 1;
         check_too_many_bad_tokens(); // always throws Error()
     }
     auto obj = QPDFObject::create<T>(std::forward<Args>(args)...);
@@ -646,26 +644,29 @@ void
 QPDFParser::check_too_many_bad_tokens()
 {
     auto limit = Limits::parser_max_container_size(bad_count || sanity_checks);
-    if (frame->olist.size() > limit || frame->dict.size() > limit) {
+    if (frame->olist.size() >= limit || frame->dict.size() >= limit) {
         if (bad_count) {
-            Limits::error();
-            warn(
-                "limits error: encountered errors while parsing an array or dictionary with more "
-                "than " +
-                std::to_string(limit) + " elements; giving up on reading object");
-            throw Error();
+            limits_error(
+                "parser-max-container-size-damaged",
+                "encountered errors while parsing an array or dictionary with more than " +
+                    std::to_string(limit) + " elements; giving up on reading object");
         }
-        warn(
+        limits_error(
+            "parser-max-container-size",
             "encountered an array or dictionary with more than " + std::to_string(limit) +
-            " elements during xref recovery; giving up on reading object");
+                " elements during xref recovery; giving up on reading object");
     }
-    if (max_bad_count && --max_bad_count > 0 && good_count > 4) {
+    if (max_bad_count && --max_bad_count == 0) {
+        limits_error(
+            "parser-max-errors", "too many errors during parsing; treating object as null");
+    }
+    if (good_count > 4) {
         good_count = 0;
         bad_count = 1;
         return;
     }
     if (++bad_count > 5 ||
-        (frame->state != st_array && QIntC::to_size(max_bad_count) < frame->olist.size())) {
+        (frame->state != st_array && std::cmp_less(max_bad_count, frame->olist.size()))) {
         // Give up after 5 errors in close proximity or if the number of missing dictionary keys
         // exceeds the remaining number of allowable total errors.
         warn("too many errors; giving up on reading object");
@@ -675,6 +676,14 @@ QPDFParser::check_too_many_bad_tokens()
 }
  
 void
+QPDFParser::limits_error(std::string const& limit, std::string const& msg)
+{
+    Limits::error();
+    warn("limits error("s + limit + "): " + msg);
+    throw Error();
+}
+
+void
 QPDFParser::warn(QPDFExc const& e) const
 {
     // If parsing on behalf of a QPDF object and want to give a warning, we can warn through the
@@ -124,6 +124,7 @@ class QPDFParser
     void check_too_many_bad_tokens();
     void warnDuplicateKey();
     void fixMissingKeys();
+    [[noreturn]] void limits_error(std::string const& limit, std::string const& msg);
     void warn(qpdf_offset_t offset, std::string const& msg) const;
     void warn(std::string const& msg) const;
     void warn(QPDFExc const&) const;
 WARNING: issue-146.pdf: file is damaged
 WARNING: issue-146.pdf: can't find startxref
 WARNING: issue-146.pdf: Attempting to reconstruct cross-reference table
-WARNING: issue-146.pdf (trailer, offset 695): limits error: ignoring excessively deeply nested data structure
+WARNING: issue-146.pdf (trailer, offset 695): limits error(parser-max-nesting): ignoring excessively deeply nested data structure
 WARNING: issue-146.pdf (object 1 0, offset 92): expected endobj
 WARNING: issue-146.pdf (object 7 0, offset 146): unknown token while reading object; treating as null
 WARNING: issue-146.pdf (object 7 0, offset 168): expected endobj
-WARNING: issue-202.pdf (trailer, offset 55770): limits error: ignoring excessively deeply nested data structure
+WARNING: issue-202.pdf (trailer, offset 55770): limits error(parser-max-nesting): ignoring excessively deeply nested data structure
 WARNING: issue-202.pdf: file is damaged
 WARNING: issue-202.pdf (offset 54769): expected trailer dictionary
 WARNING: issue-202.pdf: Attempting to reconstruct cross-reference table
-WARNING: issue-202.pdf (trailer, offset 55770): limits error: ignoring excessively deeply nested data structure
+WARNING: issue-202.pdf (trailer, offset 55770): limits error(parser-max-nesting): ignoring excessively deeply nested data structure
 WARNING: issue-202.pdf (object 222 0, offset 50101): dictionary has duplicated key /Creator; last occurrence overrides earlier ones
 WARNING: issue-202.pdf (object 222 0, offset 50101): dictionary has duplicated key /Producer; last occurrence overrides earlier ones
 WARNING: issue-202.pdf: unable to find trailer dictionary while recovering damaged file