diff mbox

[8/9] Avoid undefined behavior in expression dumping

Message ID 20180827145620.11055-9-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Aug. 27, 2018, 2:56 p.m. UTC 
  -fsanitize=undefined pointed out undefined behavior in
dump_raw_expression like:

    runtime error: load of value 2887952, which is not a valid value for type 'exp_opcode'

dump_raw_expression will try to print the opcode for each element of
the expression, even when it is not valid.  To allow this, but have it
avoid undefined behavior, this patch sets the underlying type of enum
exp_opcode, and arranges for op_name to handle invalid opcodes more
nicely.

ChangeLog
2018-08-27  Tom Tromey  <tom@tromey.com>

	* expression.h (enum exp_opcode): Use uint8_t as base type.
	* expprint.c (op_name): Handle invalid opcodes.
---
 gdb/ChangeLog    | 5 +++++
 gdb/expprint.c   | 6 ++++++
 gdb/expression.h | 2 +-
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Pedro Alves Aug. 28, 2018, 6:48 p.m. UTC | #1 
On 08/27/2018 03:56 PM, Tom Tromey wrote:
> -fsanitize=undefined pointed out undefined behavior in
> dump_raw_expression like:
> 
>     runtime error: load of value 2887952, which is not a valid value for type 'exp_opcode'
> 
> dump_raw_expression will try to print the opcode for each element of
> the expression, even when it is not valid.  To allow this, but have it
> avoid undefined behavior, this patch sets the underlying type of enum
> exp_opcode, and arranges for op_name to handle invalid opcodes more
> nicely.

Could you include before/after example output?

>  const char *
>  op_name (struct expression *exp, enum exp_opcode opcode)
>  {
> +  if (opcode >= OP_UNUSED_LAST)
> +    {
> +      char *cell = get_print_cell ();
> +      xsnprintf (cell, PRINT_CELL_SIZE, "unknown opcode: %d", int (opcode));

If the underlying type is unsigned, what is the rationale for
printing it as signed?

> +      return cell;
> +    }
>    return exp->language_defn->la_exp_desc->op_name (opcode);
>  }
>  
> diff --git a/gdb/expression.h b/gdb/expression.h
> index 9f26bb8d60b..db572efe2a3 100644
> --- a/gdb/expression.h
> +++ b/gdb/expression.h
> @@ -39,7 +39,7 @@
>     and skip that many.  Strings, like numbers, are indicated
>     by the preceding opcode.  */
>  
> -enum exp_opcode
> +enum exp_opcode : uint8_t
>    {
>  #define OP(name) name ,
>  
> 

Thanks,
Pedro Alves
Tom Tromey Aug. 29, 2018, 12:27 a.m. UTC | #2 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Could you include before/after example output?

It'll be in the next revision.

>> const char *
>> op_name (struct expression *exp, enum exp_opcode opcode)
>> {
>> +  if (opcode >= OP_UNUSED_LAST)
>> +    {
>> +      char *cell = get_print_cell ();
>> +      xsnprintf (cell, PRINT_CELL_SIZE, "unknown opcode: %d", int (opcode));

Pedro> If the underlying type is unsigned, what is the rationale for
Pedro> printing it as signed?

Thinko.

Tom
diff mbox

Patch

diff --git a/gdb/expprint.c b/gdb/expprint.c
index d6ed41253ed..7c91cc73749 100644
--- a/gdb/expprint.c
+++ b/gdb/expprint.c
@@ -687,6 +687,12 @@  static int dump_subexp_body (struct expression *exp, struct ui_file *, int);
 const char *
 op_name (struct expression *exp, enum exp_opcode opcode)
 {
+  if (opcode >= OP_UNUSED_LAST)
+    {
+      char *cell = get_print_cell ();
+      xsnprintf (cell, PRINT_CELL_SIZE, "unknown opcode: %d", int (opcode));
+      return cell;
+    }
   return exp->language_defn->la_exp_desc->op_name (opcode);
 }
 
diff --git a/gdb/expression.h b/gdb/expression.h
index 9f26bb8d60b..db572efe2a3 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -39,7 +39,7 @@ 
    and skip that many.  Strings, like numbers, are indicated
    by the preceding opcode.  */
 
-enum exp_opcode
+enum exp_opcode : uint8_t
   {
 #define OP(name) name ,