Fix string bounds checking in completion code (fixes #441)

7 files changed, 43 insertions, 8 deletions

diff --git a/ChangeLog b/ChangeLog
index 42e80fe1..3c568a3e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2021-05-12  Jay Berkenbilt  <ejb@ql.org>
+
+	* Bug fix: ensure we don't overflow any string bounds while
+	handling completion, even when we are given bogus input values.
+	Fixes #441.
+
 2021-05-09  Jay Berkenbilt  <ejb@ql.org>
 
 	* Improve performance of preservation of object streams by
diff --git a/qpdf/qpdf.cc b/qpdf/qpdf.cc
index cf50efdc..7e450cf8 100644
--- a/qpdf/qpdf.cc
+++ b/qpdf/qpdf.cc
@@ -3469,16 +3469,27 @@ ArgParser::checkCompletion()
 {
     // See if we're being invoked from bash completion.
     std::string bash_point_env;
-    if (QUtil::get_env("COMP_LINE", &bash_line) &&
-        QUtil::get_env("COMP_POINT", &bash_point_env))
+    // On Windows with mingw, there have been times when there appears
+    // to be no way to distinguish between an empty environment
+    // variable and an unset variable. There are also conditions under
+    // which bash doesn't set COMP_LINE. Therefore, enter this logic
+    // if either COMP_LINE or COMP_POINT are set. They will both be
+    // set together under ordinary circumstances.
+    bool got_line = QUtil::get_env("COMP_LINE", &bash_line);
+    bool got_point = QUtil::get_env("COMP_POINT", &bash_point_env);
+    if (got_line || got_point)
     {
         size_t p = QUtil::string_to_uint(bash_point_env.c_str());
-        if ((p > 0) && (p <= bash_line.length()))
+        if (p < bash_line.length())
         {
             // Truncate the line. We ignore everything at or after the
             // cursor for completion purposes.
             bash_line = bash_line.substr(0, p);
         }
+        if (p > bash_line.length())
+        {
+            p = bash_line.length();
+        }
         // Set bash_cur and bash_prev based on bash_line rather than
         // relying on argv. This enables us to use bashcompinit to get
         // completion in zsh too since bashcompinit sets COMP_LINE and
@@ -3489,8 +3500,9 @@ ArgParser::checkCompletion()
         // for the first separator. bash_cur is everything after the
         // last separator, possibly empty.
         char sep(0);
-        while (--p > 0)
+        while (p > 0)
         {
+            --p;
             char ch = bash_line.at(p);
             if ((ch == ' ') || (ch == '=') || (ch == ':'))
             {
@@ -3498,7 +3510,10 @@ ArgParser::checkCompletion()
                 break;
             }
         }
-        bash_cur = bash_line.substr(1+p, std::string::npos);
+        if (1+p <= bash_line.length())
+        {
+            bash_cur = bash_line.substr(1+p, std::string::npos);
+        }
         if ((sep == ':') || (sep == '='))
         {
             // Bash sets prev to the non-space separator if any.
@@ -3512,8 +3527,9 @@ ArgParser::checkCompletion()
             // Go back to the last separator and set prev based on
             // that.
             size_t p1 = p;
-            while (--p1 > 0)
+            while (p1 > 0)
             {
+                --p1;
                 char ch = bash_line.at(p1);
                 if ((ch == ' ') || (ch == ':') || (ch == '='))
                 {
diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test
index 5a978d30..6350c045 100644
--- a/qpdf/qtest/qpdf.test
+++ b/qpdf/qtest/qpdf.test
@@ -86,6 +86,10 @@ $td->runtest("UTF-16 encoding errors",
 	     $td->NORMALIZE_NEWLINES);
 
 my @completion_tests = (
+    ['', 0, 'bad-input-1'],
+    ['', 1, 'bad-input-2'],
+    ['', 2, 'bad-input-3'],
+    ['qpdf', 2, 'bad-input-4'],
     ['qpdf ', undef, 'top'],
     ['qpdf -', undef, 'top-arg'],
     ['qpdf --enc', undef, 'enc'],
@@ -5229,8 +5233,13 @@ sub bash_completion
         $point = length($line);
     }
     my $before_point = substr($line, 0, $point);
-    $before_point =~ m/^(.*)([ =])([^= ]*)$/ or die;
-    my ($first, $sep, $cur) = ($1, $2, $3);
+    my $first = '';
+    my $sep = '';
+    my $cur = '';
+    if ($before_point =~ m/^(.*)([ =])([^= ]*)$/)
+    {
+        ($first, $sep, $cur) = ($1, $2, $3);
+    }
     my $prev = ($sep eq '=' ? $sep : $first);
     $prev =~ s/.* (\S+)$/$1/;
     my $this = $first;
diff --git a/qpdf/qtest/qpdf/completion-bad-input-1.out b/qpdf/qtest/qpdf/completion-bad-input-1.out
new file mode 100644
index 00000000..cdf4cb4f
--- /dev/null
+++ b/qpdf/qtest/qpdf/completion-bad-input-1.out
@@ -0,0 +1 @@
+!
diff --git a/qpdf/qtest/qpdf/completion-bad-input-2.out b/qpdf/qtest/qpdf/completion-bad-input-2.out
new file mode 100644
index 00000000..cdf4cb4f
--- /dev/null
+++ b/qpdf/qtest/qpdf/completion-bad-input-2.out
@@ -0,0 +1 @@
+!
diff --git a/qpdf/qtest/qpdf/completion-bad-input-3.out b/qpdf/qtest/qpdf/completion-bad-input-3.out
new file mode 100644
index 00000000..cdf4cb4f
--- /dev/null
+++ b/qpdf/qtest/qpdf/completion-bad-input-3.out
@@ -0,0 +1 @@
+!
diff --git a/qpdf/qtest/qpdf/completion-bad-input-4.out b/qpdf/qtest/qpdf/completion-bad-input-4.out
new file mode 100644
index 00000000..cdf4cb4f
--- /dev/null
+++ b/qpdf/qtest/qpdf/completion-bad-input-4.out
@@ -0,0 +1 @@
+!