[PATCH 0/5] Fix for 'list task 123' (PR cli/28668)

[PATCH 0/5] Fix for 'list task 123' (PR cli/28668)

[Andrew Burgess]

This is my proposed fix for issue PR cli/28668.

All feedback welcome.

Thanks,
Andrew


---

Andrew Burgess (5):
  gdb: update the comment on string_to_event_location
  gdb: add empty string check in parse_linespec
  gdb/testsuite: move linespec test into gdb.linespec/ directory
  gdb: handle calls to list command passing only a linespec condition
  gdb: handle calls to edit command passing only a linespec condition

 gdb/cli/cli-cmds.c                            | 16 +++++++++++++---
 gdb/linespec.c                                | 14 +++++++-------
 gdb/location.h                                |  9 ++++-----
 .../linespecs.exp => gdb.linespec/errors.exp} | 19 ++++++++++++++++++-
 4 files changed, 42 insertions(+), 16 deletions(-)
 rename gdb/testsuite/{gdb.base/linespecs.exp => gdb.linespec/errors.exp} (69%)

-- 
2.25.4

[PATCH 1/5] gdb: update the comment on string_to_event_location

[Andrew Burgess]

The comment on string_to_event_location is (I believe) out of date.
This commit fixes the two issues I see:

  1. This function can't return NULL any more.  The implementation
  calls string_to_explicit_location which can return NULL, but if this
  is the case we then call string_to_event_location_basic, which I
  don't believe can ever return NULL.

  2. I've removed the mention that the returned string is malloc'd,
  though this is true, now that we return a managed pointer, I believe
  the source of the memory allocation is irrelevant, and so, shouldn't
  be discussed in the header comment.

There should be no user visible changes after this commit.
---
 gdb/location.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/location.h b/gdb/location.h
index 0c37784d59e..b999ef2c5f7 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -211,12 +211,11 @@ extern event_location_up
   copy_event_location (const struct event_location *src);
 
 /* Attempt to convert the input string in *ARGP into an event_location.
-   ARGP is advanced past any processed input.  Returns an event_location
-   (malloc'd) if an event location was successfully found in *ARGP,
-   NULL otherwise.
+   ARGP is advanced past any processed input.  Always returns a non-nullptr
+   event_location unique pointer object.
 
-   This function may call error() if *ARGP looks like properly formed,
-   but invalid, input, e.g., if it is called with missing argument parameters
+   This function may call error() if *ARGP looks like properly formed, but
+   invalid, input, e.g., if it is called with missing argument parameters
    or invalid options.
 
    This function is intended to be used by CLI commands and will parse
-- 
2.25.4

[PATCH 2/5] gdb: add empty string check in parse_linespec

[Andrew Burgess]

If parse_linespec (linespec.c) is passed ARG as an empty string then
we end up calling `strchr (linespec_quote_characters, '\0')`, which
will return a pointer to the `\0` at the end of
linespec_quote_characters.  This then results in GDB calling
skip_quote_char with `ARG + 1`, which is undefined behaviour (as ARG
only contained a single character, the '\0').

Fix this by checking for the first character of ARG being '\0' before
the call to strchr.

I have additionally added an assertion that ARG can't itself be
nullptr, as calling is_ada_operator with nullptr can end up calling
'startswith' on the nullptr, which is undefined behaviour.

Finally, I moved the declaration of TOKEN into the body of
parse_linespec, to where TOKEN is defined.

This patch came about while I was working on a fix that is not yet in
GDB, I don't believe that current GDB hits any of these problems.
---
 gdb/linespec.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index 56bfaede9d9..0d02e41377d 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2524,14 +2524,15 @@ convert_explicit_location_to_sals (struct linespec_state *self,
    if no file is validly specified.  Callers must check that.
    Also, the line number returned may be invalid.  */
 
