[2/3] Fix another crash with gdb parameters in Python

Message ID 20220105171347.2307106-3-tromey@adacore.com
State Superseded
Headers show
Series
  • Improvements to Python parameters
Related show

Commit Message

Simon Marchi via Gdb-patches Jan. 5, 2022, 5:13 p.m.
While looking into the language-capturing issue, I found another way
to crash gdb using parameters from Python:

(gdb) python print(gdb.parameter('endian'))

(This is related to PR python/12188, though this patch isn't going to
fix what that bug is really about.)

The problem here is that the global variable that underlies the
"endian" parameter is initialized to NULL.  However, that's not a
valid value for an "enum" set/show parameter.

My understanding is that, in gdb, an "enum" parameter's underlying
variable must have a value that is "==" (not just strcmp-equal) to one
of the values coming from the enum array.  This invariant is relied on
in various places.

I started this patch by fixing the problem with "endian".  Then I
added some assertions to add_setshow_enum_cmd to try to catch other
problems of the same type.

This patch fixes all the problems that I found.  I also looked at all
the calls to add_setshow_enum_cmd to ensure that they were all
included in the gdb I tested.  I think they are: there are no calls in
nat-* files, or in remote-sim.c; and I was trying a build with all
targets, Python, and Guile enabled.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12188
---
 gdb/arch-utils.c                          |  5 +++--
 gdb/arm-tdep.c                            |  2 ++
 gdb/charset.c                             |  9 +++++++++
 gdb/cli/cli-decode.c                      | 10 ++++++++++
 gdb/guile/scm-param.c                     |  4 ++--
 gdb/language.c                            | 11 ++++++-----
 gdb/osabi.c                               |  4 +++-
 gdb/python/py-param.c                     |  4 ++--
 gdb/testsuite/gdb.python/py-parameter.exp |  4 ++++
 9 files changed, 41 insertions(+), 12 deletions(-)

-- 
2.31.1

Comments

Simon Marchi via Gdb-patches Jan. 10, 2022, 3:46 p.m. | #1
* Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> [2022-01-05 10:13:46 -0700]:

> While looking into the language-capturing issue, I found another way

> to crash gdb using parameters from Python:

> 

> (gdb) python print(gdb.parameter('endian'))

> 

> (This is related to PR python/12188, though this patch isn't going to

> fix what that bug is really about.)

> 

> The problem here is that the global variable that underlies the

> "endian" parameter is initialized to NULL.  However, that's not a

> valid value for an "enum" set/show parameter.

> 

> My understanding is that, in gdb, an "enum" parameter's underlying

> variable must have a value that is "==" (not just strcmp-equal) to one

> of the values coming from the enum array.  This invariant is relied on

> in various places.

> 

> I started this patch by fixing the problem with "endian".  Then I

> added some assertions to add_setshow_enum_cmd to try to catch other

> problems of the same type.

> 

> This patch fixes all the problems that I found.  I also looked at all

> the calls to add_setshow_enum_cmd to ensure that they were all

> included in the gdb I tested.  I think they are: there are no calls in

> nat-* files, or in remote-sim.c; and I was trying a build with all

> targets, Python, and Guile enabled.

> 

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12188


LGTM.

Thanks,
Andrew

> ---

>  gdb/arch-utils.c                          |  5 +++--

>  gdb/arm-tdep.c                            |  2 ++

>  gdb/charset.c                             |  9 +++++++++

>  gdb/cli/cli-decode.c                      | 10 ++++++++++

>  gdb/guile/scm-param.c                     |  4 ++--

>  gdb/language.c                            | 11 ++++++-----

>  gdb/osabi.c                               |  4 +++-

>  gdb/python/py-param.c                     |  4 ++--

>  gdb/testsuite/gdb.python/py-parameter.exp |  4 ++++

>  9 files changed, 41 insertions(+), 12 deletions(-)

> 

> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c

> index 38e3132668d..3ee978a0249 100644

> --- a/gdb/arch-utils.c

> +++ b/gdb/arch-utils.c

> @@ -359,7 +359,7 @@ static const char *const endian_enum[] =

>    endian_auto,

>    NULL,

>  };

> -static const char *set_endian_string;

> +static const char *set_endian_string = endian_auto;

>  

>  enum bfd_endian

