Better diagnostics when --pages is not closed (fixes #555)

Jay Berkenbilt
1 parent f545f8b0
Showing 4 changed files with 71 additions and 24 deletions
ChangeLog
qpdf/qpdf.cc
qpdf/qpdf.testcov
qpdf/qtest/qpdf.test
+2021-11-02  Jay Berkenbilt  <ejb@ql.org>
+
+	* Improve error reporting when someone forgets the -- after
+	--pages. Fixes #555.
+
 2021-05-12  Jay Berkenbilt  <ejb@ql.org>
 	* Bug fix: ensure we don't overflow any string bounds while
@@ -738,6 +738,21 @@ static void parse_object_id(std::string const&amp; objspec,
     }
 }
+static bool file_exists(char const* filename)
+{
+    try
+    {
+        fclose(QUtil::safe_fopen(filename, "rb"));
+        return true;
+    }
+    catch (std::runtime_error&)
+    {
+        // can't open the file
+    }
+    return false;
+}
+
+
 // This is not a general-purpose argument parser. It is tightly
 // crafted to work with qpdf. qpdf's command-line syntax is very
 // complex because of its long history, and it doesn't really follow
@@ -2080,6 +2095,10 @@ void
 ArgParser::argPages()
 {
     ++cur_arg;
+    if (! o.page_specs.empty())
+    {
+        usage("the --pages may only be specified one time");
+    }
     o.page_specs = parsePagesOptions();
     if (o.page_specs.empty())
     {
@@ -2937,19 +2956,15 @@ ArgParser::handleArgFileArguments()
         char* argfile = 0;
         if ((strlen(argv[i]) > 1) && (argv[i][0] == '@'))
         {
-            try
+            argfile = 1 + argv[i];
+            if (strcmp(argfile, "-") != 0)
             {
-                argfile = 1 + argv[i];
-                if (strcmp(argfile, "-") != 0)
+                if (! file_exists(argfile))
                 {
-                    fclose(QUtil::safe_fopen(argfile, "rb"));
+                    // The file's not there; treating as regular option
+                    argfile = nullptr;
                 }
             }
-            catch (std::runtime_error&)
-            {
-                // The file's not there; treating as regular option
-                argfile = 0;
-            }
         }
         if (argfile)
         {
@@ -3220,6 +3235,16 @@ ArgParser::parseNumrange(char const* range, int max, bool throw_error)
 std::vector<PageSpec>
 ArgParser::parsePagesOptions()
 {
+    auto check_unclosed = [this](char const* arg, int n) {
+        if ((strlen(arg) > 0) && (arg[0] == '-'))
+        {
+            // A common error is to forget to close --pages with --,
+            // so catch this as special case
+            QTC::TC("qpdf", "check unclosed --pages", n);
+            usage("the --pages option must be terminated with -- by itself");
+        }
+    };
+
     std::vector<PageSpec> result;
     while (1)
     {
@@ -3234,6 +3259,10 @@ ArgParser::parsePagesOptions()
         char const* file = argv[cur_arg++];
         char const* password = 0;
         char const* range = argv[cur_arg++];
+        if (! file_exists(file))
+        {
+            check_unclosed(file, 0);
+        }
         if (strncmp(range, "--password=", 11) == 0)
         {
             // Oh, that's the password, not the range
@@ -3263,23 +3292,20 @@ ArgParser::parsePagesOptions()
             catch (std::runtime_error& e1)
             {
                 // The range is invalid.  Let's see if it's a file.
-                try
+                range_omitted = true;
+                if (strcmp(range, ".") == 0)
                 {
-                    if (strcmp(range, ".") == 0)
-                    {
-                        // "." means the input file.
-                        QTC::TC("qpdf", "qpdf pages range omitted with .");
-                    }
-                    else
-                    {
-                        fclose(QUtil::safe_fopen(range, "rb"));
-                        QTC::TC("qpdf", "qpdf pages range omitted in middle");
-                        // Yup, it's a file.
-                    }
-                    range_omitted = true;
+                    // "." means the input file.
+                    QTC::TC("qpdf", "qpdf pages range omitted with .");
                 }
-                catch (std::runtime_error&)
+                else if (file_exists(range))
+                {
+                    QTC::TC("qpdf", "qpdf pages range omitted in middle");
+                    // Yup, it's a file.
+                }
+                else
                 {
+                    check_unclosed(range, 1);
                     // Give the range error
                     usage(e1.what());
                 }
@@ -594,3 +594,4 @@ qpdf copy fields non-first from orig 0
 QPDF resolve duplicated page in insert 0
 QPDFWriter preserve object streams 1
 QPDFWriter exclude from object stream 0
+check unclosed --pages 1
@@ -139,7 +139,7 @@ foreach my $c (@completion_tests)
 show_ntests();
 # ----------
 $td->notify("--- Argument Parsing ---");
-$n_tests += 6;
+$n_tests += 9;
 $td->runtest("required argument",
              {$td->COMMAND => "qpdf --password minimal.pdf"},
@@ -171,6 +171,21 @@ $td-&gt;runtest(&quot;extra overlay filename&quot;,
              {$td->REGEXP => ".*overlay file already specified.*",
                   $td->EXIT_STATUS => 2},
              $td->NORMALIZE_NEWLINES);
+$td->runtest("multiple pages options",
+             {$td->COMMAND => "qpdf --pages . -- --pages . --"},
+             {$td->REGEXP => ".*--pages may only be specified one time.*",
+                  $td->EXIT_STATUS => 2},
+             $td->NORMALIZE_NEWLINES);
+$td->runtest("bad numeric range detects unclosed --pages",
+             {$td->COMMAND => "qpdf --pages . --pages . --"},
+             {$td->REGEXP => ".*--pages option must be terminated with --.*",
+                  $td->EXIT_STATUS => 2},
+             $td->NORMALIZE_NEWLINES);
+$td->runtest("bad file detected as unclosed --pages",
+             {$td->COMMAND => "qpdf --pages . 1 --xyz out"},
+             {$td->REGEXP => ".*--pages option must be terminated with --.*",
+                  $td->EXIT_STATUS => 2},
+             $td->NORMALIZE_NEWLINES);
 show_ntests();
 # ----------