-/* Parse the linespec in ARG.  MATCH_TYPE indicates how function names
-   should be matched.  */
+/* Parse the linespec in ARG, which must not be nullptr.  MATCH_TYPE
+   indicates how function names should be matched.  */
 
 static std::vector<symtab_and_line>
 parse_linespec (linespec_parser *parser, const char *arg,
 		symbol_name_match_type match_type)
 {
-  linespec_token token;
+  gdb_assert (arg != nullptr);
+
   struct gdb_exception file_exception;
 
   /* A special case to start.  It has become quite popular for
@@ -2540,11 +2541,10 @@ parse_linespec (linespec_parser *parser, const char *arg,
   parser->is_quote_enclosed = 0;
   if (parser->completion_tracker == NULL
       && !is_ada_operator (arg)
+      && *arg != '\0'
       && strchr (linespec_quote_characters, *arg) != NULL)
     {
-      const char *end;
-
-      end = skip_quote_char (arg + 1, *arg);
+      const char *end = skip_quote_char (arg + 1, *arg);
       if (end != NULL && is_closing_quote_enclosed (end))
 	{
 	  /* Here's the special case.  Skip ARG past the initial
@@ -2585,7 +2585,7 @@ parse_linespec (linespec_parser *parser, const char *arg,
   /* Start parsing.  */
 
   /* Get the first token.  */
-  token = linespec_lexer_consume_token (parser);
+  linespec_token token = linespec_lexer_consume_token (parser);
 
   /* It must be either LSTOKEN_STRING or LSTOKEN_NUMBER.  */
   if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
-- 
2.25.4

[PATCH 3/5] gdb/testsuite: move linespec test into gdb.linespec/ directory

[Andrew Burgess]

The gdb.base/linespecs.exp test should really live in the gdb.linespec
directory, so lets move it there.

As we already have gdb.linespec/linespec.exp, I've renamed the test to
gdb.linespec/errors.exp, as this better reflects what the test is
actually checking.
---
 .../{gdb.base/linespecs.exp => gdb.linespec/errors.exp}         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename gdb/testsuite/{gdb.base/linespecs.exp => gdb.linespec/errors.exp} (94%)

diff --git a/gdb/testsuite/gdb.base/linespecs.exp b/gdb/testsuite/gdb.linespec/errors.exp
similarity index 94%
rename from gdb/testsuite/gdb.base/linespecs.exp
rename to gdb/testsuite/gdb.linespec/errors.exp
index 4f171d7ae4e..6f11ad8575c 100644
--- a/gdb/testsuite/gdb.base/linespecs.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -18,7 +18,7 @@
 # We don't currently need our own test case for testing, so grab
 # another one.
 
-if {[prepare_for_testing "failed to prepare" linespecs memattr.c]} {
+if {[prepare_for_testing "failed to prepare" linespecs keywords.c]} {
   return -1
 }
 
-- 
2.25.4

[PATCH 4/5] gdb: handle calls to list command passing only a linespec condition

[Andrew Burgess]

In PR cli/28668, 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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28668
---
 gdb/cli/cli-cmds.c                    |  9 +++++++++
 gdb/testsuite/gdb.linespec/errors.exp | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 3fe47940076..857cc8ab45d 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);
diff --git a/gdb/testsuite/gdb.linespec/errors.exp b/gdb/testsuite/gdb.linespec/errors.exp
index 6f11ad8575c..a46558e7d74 100644
--- a/gdb/testsuite/gdb.linespec/errors.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -27,3 +27,13 @@ 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_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\\."
-- 
2.25.4

[PATCH 5/5] gdb: handle calls to edit command passing only a linespec condition

[Andrew Burgess]

While working on the previous commit to fix PR cli/28668, I noticed
that the 'edit' command would suffer from the same problem.  That is,
something like:

  (gdb) edit task 123

would cause GDB to break.  For a full explanation of what's going on
here, see the commit message for the previous commit.

As with the previous commit, this issue can be prevented by detecting,
and throwing, a junk at the end of the line error earlier, before
calling decode_line_1.

So, that's what this commit does.  I've also added some tests for this
issue.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28668
---
 gdb/cli/cli-cmds.c                    |  7 ++++---
 gdb/testsuite/gdb.linespec/errors.exp | 23 +++++++++++++++--------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 857cc8ab45d..d577acf01e4 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -968,6 +968,10 @@ edit_command (const char *arg, int from_tty)
       arg1 = arg;
       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 = decode_line_1 (location.get (),
 							 DECODE_LINE_LIST_MODE,
 							 NULL, NULL, 0);
@@ -987,9 +991,6 @@ edit_command (const char *arg, int from_tty)
 
       sal = sals[0];
 
-      if (*arg1)
-	error (_("Junk at end of line specification."));
-
       /* If line was specified by address, first print exactly which
 	 line, and which file.  In this case, sal.symtab == 0 means
 	 address is outside of all known source files, not that user
diff --git a/gdb/testsuite/gdb.linespec/errors.exp b/gdb/testsuite/gdb.linespec/errors.exp
index a46558e7d74..496612d75ea 100644
--- a/gdb/testsuite/gdb.linespec/errors.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -29,11 +29,18 @@ 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_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\\."
+save_vars { env(EDITOR) } {
+    setenv EDITOR true
+
+    foreach cmd {list edit} {
+	gdb_test "${cmd} task 123" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} if (0)" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} thread 1" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} -force-condition" \
+	    "Junk at end of line specification\\."
+    }
+}
+
-- 
2.25.4

Re: [PATCH 0/5] Fix for 'list task 123' (PR cli/28668)

[Andrew Burgess]

Darn!  Despite working in a branch called gdb-bug-28665, I managed to
place references to bug 28668 throughout this series.

My apologies.

Here's an actual link to the bug I'm fixing:

  https://sourceware.org/bugzilla/show_bug.cgi?id=28665

I've locally updated all references to the incorrect bug number, and
fixed all the links.

Again, apologies,

Andrew


* Andrew Burgess <aburgess@redhat.com> [2021-12-07 23:13:40 +0000]:

> This is my proposed fix for issue PR cli/28668.
> 
> All feedback welcome.
> 
> Thanks,
> Andrew
> 
> 
> ---
> 
> Andrew Burgess (5):
>   gdb: update the comment on string_to_event_location
>   gdb: add empty string check in parse_linespec
>   gdb/testsuite: move linespec test into gdb.linespec/ directory
>   gdb: handle calls to list command passing only a linespec condition
>   gdb: handle calls to edit command passing only a linespec condition
> 
>  gdb/cli/cli-cmds.c                            | 16 +++++++++++++---
>  gdb/linespec.c                                | 14 +++++++-------
>  gdb/location.h                                |  9 ++++-----
>  .../linespecs.exp => gdb.linespec/errors.exp} | 19 ++++++++++++++++++-
>  4 files changed, 42 insertions(+), 16 deletions(-)
>  rename gdb/testsuite/{gdb.base/linespecs.exp => gdb.linespec/errors.exp} (69%)
> 
> -- 
> 2.25.4
> 

[PATCHv2 0/6] Fix for 'list task 123' (PR cli/28665)

[Andrew Burgess]

Since v1:

  - Replaces all references to PR gdb/28668 with PR gdb/28665 as
    that's the bug I was actually fixing,

  - Extended patch #4 to also fix PR gdb/28797,

  - Added patch #6 to test an additional situation that I don't think
    is currently covered by the gdb testsuite.

---

Andrew Burgess (6):
  gdb: update the comment on string_to_event_location
  gdb: add empty string check in parse_linespec
  gdb/testsuite: move linespec test into gdb.linespec/ directory
  gdb: handle calls to list command passing only a linespec condition
  gdb: handle calls to edit command passing only a linespec condition
  gdb: test to check one aspect of the linespec parsing code

 gdb/cli/cli-cmds.c                            | 19 +++++++++++++++---
 gdb/linespec.c                                | 14 ++++++-------
 gdb/location.c                                |  5 +++++
 gdb/location.h                                |  9 ++++-----
 gdb/testsuite/gdb.base/break.exp              |  5 +++++
 .../linespecs.exp => gdb.linespec/errors.exp} | 20 ++++++++++++++++++-
 6 files changed, 56 insertions(+), 16 deletions(-)
 rename gdb/testsuite/{gdb.base/linespecs.exp => gdb.linespec/errors.exp} (66%)

-- 
2.25.4

[PATCHv2 1/6] gdb: update the comment on string_to_event_location

[Andrew Burgess]

The comment on string_to_event_location is (I believe) out of date.
This commit fixes the two issues I see:

  1. This function can't return NULL any more.  The implementation
  calls string_to_explicit_location which can return NULL, but if this
  is the case we then call string_to_event_location_basic, which I
  don't believe can ever return NULL.

  2. I've removed the mention that the returned string is malloc'd,
  though this is true, now that we return a managed pointer, I believe
  the source of the memory allocation is irrelevant, and so, shouldn't
  be discussed in the header comment.

There should be no user visible changes after this commit.
---
 gdb/location.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gdb/location.h b/gdb/location.h
index 848f6458e5e..ff21c1c21cc 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -205,12 +205,11 @@ extern event_location_up
   copy_event_location (const struct event_location *src);
 
 /* Attempt to convert the input string in *ARGP into an event_location.
-   ARGP is advanced past any processed input.  Returns an event_location
-   (malloc'd) if an event location was successfully found in *ARGP,
-   NULL otherwise.
+   ARGP is advanced past any processed input.  Always returns a non-nullptr
+   event_location unique pointer object.
 
-   This function may call error() if *ARGP looks like properly formed,
-   but invalid, input, e.g., if it is called with missing argument parameters
+   This function may call error() if *ARGP looks like properly formed, but
+   invalid, input, e.g., if it is called with missing argument parameters
    or invalid options.
 
    This function is intended to be used by CLI commands and will parse
-- 
2.25.4

[PATCHv2 2/6] gdb: add empty string check in parse_linespec

[Andrew Burgess]

If parse_linespec (linespec.c) is passed ARG as an empty string then
we end up calling `strchr (linespec_quote_characters, '\0')`, which
will return a pointer to the '\0' at the end of
linespec_quote_characters.  This then results in GDB calling
skip_quote_char with `ARG + 1`, which is undefined behaviour (as ARG
only contained a single character, the '\0').

Fix this by checking for the first character of ARG being '\0' before
the call to strchr.

I have additionally added an assertion that ARG can't itself be
nullptr, as calling is_ada_operator with nullptr can end up calling
'startswith' on the nullptr, which is undefined behaviour.

Finally, I moved the declaration of TOKEN into the body of
parse_linespec, to where TOKEN is defined.

This patch came about while I was working on fixes for PR cli/28665
and PR gdb/28797.  The actual fixes for these two issues will be in a
later commit in this series, but, with this patch in place, both of
the above bugs would hit the new assertion rather than accessing
invalid memory and crashing.  The '\0' check is not currently ever
hit, but just makes the code a little safer.

Because this patch only changes the nature of the failure for the
above two bugs, there's no tests here.  A later commit will fix the
above two issues, at which point I'll add some tests.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28665
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28797
---
 gdb/linespec.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/linespec.c b/gdb/linespec.c
index b24cf30dc71..27ceaa41adb 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2522,14 +2522,15 @@ convert_explicit_location_to_sals (struct linespec_state *self,
    if no file is validly specified.  Callers must check that.
    Also, the line number returned may be invalid.  */
 
-/* Parse the linespec in ARG.  MATCH_TYPE indicates how function names
-   should be matched.  */
+/* Parse the linespec in ARG, which must not be nullptr.  MATCH_TYPE
+   indicates how function names should be matched.  */
 
 static std::vector<symtab_and_line>
 parse_linespec (linespec_parser *parser, const char *arg,
 		symbol_name_match_type match_type)
 {
-  linespec_token token;
+  gdb_assert (arg != nullptr);
+
   struct gdb_exception file_exception;
 
   /* A special case to start.  It has become quite popular for
@@ -2538,11 +2539,10 @@ parse_linespec (linespec_parser *parser, const char *arg,
   parser->is_quote_enclosed = 0;
   if (parser->completion_tracker == NULL
       && !is_ada_operator (arg)
+      && *arg != '\0'
       && strchr (linespec_quote_characters, *arg) != NULL)
     {
-      const char *end;
-
-      end = skip_quote_char (arg + 1, *arg);
+      const char *end = skip_quote_char (arg + 1, *arg);
       if (end != NULL && is_closing_quote_enclosed (end))
 	{
 	  /* Here's the special case.  Skip ARG past the initial
@@ -2583,7 +2583,7 @@ parse_linespec (linespec_parser *parser, const char *arg,
   /* Start parsing.  */
 
   /* Get the first token.  */
-  token = linespec_lexer_consume_token (parser);
+  linespec_token token = linespec_lexer_consume_token (parser);
 
   /* It must be either LSTOKEN_STRING or LSTOKEN_NUMBER.  */
   if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$')
-- 
2.25.4

[PATCHv2 3/6] gdb/testsuite: move linespec test into gdb.linespec/ directory

[Andrew Burgess]

The gdb.base/linespecs.exp test should really live in the gdb.linespec
directory, so lets move it there.

As we already have gdb.linespec/linespec.exp, I've renamed the test to
gdb.linespec/errors.exp, as this better reflects what the test is
actually checking.

Finally, the test script doesn't have its own source file, it was
reusing a random other source file, gdb.base/memattr.c.  Now the tests
script is in gdb.linespec/, I've updated the test to use a different
source file from that directory.
---
 .../{gdb.base/linespecs.exp => gdb.linespec/errors.exp}         | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename gdb/testsuite/{gdb.base/linespecs.exp => gdb.linespec/errors.exp} (94%)

diff --git a/gdb/testsuite/gdb.base/linespecs.exp b/gdb/testsuite/gdb.linespec/errors.exp
similarity index 94%
rename from gdb/testsuite/gdb.base/linespecs.exp
rename to gdb/testsuite/gdb.linespec/errors.exp
index bc16863a3e3..0baef1891a7 100644
--- a/gdb/testsuite/gdb.base/linespecs.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -18,7 +18,7 @@
 # We don't currently need our own test case for testing, so grab
 # another one.
 
-if {[prepare_for_testing "failed to prepare" linespecs memattr.c]} {
+if {[prepare_for_testing "failed to prepare" linespecs keywords.c]} {
   return -1
 }
 
-- 
2.25.4

[PATCHv2 4/6] gdb: handle calls to list command passing only a linespec condition

[Andrew Burgess]

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
---
 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\\."
-- 
2.25.4

[PATCHv2 5/6] gdb: handle calls to edit command passing only a linespec condition

[Andrew Burgess]

While working on the previous commit to fix PR cli/28665, I noticed
that the 'edit' command would suffer from the same problem.  That is,
something like:

  (gdb) edit task 123

would cause GDB to break.  For a full explanation of what's going on
here, see the commit message for the previous commit.

As with the previous commit, this issue can be prevented by detecting,
and throwing, a junk at the end of the line error earlier, before
calling decode_line_1.

So, that's what this commit does.  I've also added some tests for this
issue.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28665
---
 gdb/cli/cli-cmds.c                    |  7 ++++---
 gdb/testsuite/gdb.linespec/errors.exp | 26 ++++++++++++++++----------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 648005ffdfe..1d14b8e4527 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -968,6 +968,10 @@ edit_command (const char *arg, int from_tty)
       arg1 = arg;
       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 = decode_line_1 (location.get (),
 							 DECODE_LINE_LIST_MODE,
 							 NULL, NULL, 0);
@@ -987,9 +991,6 @@ edit_command (const char *arg, int from_tty)
 
       sal = sals[0];
 
-      if (*arg1)
-	error (_("Junk at end of line specification."));
-
       /* If line was specified by address, first print exactly which
 	 line, and which file.  In this case, sal.symtab == 0 means
 	 address is outside of all known source files, not that user
diff --git a/gdb/testsuite/gdb.linespec/errors.exp b/gdb/testsuite/gdb.linespec/errors.exp
index e258f6bb98c..c895e25da03 100644
--- a/gdb/testsuite/gdb.linespec/errors.exp
+++ b/gdb/testsuite/gdb.linespec/errors.exp
@@ -29,13 +29,19 @@ 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\\."
+save_vars { env(EDITOR) } {
+    setenv EDITOR true
+
+    foreach cmd {list edit} {
+	gdb_test "${cmd} task 123" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} if (0)" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} thread 1" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} -force-condition" \
+	    "Junk at end of line specification\\."
+	gdb_test "${cmd} ,," \
+	    "Junk at end of line specification\\."
+    }
+}
-- 
2.25.4

[PATCHv2 6/6] gdb: test to check one aspect of the linespec parsing code

[Andrew Burgess]

While working on the fix for PR cli/28665 (see previous couple of
commits), I was playing with making a change in the linespec parsing
code.  Specifically, I was thinking about whether the spec_string for
LINESPEC_LOCATION locations should ever be nullptr.

I made a change to prevent the spec_string from ever being nullptr,
tested gdb, and saw no regressions.

However, as part of this work I was reviewing how the breakpoint code
handles this case (spec_string being nullptr), and spotted that in
parse_breakpoint_sals the nullptr case is specifically handled, so
changing this should have caused a regression.  But I didn't see one.

So, this commit adds a comment in location.c mentioning that the
nullptr case is (a) not an oversight, and (b) is required.  Then I add
a new test to gdb.base/break.exp that ensures a change in this area
will cause a regression.

This test passes on current gdb, but with my modified (and broken)
gdb, the test would fail.
---
 gdb/location.c                   | 5 +++++
 gdb/testsuite/gdb.base/break.exp | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/gdb/location.c b/gdb/location.c
index 299ef7ecadd..0459980ab8c 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -134,6 +134,11 @@ struct event_location_linespec : public event_location
 
 	linespec_lex_to_end (linespec);
 	p = remove_trailing_whitespace (orig, *linespec);
+
+	/* If there is no valid linespec then this will leave the
+	   spec_string as nullptr.  This behaviour is relied on in the
+	   breakpoint setting code, where spec_string being nullptr means
+	   to use the default breakpoint location.  */
 	if ((p - orig) > 0)
 	  linespec_location.spec_string = savestring (orig, p - orig);
       }
diff --git a/gdb/testsuite/gdb.base/break.exp b/gdb/testsuite/gdb.base/break.exp
index 39a8f32e888..2c939ada14a 100644
--- a/gdb/testsuite/gdb.base/break.exp
+++ b/gdb/testsuite/gdb.base/break.exp
@@ -520,6 +520,11 @@ gdb_test "break" \
     "Note: breakpoints \[0-9\]*, \[0-9\]* and \[0-9\]* also set at .*Breakpoint \[0-9\]*.*" \
     "break on default location, 4th time"
 
+# Check setting a breakpoint at the default location with a condition attached.
+gdb_test "break if (1)" \
+    "Note: breakpoints \[0-9\]*, \[0-9\]*, \[0-9\]* and \[0-9\]* also set at .*Breakpoint \[0-9\]*.*" \
+    "break on the default location, 5th time, but with a condition"
+
 # Verify that a "silent" breakpoint can be set, and that GDB is indeed
 # "silent" about its triggering.
 #
-- 
2.25.4

Re: [PATCHv2 0/6] Fix for 'list task 123' (PR cli/28665)

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Since v1:
Andrew>   - Replaces all references to PR gdb/28668 with PR gdb/28665 as
Andrew>     that's the bug I was actually fixing,
Andrew>   - Extended patch #4 to also fix PR gdb/28797,
Andrew>   - Added patch #6 to test an additional situation that I don't think
Andrew>     is currently covered by the gdb testsuite.

I read through these and didn't have any comments.
Thank you for doing this.

Tom

Re: [PATCHv2 0/6] Fix for 'list task 123' (PR cli/28665)

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> Since v1:
> Andrew>   - Replaces all references to PR gdb/28668 with PR gdb/28665 as
> Andrew>     that's the bug I was actually fixing,
> Andrew>   - Extended patch #4 to also fix PR gdb/28797,
> Andrew>   - Added patch #6 to test an additional situation that I don't think
> Andrew>     is currently covered by the gdb testsuite.
>
> I read through these and didn't have any comments.
> Thank you for doing this.

Thanks, now pushed.

Andrew