[PATCH] Fix crash with "maintenance print arc"

[PATCH] Fix crash with "maintenance print arc"

[Luis Machado]

While doing something else, I noticed GDB crashed with
"maintenance print arc".

This happens because the code expects to find a "show" string pattern
within "maintenance print arc", since "arc" here is a prefix, and skip it.
In this case though, it won't find it, and we will have a bad pointer
getting dereferenced.

There is another part of the code with a similar assumption.

This patch hardens both code paths to prevent future crashes.

Regression-tested on x86_64 and aarch64 Linux Ubuntu 20.04.
---
 gdb/cli/cli-setshow.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 213573e443e..3852a505cf5 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -712,7 +712,13 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
 	{
 	  ui_out_emit_tuple optionlist_emitter (uiout, "optionlist");
 	  std::string prefixname = list->prefixname ();
-	  const char *new_prefix = strstr (prefixname.c_str (), "show ") + 5;
+	  const char *new_prefix = strstr (prefixname.c_str (), "show ");
+
+	  /* If we've found a "show" string, remove it now.  */
+	  if (new_prefix != nullptr)
+	    new_prefix += 5;
+	  else
+	    new_prefix = prefixname.c_str ();
 
 	  if (uiout->is_mi_like_p ())
 	    uiout->field_string ("prefix", new_prefix);
@@ -726,8 +732,15 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
 	    {
 	      /* If we find a prefix, output it (with "show " skipped).  */
 	      std::string prefixname = list->prefix->prefixname ();
-	      prefixname = (!list->prefix->is_prefix () ? ""
-			    : strstr (prefixname.c_str (), "show ") + 5);
+	      const char *prefix = nullptr;
+
+	      if (list->prefix->is_prefix ())
+		prefix = strstr (prefixname.c_str (), "show ");
+
+	      /* If we've found a "show" string, remove it now.  */
+	      if (prefix != nullptr)
+		prefixname = prefix + 5;
+
 	      uiout->text (prefixname);
 	    }
 	  uiout->field_string ("name", list->name);
-- 
2.25.1

Re: [PATCH] Fix crash with "maintenance print arc"

[Andrew Burgess]

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> While doing something else, I noticed GDB crashed with
> "maintenance print arc".

I tried building current master, and "maintenance print arc" completes
just fine (for me).  Can you give more details on how to reproduce the
crash?

Thanks,
Andrew


>
> This happens because the code expects to find a "show" string pattern
> within "maintenance print arc", since "arc" here is a prefix, and skip it.
> In this case though, it won't find it, and we will have a bad pointer
> getting dereferenced.
>
> There is another part of the code with a similar assumption.
>
> This patch hardens both code paths to prevent future crashes.
>
> Regression-tested on x86_64 and aarch64 Linux Ubuntu 20.04.
> ---
>  gdb/cli/cli-setshow.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
> index 213573e443e..3852a505cf5 100644
> --- a/gdb/cli/cli-setshow.c
> +++ b/gdb/cli/cli-setshow.c
> @@ -712,7 +712,13 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
>  	{
>  	  ui_out_emit_tuple optionlist_emitter (uiout, "optionlist");
>  	  std::string prefixname = list->prefixname ();
> -	  const char *new_prefix = strstr (prefixname.c_str (), "show ") + 5;
> +	  const char *new_prefix = strstr (prefixname.c_str (), "show ");
> +
> +	  /* If we've found a "show" string, remove it now.  */
> +	  if (new_prefix != nullptr)
> +	    new_prefix += 5;
> +	  else
> +	    new_prefix = prefixname.c_str ();
>  
>  	  if (uiout->is_mi_like_p ())
>  	    uiout->field_string ("prefix", new_prefix);
> @@ -726,8 +732,15 @@ cmd_show_list (struct cmd_list_element *list, int from_tty)
>  	    {
>  	      /* If we find a prefix, output it (with "show " skipped).  */
>  	      std::string prefixname = list->prefix->prefixname ();
> -	      prefixname = (!list->prefix->is_prefix () ? ""
> -			    : strstr (prefixname.c_str (), "show ") + 5);
> +	      const char *prefix = nullptr;
> +
> +	      if (list->prefix->is_prefix ())
> +		prefix = strstr (prefixname.c_str (), "show ");
> +
> +	      /* If we've found a "show" string, remove it now.  */
> +	      if (prefix != nullptr)
> +		prefixname = prefix + 5;
> +
>  	      uiout->text (prefixname);
>  	    }
>  	  uiout->field_string ("name", list->name);
> -- 
> 2.25.1

Re: [PATCH] Fix crash with "maintenance print arc"

[Luis Machado]

On 4/27/22 11:43, Andrew Burgess wrote:
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> While doing something else, I noticed GDB crashed with
>> "maintenance print arc".
> 
> I tried building current master, and "maintenance print arc" completes
> just fine (for me).  Can you give more details on how to reproduce the
> crash?

There is not much more to it other than building a 64-bit aarch64 Linux 
Ubuntu 20.04 master with --enable-targets=all. I can reproduce it quite 
reliably.

--

(gdb) maint print arc


Fatal signal: Segmentation fault
----- Backtrace -----
0xad07a3 gdb_internal_backtrace_1
         ../../../repos/binutils-gdb/gdb/bt-utils.c:122
0xad07a3 _Z22gdb_internal_backtracev
         ../../../repos/binutils-gdb/gdb/bt-utils.c:168
0xb8d6cd handle_fatal_signal
         ../../../repos/binutils-gdb/gdb/event-top.c:946
0xb8d761 handle_sigsegv
         ../../../repos/binutils-gdb/gdb/event-top.c:1019
0xf72f47bf ???
---------------------
A fatal error internal to GDB has been detected, further
debugging is not possible.  GDB will now terminate.

This is a bug, please report it.  For instructions, see:
<https://www.gnu.org/software/gdb/bugs/>.

Segmentation fault (core dumped)

--

Re: [PATCH] Fix crash with "maintenance print arc"

[Tom Tromey]

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

>> While doing something else, I noticed GDB crashed with
>> "maintenance print arc".

Andrew> I tried building current master, and "maintenance print arc" completes
Andrew> just fine (for me).  Can you give more details on how to reproduce the
Andrew> crash?

It crashes for me too.  Instead of using completion, just invoke the
command directly.

Tom

Re: [PATCH] Fix crash with "maintenance print arc"

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>>> While doing something else, I noticed GDB crashed with
>>> "maintenance print arc".
>
> Andrew> I tried building current master, and "maintenance print arc" completes
> Andrew> just fine (for me).  Can you give more details on how to reproduce the
> Andrew> crash?
>
> It crashes for me too.  Instead of using completion, just invoke the
> command directly.

Thanks.  When I looked at you proposed patch (which LGTM) that pointed
me at why this wasn't working for me.  Turns out I had a typo in the
configure line for the build of GDB I was testing on
'--enable-targts=all' so this arc specific command was not being
compiled in, and instead 'maint print arc' was expanding to 'maint print
architecture'.

Thanks,
Andrew

Re: [PATCH] Fix crash with "maintenance print arc"

[Tom Tromey]

>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

Luis> While doing something else, I noticed GDB crashed with
Luis> "maintenance print arc".

I think the bug here is that this uses add_show_prefix_cmd and not
add_basic_prefix_cmd.  See the appended, which also fixes the crash.

Luis> This happens because the code expects to find a "show" string pattern
Luis> within "maintenance print arc", since "arc" here is a prefix, and skip it.
Luis> In this case though, it won't find it, and we will have a bad pointer
Luis> getting dereferenced.

This looks reasonable to me.

Tom

diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index 98bd1c4bc0a..3edfd466f3b 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -2474,11 +2474,11 @@ _initialize_arc_tdep ()
   /* Register ARC-specific commands with gdb.  */
 
   /* Add root prefix command for "maintenance print arc" commands.  */
-  add_show_prefix_cmd ("arc", class_maintenance,
-		       _("ARC-specific maintenance commands for printing GDB "
-			 "internal state."),
-		       &maintenance_print_arc_list,
-		       0, &maintenanceprintlist);
+  add_basic_prefix_cmd ("arc", class_maintenance,
+			_("ARC-specific maintenance commands for printing GDB "
+			  "internal state."),
+			&maintenance_print_arc_list,
+			0, &maintenanceprintlist);
 
   add_cmd ("arc-instruction", class_maintenance,
 	   dump_arc_instruction_command,

Re: [PATCH] Fix crash with "maintenance print arc"

[Luis Machado]

On 4/28/22 16:26, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Luis> While doing something else, I noticed GDB crashed with
> Luis> "maintenance print arc".
> 
> I think the bug here is that this uses add_show_prefix_cmd and not
> add_basic_prefix_cmd.  See the appended, which also fixes the crash.

Ah, you're right. It did look odd why we were feeding maint commands to 
code that is supposed to handle show commands.

> 
> Luis> This happens because the code expects to find a "show" string pattern
> Luis> within "maintenance print arc", since "arc" here is a prefix, and skip it.
> Luis> In this case though, it won't find it, and we will have a bad pointer
> Luis> getting dereferenced.
> 
> This looks reasonable to me.

Should we prevent further situations like this by asserting that we 
don't have a class_maintenance prefix being registered as a show prefix?

> 
> Tom
> 
> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
> index 98bd1c4bc0a..3edfd466f3b 100644
> --- a/gdb/arc-tdep.c
> +++ b/gdb/arc-tdep.c
> @@ -2474,11 +2474,11 @@ _initialize_arc_tdep ()
>     /* Register ARC-specific commands with gdb.  */
>   
>     /* Add root prefix command for "maintenance print arc" commands.  */
> -  add_show_prefix_cmd ("arc", class_maintenance,
> -		       _("ARC-specific maintenance commands for printing GDB "
> -			 "internal state."),
> -		       &maintenance_print_arc_list,
> -		       0, &maintenanceprintlist);
> +  add_basic_prefix_cmd ("arc", class_maintenance,
> +			_("ARC-specific maintenance commands for printing GDB "
> +			  "internal state."),
> +			&maintenance_print_arc_list,
> +			0, &maintenanceprintlist);
>   
>     add_cmd ("arc-instruction", class_maintenance,
>   	   dump_arc_instruction_command,

Re: [PATCH] Fix crash with "maintenance print arc"

[Andrew Burgess]

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 4/28/22 16:26, Tom Tromey wrote:
>>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> Luis> While doing something else, I noticed GDB crashed with
>> Luis> "maintenance print arc".
>> 
>> I think the bug here is that this uses add_show_prefix_cmd and not
>> add_basic_prefix_cmd.  See the appended, which also fixes the crash.
>
> Ah, you're right. It did look odd why we were feeding maint commands to 
> code that is supposed to handle show commands.
>
>> 
>> Luis> This happens because the code expects to find a "show" string pattern
>> Luis> within "maintenance print arc", since "arc" here is a prefix, and skip it.
>> Luis> In this case though, it won't find it, and we will have a bad pointer
>> Luis> getting dereferenced.
>> 
>> This looks reasonable to me.
>
> Should we prevent further situations like this by asserting that we 
> don't have a class_maintenance prefix being registered as a show
> prefix?

Would this not trigger for all the existing 'maint show ...' commands?

Thanks,
Andrew

>
>> 
>> Tom
>> 
>> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
>> index 98bd1c4bc0a..3edfd466f3b 100644
>> --- a/gdb/arc-tdep.c
>> +++ b/gdb/arc-tdep.c
>> @@ -2474,11 +2474,11 @@ _initialize_arc_tdep ()
>>     /* Register ARC-specific commands with gdb.  */
>>   
>>     /* Add root prefix command for "maintenance print arc" commands.  */
>> -  add_show_prefix_cmd ("arc", class_maintenance,
>> -		       _("ARC-specific maintenance commands for printing GDB "
>> -			 "internal state."),
>> -		       &maintenance_print_arc_list,
>> -		       0, &maintenanceprintlist);
>> +  add_basic_prefix_cmd ("arc", class_maintenance,
>> +			_("ARC-specific maintenance commands for printing GDB "
>> +			  "internal state."),
>> +			&maintenance_print_arc_list,
>> +			0, &maintenanceprintlist);
>>   
>>     add_cmd ("arc-instruction", class_maintenance,
>>   	   dump_arc_instruction_command,

Re: [PATCH] Fix crash with "maintenance print arc"

[Luis Machado]

On 4/29/22 11:18, Andrew Burgess wrote:
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> On 4/28/22 16:26, Tom Tromey wrote:
>>>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>
>>> Luis> While doing something else, I noticed GDB crashed with
>>> Luis> "maintenance print arc".
>>>
>>> I think the bug here is that this uses add_show_prefix_cmd and not
>>> add_basic_prefix_cmd.  See the appended, which also fixes the crash.
>>
>> Ah, you're right. It did look odd why we were feeding maint commands to
>> code that is supposed to handle show commands.
>>
>>>
>>> Luis> This happens because the code expects to find a "show" string pattern
>>> Luis> within "maintenance print arc", since "arc" here is a prefix, and skip it.
>>> Luis> In this case though, it won't find it, and we will have a bad pointer
>>> Luis> getting dereferenced.
>>>
>>> This looks reasonable to me.
>>
>> Should we prevent further situations like this by asserting that we
>> don't have a class_maintenance prefix being registered as a show
>> prefix?
> 
> Would this not trigger for all the existing 'maint show ...' commands?
> 

Ah, we have a "maint show" command. I wasn't aware of it until now.

Isn't this particular behavior a bit strange though? "maintenance" will 
always output the maintenance prefix, except for "maintenance show", 
which removes the show part.

It might be historical.

Re: [PATCH] Fix crash with "maintenance print arc"

[Pedro Alves]

On 2022-04-29 11:39, Luis Machado via Gdb-patches wrote:

> Ah, we have a "maint show" command. I wasn't aware of it until now.
> 
> Isn't this particular behavior a bit strange though? "maintenance" will always output the maintenance prefix, except for "maintenance show", which removes the show part.
> 
> It might be historical.

I think "maint show" is doing the same thing as "show".  Note "help maint show" says it lists the
_variables_, not the commands to show each variable:

 (gdb) help maint show
 Show GDB internal variables used by the GDB maintainer.
 Configure variables internal to GDB that aid in GDB's maintenance
 ...

"maint", "help show" and "help maint show" output a list of commands, thus with full prefix.

Re: [PATCH] Fix crash with "maintenance print arc"

[Luis Machado]

Hi Tom,

On 4/28/22 16:26, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Luis> While doing something else, I noticed GDB crashed with
> Luis> "maintenance print arc".
> 
> I think the bug here is that this uses add_show_prefix_cmd and not
> add_basic_prefix_cmd.  See the appended, which also fixes the crash.
> 
> Luis> This happens because the code expects to find a "show" string pattern
> Luis> within "maintenance print arc", since "arc" here is a prefix, and skip it.
> Luis> In this case though, it won't find it, and we will have a bad pointer
> Luis> getting dereferenced.
> 
> This looks reasonable to me.
> 
> Tom
> 
> diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
> index 98bd1c4bc0a..3edfd466f3b 100644
> --- a/gdb/arc-tdep.c
> +++ b/gdb/arc-tdep.c
> @@ -2474,11 +2474,11 @@ _initialize_arc_tdep ()
>     /* Register ARC-specific commands with gdb.  */
>   
>     /* Add root prefix command for "maintenance print arc" commands.  */
> -  add_show_prefix_cmd ("arc", class_maintenance,
> -		       _("ARC-specific maintenance commands for printing GDB "
> -			 "internal state."),
> -		       &maintenance_print_arc_list,
> -		       0, &maintenanceprintlist);
> +  add_basic_prefix_cmd ("arc", class_maintenance,
> +			_("ARC-specific maintenance commands for printing GDB "
> +			  "internal state."),
> +			&maintenance_print_arc_list,
> +			0, &maintenanceprintlist);
>   
>     add_cmd ("arc-instruction", class_maintenance,
>   	   dump_arc_instruction_command,

FTR, I'm happy to go with your patch for this one.

Re: [PATCH] Fix crash with "maintenance print arc"

[Tom Tromey]

>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

>> -  add_show_prefix_cmd ("arc", class_maintenance,
>> -		       _("ARC-specific maintenance commands for printing GDB "
>> -			 "internal state."),
>> -		       &maintenance_print_arc_list,
>> -		       0, &maintenanceprintlist);
>> +  add_basic_prefix_cmd ("arc", class_maintenance,
>> +			_("ARC-specific maintenance commands for printing GDB "
>> +			  "internal state."),
>> +			&maintenance_print_arc_list,
>> +			0, &maintenanceprintlist);
>> add_cmd ("arc-instruction", class_maintenance,
>> dump_arc_instruction_command,

Luis> FTR, I'm happy to go with your patch for this one.

Oops, I dropped this.  I will check it in shortly.

Tom