>  selected_byte_order (void)

> @@ -757,7 +757,8 @@ initialize_current_architecture (void)

>       list of architectures.  */

>    {

>      /* Append ``auto''.  */

> -    arches.push_back ("auto");

> +    set_architecture_string = "auto";

> +    arches.push_back (set_architecture_string);

>      arches.push_back (nullptr);

>      set_show_commands architecture_cmds

>        = add_setshow_enum_cmd ("architecture", class_support,

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c

> index d631f53f703..72d2c890bc2 100644

> --- a/gdb/arm-tdep.c

> +++ b/gdb/arm-tdep.c

> @@ -9802,6 +9802,8 @@ _initialize_arm_tdep ()

>  	size_t offset = strlen ("reg-names-");

>  	const char *style = disasm_options->name[i];

>  	valid_disassembly_styles[j++] = &style[offset];

> +	if (strcmp (&style[offset], "std") == 0)

> +	  disassembly_style = &style[offset];

>  	length = snprintf (rdptr, rest, "%s - %s\n", &style[offset],

>  			   disasm_options->description[i]);

>  	rdptr += length;

> diff --git a/gdb/charset.c b/gdb/charset.c

> index 0b3ef26f1d6..e011793f746 100644

> --- a/gdb/charset.c

> +++ b/gdb/charset.c

> @@ -1029,6 +1029,9 @@ _initialize_charset ()

>  #endif

>  #endif

>  

> +  /* Recall that the first element is always "auto".  */

> +  host_charset_name = charset_enum[0];

> +  gdb_assert (strcmp (host_charset_name, "auto") == 0);

>    add_setshow_enum_cmd ("charset", class_support,

>  			charset_enum, &host_charset_name, _("\

>  Set the host and target character sets."), _("\

> @@ -1057,6 +1060,9 @@ To see a list of the character sets GDB supports, type `set host-charset <TAB>'.

>  			show_host_charset_name,

>  			&setlist, &showlist);

>  

> +  /* Recall that the first element is always "auto".  */

> +  target_charset_name = charset_enum[0];

> +  gdb_assert (strcmp (target_charset_name, "auto") == 0);

>    add_setshow_enum_cmd ("target-charset", class_support,

>  			charset_enum, &target_charset_name, _("\

>  Set the target character set."), _("\

> @@ -1069,6 +1075,9 @@ To see a list of the character sets GDB supports, type `set target-charset'<TAB>

>  			show_target_charset_name,

>  			&setlist, &showlist);

>  

> +  /* Recall that the first element is always "auto".  */

> +  target_wide_charset_name = charset_enum[0];

> +  gdb_assert (strcmp (target_wide_charset_name, "auto") == 0);

>    add_setshow_enum_cmd ("target-wide-charset", class_support,

>  			charset_enum, &target_wide_charset_name,

>  			_("\

> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c

> index 7266fb36d0d..bf6404a5755 100644

> --- a/gdb/cli/cli-decode.c

> +++ b/gdb/cli/cli-decode.c

> @@ -613,6 +613,16 @@ add_setshow_enum_cmd (const char *name,

>  		      struct cmd_list_element **set_list,

>  		      struct cmd_list_element **show_list)

>  {

> +  /* We require *VAR to be initialized before this call, and

> +     furthermore it must be == to one of the values in ENUMLIST.  */

> +  gdb_assert (var != nullptr && *var != nullptr);

> +  for (int i = 0; ; ++i)

> +    {

> +      gdb_assert (enumlist[i] != nullptr);

> +      if (*var == enumlist[i])

> +	break;

> +    }

> +

>    set_show_commands commands

>      =  add_setshow_cmd_full<const char *> (name, theclass, var_enum, var,

>  					   set_doc, show_doc, help_doc,

> diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c

> index 125b0e4766b..a3af3c2251e 100644

> --- a/gdb/guile/scm-param.c

> +++ b/gdb/guile/scm-param.c

> @@ -458,12 +458,12 @@ add_setshow_generic (enum var_types param_type, enum command_class cmd_class,

>        break;

>  

>      case var_enum:

> +      /* Initialize the value, just in case.  */

> +      make_setting (self).set<const char *> (self->enumeration[0]);

>        commands = add_setshow_enum_cmd (cmd_name, cmd_class, self->enumeration,

>  				       &self->value.cstringval, set_doc,

>  				       show_doc, help_doc, set_func, show_func,

>  				       set_list, show_list);

> -      /* Initialize the value, just in case.  */

> -      make_setting (self).set<const char *> (self->enumeration[0]);

>        break;

>  

>      default:

> diff --git a/gdb/language.c b/gdb/language.c

> index 132ff769817..64a0a9e0fbf 100644

> --- a/gdb/language.c

> +++ b/gdb/language.c

> @@ -476,7 +476,8 @@ add_set_language_command ()

>    /* Display "auto", "local" and "unknown" first, and then the rest,

>       alpha sorted.  */

>    const char **language_names_p = language_names;

> -  *language_names_p++ = language_def (language_auto)->name ();

> +  language = language_def (language_auto)->name ();

> +  *language_names_p++ = language;

>    *language_names_p++ = "local";

>    *language_names_p++ = language_def (language_unknown)->name ();

>    const char **sort_begin = language_names_p;

> @@ -1150,6 +1151,8 @@ _initialize_language ()

>    add_alias_cmd ("c", setshow_check_cmds.show, no_class, 1, &showlist);

>    add_alias_cmd ("ch", setshow_check_cmds.show, no_class, 1, &showlist);

>  

> +  range = type_or_range_names[3];

> +  gdb_assert (strcmp (range, "auto") == 0);

>    add_setshow_enum_cmd ("range", class_support, type_or_range_names,

>  			&range,

>  			_("Set range checking (on/warn/off/auto)."),

> @@ -1158,6 +1161,8 @@ _initialize_language ()

>  			show_range_command,

>  			&setchecklist, &showchecklist);

>  

> +  case_sensitive = case_sensitive_names[2];

> +  gdb_assert (strcmp (case_sensitive, "auto") == 0);

>    add_setshow_enum_cmd ("case-sensitive", class_support, case_sensitive_names,

>  			&case_sensitive, _("\

>  Set case sensitivity in name search (on/off/auto)."), _("\

> @@ -1174,10 +1179,6 @@ For Fortran the default is off; for other languages the default is on."),

>  

>    add_set_language_command ();

>  

> -  language = "auto";

> -  range = "auto";

> -  case_sensitive = "auto";

> -

>    /* Have the above take effect.  */

>    set_language (language_auto);

>  }

> diff --git a/gdb/osabi.c b/gdb/osabi.c

> index 40c86e782e7..d4a98061dbd 100644

> --- a/gdb/osabi.c

> +++ b/gdb/osabi.c

> @@ -666,11 +666,13 @@ _initialize_gdb_osabi ()

>  				  generic_elf_osabi_sniffer);

>  

>    /* Register the "set osabi" command.  */

> +  user_osabi_state = osabi_auto;

> +  set_osabi_string = gdb_osabi_available_names[0];

> +  gdb_assert (strcmp (set_osabi_string, "auto") == 0);

>    add_setshow_enum_cmd ("osabi", class_support, gdb_osabi_available_names,

>  			&set_osabi_string,

>  			_("Set OS ABI of target."),

>  			_("Show OS ABI of target."),

>  			NULL, set_osabi, show_osabi,

>  			&setlist, &showlist);

> -  user_osabi_state = osabi_auto;

>  }

> diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c

> index 8559bc997ea..6ee14d42ab8 100644

> --- a/gdb/python/py-param.c

> +++ b/gdb/python/py-param.c

> @@ -572,13 +572,13 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,

>        break;

>  

>      case var_enum:

> +      /* Initialize the value, just in case.  */

> +      self->value.cstringval = self->enumeration[0];

>        commands = add_setshow_enum_cmd (cmd_name.get (), cmdclass,

>  				       self->enumeration,

>  				       &self->value.cstringval, set_doc,

>  				       show_doc, help_doc, get_set_value,

>  				       get_show_value, set_list, show_list);

> -      /* Initialize the value, just in case.  */

> -      self->value.cstringval = self->enumeration[0];

>        break;

>  

>      default:

> diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp

> index b025c2562f0..2de148c1b27 100644

> --- a/gdb/testsuite/gdb.python/py-parameter.exp

> +++ b/gdb/testsuite/gdb.python/py-parameter.exp

> @@ -364,3 +364,7 @@ test_really_undocumented_parameter

>  test_deprecated_api_parameter

>  test_integer_parameter

>  test_throwing_parameter

> +

> +# This caused a gdb crash.

> +gdb_test "python print(gdb.parameter('endian'))" "auto" \

> +    "print endian parameter"

> -- 

> 2.31.1

>

Patch

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 38e3132668d..3ee978a0249 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -359,7 +359,7 @@  static const char *const endian_enum[] =
   endian_auto,
   NULL,
 };
-static const char *set_endian_string;
+static const char *set_endian_string = endian_auto;
 
 enum bfd_endian
 selected_byte_order (void)
@@ -757,7 +757,8 @@  initialize_current_architecture (void)
      list of architectures.  */
   {
     /* Append ``auto''.  */
-    arches.push_back ("auto");
+    set_architecture_string = "auto";
+    arches.push_back (set_architecture_string);
     arches.push_back (nullptr);
     set_show_commands architecture_cmds
       = add_setshow_enum_cmd ("architecture", class_support,
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d631f53f703..72d2c890bc2 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9802,6 +9802,8 @@  _initialize_arm_tdep ()
 	size_t offset = strlen ("reg-names-");
 	const char *style = disasm_options->name[i];
 	valid_disassembly_styles[j++] = &style[offset];
+	if (strcmp (&style[offset], "std") == 0)
+	  disassembly_style = &style[offset];
 	length = snprintf (rdptr, rest, "%s - %s\n", &style[offset],
 			   disasm_options->description[i]);
 	rdptr += length;
diff --git a/gdb/charset.c b/gdb/charset.c
index 0b3ef26f1d6..e011793f746 100644
--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -1029,6 +1029,9 @@  _initialize_charset ()
 #endif
 #endif
 
+  /* Recall that the first element is always "auto".  */
+  host_charset_name = charset_enum[0];
+  gdb_assert (strcmp (host_charset_name, "auto") == 0);
   add_setshow_enum_cmd ("charset", class_support,
 			charset_enum, &host_charset_name, _("\
 Set the host and target character sets."), _("\
@@ -1057,6 +1060,9 @@  To see a list of the character sets GDB supports, type `set host-charset <TAB>'.
 			show_host_charset_name,
 			&setlist, &showlist);
 
+  /* Recall that the first element is always "auto".  */
+  target_charset_name = charset_enum[0];
+  gdb_assert (strcmp (target_charset_name, "auto") == 0);
   add_setshow_enum_cmd ("target-charset", class_support,
 			charset_enum, &target_charset_name, _("\
 Set the target character set."), _("\
@@ -1069,6 +1075,9 @@  To see a list of the character sets GDB supports, type `set target-charset'<TAB>
 			show_target_charset_name,
 			&setlist, &showlist);
 
+  /* Recall that the first element is always "auto".  */
+  target_wide_charset_name = charset_enum[0];
+  gdb_assert (strcmp (target_wide_charset_name, "auto") == 0);
   add_setshow_enum_cmd ("target-wide-charset", class_support,
 			charset_enum, &target_wide_charset_name,
 			_("\
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 7266fb36d0d..bf6404a5755 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -613,6 +613,16 @@  add_setshow_enum_cmd (const char *name,
 		      struct cmd_list_element **set_list,
 		      struct cmd_list_element **show_list)
 {
+  /* We require *VAR to be initialized before this call, and
+     furthermore it must be == to one of the values in ENUMLIST.  */
+  gdb_assert (var != nullptr && *var != nullptr);
+  for (int i = 0; ; ++i)
+    {
+      gdb_assert (enumlist[i] != nullptr);
+      if (*var == enumlist[i])
+	break;
+    }
+
   set_show_commands commands
     =  add_setshow_cmd_full<const char *> (name, theclass, var_enum, var,
 					   set_doc, show_doc, help_doc,
diff --git a/gdb/guile/scm-param.c b/gdb/guile/scm-param.c
index 125b0e4766b..a3af3c2251e 100644
--- a/gdb/guile/scm-param.c
+++ b/gdb/guile/scm-param.c
@@ -458,12 +458,12 @@  add_setshow_generic (enum var_types param_type, enum command_class cmd_class,
       break;
 
     case var_enum:
+      /* Initialize the value, just in case.  */
+      make_setting (self).set<const char *> (self->enumeration[0]);
       commands = add_setshow_enum_cmd (cmd_name, cmd_class, self->enumeration,
 				       &self->value.cstringval, set_doc,
 				       show_doc, help_doc, set_func, show_func,
 				       set_list, show_list);
-      /* Initialize the value, just in case.  */
-      make_setting (self).set<const char *> (self->enumeration[0]);
       break;
 
     default:
diff --git a/gdb/language.c b/gdb/language.c
index 132ff769817..64a0a9e0fbf 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -476,7 +476,8 @@  add_set_language_command ()
   /* Display "auto", "local" and "unknown" first, and then the rest,
      alpha sorted.  */
   const char **language_names_p = language_names;
-  *language_names_p++ = language_def (language_auto)->name ();
+  language = language_def (language_auto)->name ();
+  *language_names_p++ = language;
   *language_names_p++ = "local";
   *language_names_p++ = language_def (language_unknown)->name ();
   const char **sort_begin = language_names_p;
@@ -1150,6 +1151,8 @@  _initialize_language ()
   add_alias_cmd ("c", setshow_check_cmds.show, no_class, 1, &showlist);
   add_alias_cmd ("ch", setshow_check_cmds.show, no_class, 1, &showlist);
 
+  range = type_or_range_names[3];
+  gdb_assert (strcmp (range, "auto") == 0);
   add_setshow_enum_cmd ("range", class_support, type_or_range_names,
 			&range,
 			_("Set range checking (on/warn/off/auto)."),
@@ -1158,6 +1161,8 @@  _initialize_language ()
 			show_range_command,
 			&setchecklist, &showchecklist);
 
+  case_sensitive = case_sensitive_names[2];
+  gdb_assert (strcmp (case_sensitive, "auto") == 0);
   add_setshow_enum_cmd ("case-sensitive", class_support, case_sensitive_names,
 			&case_sensitive, _("\
 Set case sensitivity in name search (on/off/auto)."), _("\
@@ -1174,10 +1179,6 @@  For Fortran the default is off; for other languages the default is on."),
 
   add_set_language_command ();
 
-  language = "auto";
-  range = "auto";
-  case_sensitive = "auto";
-
   /* Have the above take effect.  */
   set_language (language_auto);
 }
diff --git a/gdb/osabi.c b/gdb/osabi.c
index 40c86e782e7..d4a98061dbd 100644
--- a/gdb/osabi.c
+++ b/gdb/osabi.c
@@ -666,11 +666,13 @@  _initialize_gdb_osabi ()
 				  generic_elf_osabi_sniffer);
 
   /* Register the "set osabi" command.  */
+  user_osabi_state = osabi_auto;
+  set_osabi_string = gdb_osabi_available_names[0];
+  gdb_assert (strcmp (set_osabi_string, "auto") == 0);
   add_setshow_enum_cmd ("osabi", class_support, gdb_osabi_available_names,
 			&set_osabi_string,
 			_("Set OS ABI of target."),
 			_("Show OS ABI of target."),
 			NULL, set_osabi, show_osabi,
 			&setlist, &showlist);
-  user_osabi_state = osabi_auto;
 }
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index 8559bc997ea..6ee14d42ab8 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -572,13 +572,13 @@  add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_enum:
+      /* Initialize the value, just in case.  */
+      self->value.cstringval = self->enumeration[0];
       commands = add_setshow_enum_cmd (cmd_name.get (), cmdclass,
 				       self->enumeration,
 				       &self->value.cstringval, set_doc,
 				       show_doc, help_doc, get_set_value,
 				       get_show_value, set_list, show_list);
-      /* Initialize the value, just in case.  */
-      self->value.cstringval = self->enumeration[0];
       break;
 
     default:
diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
index b025c2562f0..2de148c1b27 100644
--- a/gdb/testsuite/gdb.python/py-parameter.exp
+++ b/gdb/testsuite/gdb.python/py-parameter.exp
@@ -364,3 +364,7 @@  test_really_undocumented_parameter
 test_deprecated_api_parameter
 test_integer_parameter
 test_throwing_parameter
+
+# This caused a gdb crash.
+gdb_test "python print(gdb.parameter('endian'))" "auto" \
+    "print endian parameter"