[gdb] Fix heap-buffer-overflow in args_complete_p

PR gdb/33754 reports a heap-buffer-overflow args_complete_p, while checking
the while condition:
...
  while (*input != '\0')
    {
      input = skip_spaces (input);
      ...
      ++input;
    }
...

The problem can be reproduced by calling args_complete_p (" ").  The following
happens:
- at function entry, input == " "
- the while loop is entered
- after skip_spaces, input == ""
- after the ++input at the end of the loop body, input points past the
  terminating '\0'
- while checking the while condition, *input does an out-of-bound access.

Add a unit test exercising this minimal example, fix this by checking
input after skip_spaces, and add an assert to detect the heap-buffer-overflow
without Address Sanitizer.

Another heap-buffer-overflow can be found by calling args_complete_p ("\"\\").
In this case, the following happens:
- at function entry, input == "\"\\"
- the while loop is entered
- dquote is set to true and input == "\\"
- the while loop is entered a second time
- the condition *input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr
  evaluates to true (which is not trivial to understand, because the char
  found in the string "\"\\" is '\0'), leading to two increments of input,
  again making input point past the terminating '\0'.

Fix this by checking for *(input + 1) == '\0', and likewise add a unit test.

Tested on x86_64-linux.

Approved-By: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33754

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 1a7daf1461b1d7421130c4863d4ed0d5e229ff4e..5862b3e43ec8fe01ff0c5c94e3d730ee98f67273 100644 (file)
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -56,6 +56,7 @@
 #include <optional>
 #include "source.h"
 #include "cli/cli-style.h"
+#include "gdbsupport/selftest.h"
 
 /* Local functions: */
 
@@ -131,6 +132,8 @@ args_complete_p (const std::string &args)
   while (*input != '\0')
     {
       input = skip_spaces (input);
+      if (*input == '\0')
+       break;
 
       if (squote)
        {
@@ -148,7 +151,8 @@ args_complete_p (const std::string &args)
             and we don't skip the entire '\\' then we'll only skip the
             first '\', in which case we might see the second '\' as a '\"'
             sequence, which would be wrong.  */
-         if (*input == '\\' && strchr ("\"\\", *(input + 1)) != nullptr)
+         if (*input == '\\' && *(input + 1) != '\0'
+             && strchr ("\"\\", *(input + 1)) != nullptr)
            ++input;
          /* Otherwise, just look for the closing double quote.  */
          else if (*input == '"')
@@ -162,7 +166,8 @@ args_complete_p (const std::string &args)
             a quoted argument.  The '\\' we need to skip so we don't just
             skip the first '\' and then incorrectly consider the second
             '\' are part of a '\"' or '\'' sequence.  */
-         if (*input == '\\' && strchr ("\"\\'", *(input + 1)) != nullptr)
+         if (*input == '\\' && *(input + 1) != '\0'
+             && strchr ("\"\\'", *(input + 1)) != nullptr)
            ++input;
          /* Otherwise, check for the start of a single or double quoted
             argument.  Single quotes have no special meaning on Windows
@@ -180,9 +185,32 @@ args_complete_p (const std::string &args)
       ++input;
     }
 
+  /* Check that the whole string was read, and that we haven't read past
+     '\0'.  */
+  gdb_assert (input == args.data () + args.size ());
   return (!dquote && !squote);
 }
 
+#if GDB_SELF_TEST
+namespace selftests {
+
+static void
+infcmd_args_complete_p_tests ()
+{
+  /* The " " and "\"\\" cases are regression tests for PR33754.  */
+  std::vector<std::string> complete_strings = { " " };
+  std::vector<std::string> incomplete_strings = { "\"\\" };
+
+  for (auto &s : complete_strings)
+    SELF_CHECK (args_complete_p (s));
+
+  for (auto &s : incomplete_strings)
+    SELF_CHECK (!args_complete_p (s));
+}
+
+} /* namespace selftests */
+#endif /* GDB_SELF_TEST */
+
 /* Build a complete inferior argument string (all arguments to pass to the
    inferior) and return it.  ARGS is the initial part of the inferior
    arguments string, which might be the complete inferior arguments, in
@@ -3634,4 +3662,9 @@ Show whether `finish' prints the return value."), nullptr,
                           nullptr,
                           show_print_finish,
                           &setprintlist, &showprintlist);
+
+#if GDB_SELF_TEST
+  selftests::register_test ("infcmd-args-complete-p",
+                           selftests::infcmd_args_complete_p_tests);
+#endif
 }