From: Andrew Burgess <aburgess@redhat.com>
To: gdb-patches@sourceware.org
Cc: Andrew Burgess <aburgess@redhat.com>
Subject: [PATCH 4/5] gdb: handle calls to list command passing only a linespec condition
Date: Tue, 7 Dec 2021 23:13:44 +0000 [thread overview]
Message-ID: <ff1c92ff0d70c59e50b7667e80aec1d56b990c6f.1638918701.git.aburgess@redhat.com> (raw)
In-Reply-To: <cover.1638918701.git.aburgess@redhat.com>
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
next prev parent reply other threads:[~2021-12-07 23:13 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-07 23:13 [PATCH 0/5] Fix for 'list task 123' (PR cli/28668) Andrew Burgess
2021-12-07 23:13 ` [PATCH 1/5] gdb: update the comment on string_to_event_location Andrew Burgess
2021-12-07 23:13 ` [PATCH 2/5] gdb: add empty string check in parse_linespec Andrew Burgess
2021-12-07 23:13 ` [PATCH 3/5] gdb/testsuite: move linespec test into gdb.linespec/ directory Andrew Burgess
2021-12-07 23:13 ` Andrew Burgess [this message]
2021-12-07 23:13 ` [PATCH 5/5] gdb: handle calls to edit command passing only a linespec condition Andrew Burgess
2021-12-07 23:18 ` [PATCH 0/5] Fix for 'list task 123' (PR cli/28668) Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 0/6] Fix for 'list task 123' (PR cli/28665) Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 1/6] gdb: update the comment on string_to_event_location Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 2/6] gdb: add empty string check in parse_linespec Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 3/6] gdb/testsuite: move linespec test into gdb.linespec/ directory Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 4/6] gdb: handle calls to list command passing only a linespec condition Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 5/6] gdb: handle calls to edit " Andrew Burgess
2022-01-27 17:47 ` [PATCHv2 6/6] gdb: test to check one aspect of the linespec parsing code Andrew Burgess
2022-01-27 20:35 ` [PATCHv2 0/6] Fix for 'list task 123' (PR cli/28665) Tom Tromey
2022-02-02 16:32 ` Andrew Burgess
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ff1c92ff0d70c59e50b7667e80aec1d56b990c6f.1638918701.git.aburgess@redhat.com \
--to=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).