[binutils-gdb] gdb: handle calls to list command passing only a linespec condition

[binutils-gdb] gdb: handle calls to list command passing only a linespec condition

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3c5fcec6dccb0e598d1e64640e55d50ed3ddedb6

commit 3c5fcec6dccb0e598d1e64640e55d50ed3ddedb6
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Dec 7 14:01:23 2021 +0000

    gdb: handle calls to list command passing only a linespec condition
    
    In PR cli/28665, it was reported that GDB would crash when given a
    command like:
    
      (gdb) list task 123
    
    The problem here is that in cli/cli-cmd.c:list_command, the string
    'task 123' is passed to string_to_event_location in find a location
    specification.  However, this location parsing understands about
    breakpoint conditions, and so, will stop parsing when it sees
    something that looks like a condition, in this case, the 'task 123'
    looks like a breakpoint condition.
    
    As a result, the location we get back from string_to_event_location
    has no actual location specification attached to it.  The actual call
    path is:
    
      list_command
        string_to_event_location
          string_to_event_location_basic
            new_linespec_location
    
    In new_linespec_location we call linespec_lex_to_end, which looks at
    'task 123' and decides that there's nothing there that describes a
    location.  As such, in new_linespec_location, the spec_string field of
    the location is left as nullptr.
    
    Back in list_command we then call decode_line_1, which calls
    event_location_to_sals, which calls parse_linespec, which takes the
    spec_string we found earlier, and tries to converts this into a list
    of sals.
    
    However, parse_linespec is not intended to be passed a nullptr, for
    example, calling is_ada_operator will try to access through the
    nullptr, causing undefined behaviour.  But there are other cases
    within parse_linespec which don't expect to see a nullptr.
    
    When looking at how to fix this issue, I first considered having
    linespec_lex_to_end detect the problem.  That function understands
    when the first thing in the linespec is a condition keyword, and so,
    could throw an error saying something like: "no linespec before
    condition keyword", however, this is not going to work, at least, not
    without additional changes to GDB, it is valid to place a breakpoint
    like:
    
      (gdb) break task 123
    
    This will place a breakpoint at the current location with the
    condition 'task 123', and changing linespec_lex_to_end breaks this
    behaviour.
    
    So, next, I considered what would happen if I added a condition to an
    otherwise valid list command, this is what I see:
    
      (gdb) list file.c:1 task 123
      Junk at end of line specification.
      (gdb)
    
    So, then I wondered, could we just pull the "Junk" detection forward,
    so that we throw the error earlier, before we call decode_line_1?
    
    It turns out that yes we can.  Well, sort of.
    
    It is simpler, I think, to add a separate check into the list_command
    function, after calling string_to_event_location, but before calling
    decode_line_1.  We know when we call string_to_event_location that the
    string in question is not empty, so, after calling
    string_to_event_location, if non of the string has been consumed, then
    the content of the string must be junk - it clearly doesn't look like
    a location specification.
    
    I've reused the same "Junk at end of line specification." error for
    consistency, and added a few tests to cover this issue.
    
    While the first version of this patch was on the mailing list, a
    second bug PR gdb/28797 was raised.  This was for a very similar
    issue, but this time the problem command was:
    
      (gdb) list ,,
    
    Here the list command understands about the first comma, list can have
    two arguments separated by a comma, and the first argument can be
    missing.  So we end up trying to parse the second command "," as a
    linespec.
    
    However, in linespec_lex_to_end, we will stop parsing a linespec at a
    comma, so, in the above case we end up with an empty linespec (between
    the two commas), and, like above, this results in the spec_string
    being nullptr.
    
    As with the previous case, I've resolved this issue by adding an extra
    check for junk at the end of the line - after parsing (or failing to
    parse) the nothing between the two commas, we still have the "," left
    at the end of the list command line - when we see this we can throw
    the same "junk at the end of the line" error, and all is good.
    
    I've added tests for this case too.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28665
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28797

Diff:
---
 gdb/cli/cli-cmds.c                    | 12 ++++++++++++
 gdb/testsuite/gdb.linespec/errors.exp | 12 ++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 2f5ce3e5fff..648005ffdfe 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -1238,6 +1238,15 @@ list_command (const char *arg, int from_tty)
     {
       event_location_up location = string_to_event_location (&arg1,
 							     current_language);
+
+      /* We know that the ARG string is not empty, yet the attempt to parse
+	 a location from the string consumed no characters.  This most
+	 likely means that the first thing in ARG looks like a location
+	 condition, and so the string_to_event_location call stopped
+	 parsing.  */
+      if (arg1 == arg)
+	error (_("Junk at end of line specification."));
+
       sals = decode_line_1 (location.get (), DECODE_LINE_LIST_MODE,
 			    NULL, NULL, 0);
       filter_sals (sals);
@@ -1286,6 +1295,9 @@ list_command (const char *arg, int from_tty)
 	  event_location_up location
 	    = string_to_event_location (&arg1, current_language);
 
+	  if (*arg1)
+	    error (_("Junk at end of line specification."));
+
 	  std::vector<symtab_and_line> sals_end
 	    = (dummy_beg
 	       ? decode_line_1 (location.get (), DECODE_LINE_LIST_MODE,
diff --git a/gdb/testsuite/gdb.linespec/errors.exp b/gdb/testsuite/gdb.linespec/errors.exp
index 0baef1891a7..e258f6bb98c 100644
--- a/gdb/testsuite/gdb.linespec/errors.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -27,3 +27,15 @@ gdb_test "list c:/foo/bar/baz.c:1" "No source file named c:/foo/bar/baz.c."
 gdb_test "list c:/foo/bar/baz.c" "Function \"c:/foo/bar/baz.c\" not defined."
 gdb_test "list fooc:/foo/bar/baz.c:1" "No source file named fooc."
 gdb_test "list fooc:/foo/bar/baz.c" "No source file named fooc."
+
+# PR cli/28665, gdb/28797
+gdb_test "list task 123" \
+    "Junk at end of line specification\\."
+gdb_test "list if (0)" \
+    "Junk at end of line specification\\."
+gdb_test "list thread 1" \
+    "Junk at end of line specification\\."
+gdb_test "list -force-condition" \
+    "Junk at end of line specification\\."
+gdb_test "list ,," \
+    "Junk at end of line specification\\."