* [PATCH] gdb: fix post-hook execution for remote targets @ 2023-04-14 15:17 Jan Vrany 2023-05-03 14:40 ` Jan Vraný 2023-05-10 15:07 ` Tom Tromey 0 siblings, 2 replies; 7+ messages in thread From: Jan Vrany @ 2023-04-14 15:17 UTC (permalink / raw) To: gdb-patches; +Cc: Wenyan.Xin, Jan Vrany Commit b5661ff2 ("gdb: fix possible use-after-free when executing commands") attempted to fix possible use-after-free in case command redefines itself. Commit 37e5833d ("gdb: fix command lookup in execute_command ()") updated the previous fix to handle subcommands as well by using the original command string to lookup the command again after its execution. This fixed the test in gdb.base/define.exp but it turned out that it does not work (at least) for "target remote" and "target extended-remote". The problem is that the command buffer P passed to execute_command () gets overwritten in dont_repeat () while executing "target remote" command itself: #0 dont_repeat () at top.c:822 #1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483 #2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0) at remote.c:5946 #3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272 #4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490) at target.c:853 #5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1) at cli/cli-decode.c:2737 #6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688 Therefore the second call to lookup_cmd () at line 697 fails to find command because the original command string is gone. This commit addresses this particular problem by creating a *copy* of original command string for the sole purpose of using it after command execution to lookup the command again. It may not be the most efficient way but it's safer given that command buffer is shared and overwritten in hard-to-foresee situations. Tested on x86_64-linux. PR 30249 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249 --- gdb/top.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gdb/top.c b/gdb/top.c index 81f74f72f61..63798789553 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty) struct cmd_list_element *c; const char *line; const char *cmd_start = p; + std::string cmd_copy = p; auto cleanup_if_error = make_scope_exit (bpstat_clear_actions); scoped_value_mark cleanup = prepare_execute_command (); @@ -692,7 +693,7 @@ execute_command (const char *p, int from_tty) We need to lookup the command again since during its execution, a command may redefine itself. In this case, C pointer becomes invalid so we need to look it up again. */ - const char *cmd2 = cmd_start; + const char *cmd2 = cmd_copy.c_str (); c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1); if (c != nullptr) execute_cmd_post_hook (c); -- 2.39.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gdb: fix post-hook execution for remote targets 2023-04-14 15:17 [PATCH] gdb: fix post-hook execution for remote targets Jan Vrany @ 2023-05-03 14:40 ` Jan Vraný 2023-05-10 11:14 ` Jan Vraný 2023-05-10 15:07 ` Tom Tromey 1 sibling, 1 reply; 7+ messages in thread From: Jan Vraný @ 2023-05-03 14:40 UTC (permalink / raw) To: gdb-patches; +Cc: Wenyan.Xin Polite ping. Jan On Fri, 2023-04-14 at 17:17 +0200, Jan Vrany wrote: > Commit b5661ff2 ("gdb: fix possible use-after-free when > executing commands") attempted to fix possible use-after-free > in case command redefines itself. > > Commit 37e5833d ("gdb: fix command lookup in execute_command ()") > updated the previous fix to handle subcommands as well by using the > original command string to lookup the command again after its execution. > > This fixed the test in gdb.base/define.exp but it turned out that it > does not work (at least) for "target remote" and "target extended-remote". > > The problem is that the command buffer P passed to execute_command () > gets overwritten in dont_repeat () while executing "target remote" > command itself: > > #0 dont_repeat () at top.c:822 > #1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483 > #2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0) > at remote.c:5946 > #3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272 > #4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490) > at target.c:853 > #5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1) > at cli/cli-decode.c:2737 > #6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688 > > Therefore the second call to lookup_cmd () at line 697 fails to find > command because the original command string is gone. > > This commit addresses this particular problem by creating a *copy* of > original command string for the sole purpose of using it after command > execution to lookup the command again. It may not be the most efficient > way but it's safer given that command buffer is shared and overwritten > in hard-to-foresee situations. > > Tested on x86_64-linux. > > PR 30249 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249 > --- > gdb/top.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gdb/top.c b/gdb/top.c > index 81f74f72f61..63798789553 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty) > struct cmd_list_element *c; > const char *line; > const char *cmd_start = p; > + std::string cmd_copy = p; > > auto cleanup_if_error = make_scope_exit (bpstat_clear_actions); > scoped_value_mark cleanup = prepare_execute_command (); > @@ -692,7 +693,7 @@ execute_command (const char *p, int from_tty) > We need to lookup the command again since during its execution, > a command may redefine itself. In this case, C pointer > becomes invalid so we need to look it up again. */ > - const char *cmd2 = cmd_start; > + const char *cmd2 = cmd_copy.c_str (); > c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1); > if (c != nullptr) > execute_cmd_post_hook (c); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gdb: fix post-hook execution for remote targets 2023-05-03 14:40 ` Jan Vraný @ 2023-05-10 11:14 ` Jan Vraný 0 siblings, 0 replies; 7+ messages in thread From: Jan Vraný @ 2023-05-10 11:14 UTC (permalink / raw) To: gdb-patches; +Cc: Wenyan.Xin Polite ping 2. Jan On Wed, 2023-05-03 at 15:40 +0100, Jan Vrany wrote: > Polite ping. > > Jan > > On Fri, 2023-04-14 at 17:17 +0200, Jan Vrany wrote: > > Commit b5661ff2 ("gdb: fix possible use-after-free when > > executing commands") attempted to fix possible use-after-free > > in case command redefines itself. > > > > Commit 37e5833d ("gdb: fix command lookup in execute_command ()") > > updated the previous fix to handle subcommands as well by using the > > original command string to lookup the command again after its execution. > > > > This fixed the test in gdb.base/define.exp but it turned out that it > > does not work (at least) for "target remote" and "target extended-remote". > > > > The problem is that the command buffer P passed to execute_command () > > gets overwritten in dont_repeat () while executing "target remote" > > command itself: > > > > #0 dont_repeat () at top.c:822 > > #1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483 > > #2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0) > > at remote.c:5946 > > #3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272 > > #4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490) > > at target.c:853 > > #5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1) > > at cli/cli-decode.c:2737 > > #6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688 > > > > Therefore the second call to lookup_cmd () at line 697 fails to find > > command because the original command string is gone. > > > > This commit addresses this particular problem by creating a *copy* of > > original command string for the sole purpose of using it after command > > execution to lookup the command again. It may not be the most efficient > > way but it's safer given that command buffer is shared and overwritten > > in hard-to-foresee situations. > > > > Tested on x86_64-linux. > > > > PR 30249 > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249 > > --- > > gdb/top.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/gdb/top.c b/gdb/top.c > > index 81f74f72f61..63798789553 100644 > > --- a/gdb/top.c > > +++ b/gdb/top.c > > @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty) > > struct cmd_list_element *c; > > const char *line; > > const char *cmd_start = p; > > + std::string cmd_copy = p; > > > > auto cleanup_if_error = make_scope_exit (bpstat_clear_actions); > > scoped_value_mark cleanup = prepare_execute_command (); > > @@ -692,7 +693,7 @@ execute_command (const char *p, int from_tty) > > We need to lookup the command again since during its execution, > > a command may redefine itself. In this case, C pointer > > becomes invalid so we need to look it up again. */ > > - const char *cmd2 = cmd_start; > > + const char *cmd2 = cmd_copy.c_str (); > > c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1); > > if (c != nullptr) > > execute_cmd_post_hook (c); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] gdb: fix post-hook execution for remote targets 2023-04-14 15:17 [PATCH] gdb: fix post-hook execution for remote targets Jan Vrany 2023-05-03 14:40 ` Jan Vraný @ 2023-05-10 15:07 ` Tom Tromey 2023-05-17 18:14 ` [PATCH v2] " Jan Vrany 1 sibling, 1 reply; 7+ messages in thread From: Tom Tromey @ 2023-05-10 15:07 UTC (permalink / raw) To: Jan Vrany via Gdb-patches; +Cc: Jan Vrany, Wenyan.Xin >>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes: Sorry about the delay on this. Jan> The problem is that the command buffer P passed to execute_command () Jan> gets overwritten in dont_repeat () while executing "target remote" Jan> command itself: Jan> #0 dont_repeat () at top.c:822 I wonder if it would be possible to reimplement dont_repeat so that it does not overwrite the command. Like, could it just set a flag? Jan> @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty) Jan> struct cmd_list_element *c; Jan> const char *line; Jan> const char *cmd_start = p; Jan> + std::string cmd_copy = p; A bit after this, there's a null check for p: /* This can happen when command_line_input hits end of file. */ if (p == NULL) { cleanup_if_error.release (); return; } If that can really happen, then cmd_copy has to be created afterward the check. Is it possible to write a test case for this? Even one that only fails with ASAN? Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] gdb: fix post-hook execution for remote targets 2023-05-10 15:07 ` Tom Tromey @ 2023-05-17 18:14 ` Jan Vrany 2023-05-17 18:56 ` Tom Tromey 0 siblings, 1 reply; 7+ messages in thread From: Jan Vrany @ 2023-05-17 18:14 UTC (permalink / raw) To: gdb-patches; +Cc: tom, Wenyan.Xin, Jan Vrany > I wonder if it would be possible to reimplement dont_repeat so that it > does not overwrite the command. Like, could it just set a flag? Yes, that'd make things clear. I tried couple things but each time something broke. The issue seem to be code like (top.c, execute_command()):id if (repeat_arguments != NULL && cmd_start == saved_command_line) { gdb_assert (strlen (args_pointer) >= strlen (repeat_arguments)); strcpy (saved_command_line + (args_pointer - cmd_start), repeat_arguments); } Here we compare the value of saved_command_line so we cannot easily set the flag and then when reading input and flag is set, return empty string instead of saved_command_line. Truth to be told, I don't understand how it works and what exactly (for example) the code above does or why it is needed. > > Jan> @@ -575,6 +575,7 @@ execute_command (const char *p, int from_tty) > Jan> struct cmd_list_element *c; > Jan> const char *line; > Jan> const char *cmd_start = p; > Jan> + std::string cmd_copy = p; > > A bit after this, there's a null check for p: > > /* This can happen when command_line_input hits end of file. */ > if (p == NULL) > { > cleanup_if_error.release (); > return; > } > > If that can really happen, then cmd_copy has to be created afterward the > check. Done that, see below. > > Is it possible to write a test case for this? Even one that only fails > with ASAN? > I checked that with my version of GCC, std::string s = nullptr; throws an error and abort()s but I could not find a situation when p == NULL. The testsuite passes on my machine (even with ASAN). I also tried to open two command line interpreters (using new-io console ...) and then close the PTY but still, I did not hit the case of p being NULL. Jan -- >8 -- Commit b5661ff2 ("gdb: fix possible use-after-free when executing commands") attempted to fix possible use-after-free in case command redefines itself. Commit 37e5833d ("gdb: fix command lookup in execute_command ()") updated the previous fix to handle subcommands as well by using the original command string to lookup the command again after its execution. This fixed the test in gdb.base/define.exp but it turned out that it does not work (at least) for "target remote" and "target extended-remote". The problem is that the command buffer P passed to execute_command () gets overwritten in dont_repeat () while executing "target remote" command itself: #0 dont_repeat () at top.c:822 #1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483 #2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0) at remote.c:5946 #3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272 #4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490) at target.c:853 #5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1) at cli/cli-decode.c:2737 #6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688 Therefore the second call to lookup_cmd () at line 697 fails to find command because the original command string is gone. This commit addresses this particular problem by creating a *copy* of original command string for the sole purpose of using it after command execution to lookup the command again. It may not be the most efficient way but it's safer given that command buffer is shared and overwritten in hard-to-foresee situations. Tested on x86_64-linux. PR 30249 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249 --- gdb/top.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gdb/top.c b/gdb/top.c index 0b819091d11..92de30a1472 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -471,6 +471,8 @@ execute_command (const char *p, int from_tty) return; } + std::string cmd_copy = p; + target_log_command (p); while (*p == ' ' || *p == '\t') @@ -577,7 +579,7 @@ execute_command (const char *p, int from_tty) We need to lookup the command again since during its execution, a command may redefine itself. In this case, C pointer becomes invalid so we need to look it up again. */ - const char *cmd2 = cmd_start; + const char *cmd2 = cmd_copy.c_str (); c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1); if (c != nullptr) execute_cmd_post_hook (c); -- 2.39.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] gdb: fix post-hook execution for remote targets 2023-05-17 18:14 ` [PATCH v2] " Jan Vrany @ 2023-05-17 18:56 ` Tom Tromey 2023-05-19 12:40 ` [pushed] " Jan Vrany 0 siblings, 1 reply; 7+ messages in thread From: Tom Tromey @ 2023-05-17 18:56 UTC (permalink / raw) To: Jan Vrany via Gdb-patches; +Cc: Jan Vrany, tom, Wenyan.Xin >>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes: >> I wonder if it would be possible to reimplement dont_repeat so that it >> does not overwrite the command. Like, could it just set a flag? Jan> Yes, that'd make things clear. I tried couple things but each time Jan> something broke. Yeah, the older the code is, the more likely it is to have some bizarre hidden dependency. Jan> Here we compare the value of saved_command_line so we cannot easily Jan> set the flag and then when reading input and flag is set, return empty string Jan> instead of saved_command_line. Truth to be told, I don't understand how it works Jan> and what exactly (for example) the code above does or why it is needed. Me too. Jan> I checked that with my version of GCC, std::string s = nullptr; throws an Jan> error and abort()s but I could not find a situation when p == NULL. It's possible that there isn't a case, but also possible that it's just never tested. Difficult to reason about these things. Anyway, this is ok. Approved-By: Tom Tromey <tom@tromey.com> Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* [pushed] gdb: fix post-hook execution for remote targets 2023-05-17 18:56 ` Tom Tromey @ 2023-05-19 12:40 ` Jan Vrany 0 siblings, 0 replies; 7+ messages in thread From: Jan Vrany @ 2023-05-19 12:40 UTC (permalink / raw) To: gdb-patches; +Cc: Wenyan.Xin, Jan Vrany, Tom Tromey Commit b5661ff2 ("gdb: fix possible use-after-free when executing commands") attempted to fix possible use-after-free in case command redefines itself. Commit 37e5833d ("gdb: fix command lookup in execute_command ()") updated the previous fix to handle subcommands as well by using the original command string to lookup the command again after its execution. This fixed the test in gdb.base/define.exp but it turned out that it does not work (at least) for "target remote" and "target extended-remote". The problem is that the command buffer P passed to execute_command () gets overwritten in dont_repeat () while executing "target remote" command itself: #0 dont_repeat () at top.c:822 #1 0x000055555730982a in target_preopen (from_tty=1) at target.c:2483 #2 0x000055555711e911 in remote_target::open_1 (name=0x55555881c7fe ":1234", from_tty=1, extended_p=0) at remote.c:5946 #3 0x000055555711d577 in remote_target::open (name=0x55555881c7fe ":1234", from_tty=1) at remote.c:5272 #4 0x00005555573062f2 in open_target (args=0x55555881c7fe ":1234", from_tty=1, command=0x5555589d0490) at target.c:853 #5 0x0000555556ad22fa in cmd_func (cmd=0x5555589d0490, args=0x55555881c7fe ":1234", from_tty=1) at cli/cli-decode.c:2737 #6 0x00005555573487fd in execute_command (p=0x55555881c802 "4", from_tty=1) at top.c:688 Therefore the second call to lookup_cmd () at line 697 fails to find command because the original command string is gone. This commit addresses this particular problem by creating a *copy* of original command string for the sole purpose of using it after command execution to lookup the command again. It may not be the most efficient way but it's safer given that command buffer is shared and overwritten in hard-to-foresee situations. Tested on x86_64-linux. PR 30249 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30249 Approved-By: Tom Tromey <tom@tromey.com> --- gdb/top.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gdb/top.c b/gdb/top.c index 0b819091d11..92de30a1472 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -471,6 +471,8 @@ execute_command (const char *p, int from_tty) return; } + std::string cmd_copy = p; + target_log_command (p); while (*p == ' ' || *p == '\t') @@ -577,7 +579,7 @@ execute_command (const char *p, int from_tty) We need to lookup the command again since during its execution, a command may redefine itself. In this case, C pointer becomes invalid so we need to look it up again. */ - const char *cmd2 = cmd_start; + const char *cmd2 = cmd_copy.c_str (); c = lookup_cmd (&cmd2, cmdlist, "", nullptr, 1, 1); if (c != nullptr) execute_cmd_post_hook (c); -- 2.39.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-19 12:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-04-14 15:17 [PATCH] gdb: fix post-hook execution for remote targets Jan Vrany 2023-05-03 14:40 ` Jan Vraný 2023-05-10 11:14 ` Jan Vraný 2023-05-10 15:07 ` Tom Tromey 2023-05-17 18:14 ` [PATCH v2] " Jan Vrany 2023-05-17 18:56 ` Tom Tromey 2023-05-19 12:40 ` [pushed] " Jan Vrany
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).