[PATCH] Fix struct expression regression

[PATCH] Fix struct expression regression

[Tom Tromey]

The patch to change struct expression to use new introduced a
regression -- there is a spot that reallocates expressions that I
failed to update.

This patch rewrites this code to follow the new approach.  Now the
rewriting is done in place.

gdb/ChangeLog
2020-12-02  Tom Tromey  <tom@tromey.com>

	PR ada/26999
	* ada-lang.c (replace_operator_with_call): Rewrite.
---
 gdb/ChangeLog  |  5 +++++
 gdb/ada-lang.c | 31 +++++++++++++------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 906671155d0..7d062294aa5 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4000,28 +4000,23 @@ replace_operator_with_call (expression_up *expp, int pc, int nargs,
 			    int oplen, struct symbol *sym,
 			    const struct block *block)
 {
-  /* A new expression, with 6 more elements (3 for funcall, 4 for function
-     symbol, -oplen for operator being replaced).  */
-  struct expression *newexp = (struct expression *)
-    xzalloc (sizeof (struct expression)
-	     + EXP_ELEM_TO_BYTES ((*expp)->nelts + 7 - oplen));
+  /* We want to add 6 more elements (3 for funcall, 4 for function
+     symbol, -OPLEN for operator being replaced) to the
+     expression.  */
   struct expression *exp = expp->get ();
+  int save_nelts = exp->nelts;
+  exp->nelts = exp->nelts + 7 - oplen;
+  exp->resize (exp->nelts);
 
-  newexp->nelts = exp->nelts + 7 - oplen;
-  newexp->language_defn = exp->language_defn;
-  newexp->gdbarch = exp->gdbarch;
-  memcpy (newexp->elts, exp->elts, EXP_ELEM_TO_BYTES (pc));
-  memcpy (newexp->elts + pc + 7, exp->elts + pc + oplen,
-	  EXP_ELEM_TO_BYTES (exp->nelts - pc - oplen));
+  memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
+	   EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
 
-  newexp->elts[pc].opcode = newexp->elts[pc + 2].opcode = OP_FUNCALL;
-  newexp->elts[pc + 1].longconst = (LONGEST) nargs;
+  exp->elts[pc].opcode = exp->elts[pc + 2].opcode = OP_FUNCALL;
+  exp->elts[pc + 1].longconst = (LONGEST) nargs;
 
-  newexp->elts[pc + 3].opcode = newexp->elts[pc + 6].opcode = OP_VAR_VALUE;
-  newexp->elts[pc + 4].block = block;
-  newexp->elts[pc + 5].symbol = sym;
-
-  expp->reset (newexp);
+  exp->elts[pc + 3].opcode = exp->elts[pc + 6].opcode = OP_VAR_VALUE;
+  exp->elts[pc + 4].block = block;
+  exp->elts[pc + 5].symbol = sym;
 }
 
 /* Type-class predicates */
-- 
2.17.2

Re: [PATCH] Fix struct expression regression

[Tom de Vries]

On 12/3/20 1:41 AM, Tom Tromey wrote:
> The patch to change struct expression to use new introduced a
> regression -- there is a spot that reallocates expressions that I
> failed to update.
> 
> This patch rewrites this code to follow the new approach.  Now the
> rewriting is done in place.
> 

Hi,

I've tested the patch with the test-case where the regression triggered
for me: gdb.ada/bp_c_mixed_case.exp, both with -m64 and -m32.

The patch LGTM.

I found the memmove arguments a bit hard to parse though, and did a
rewrite using variables spelling out things a bit more:
...
  int orig_op_start = pc;
  int orig_op_len = oplen;
  int orig_op_end = orig_op_start + orig_op_len;  /* Exclusive.  */
  int new_op_start = pc;
  int new_op_len = 7;
  int new_op_end = new_op_start + new_op_len;  /* Exclusive.  */
  int extra_elts = new_op_len - orig_op_len;
  int orig_nelts = exp->nelts;

  exp->nelts += extra_elts;
  exp->resize (exp->nelts);

  memmove (exp->elts + new_op_end, exp->elts + orig_op_end,
           EXP_ELEM_TO_BYTES (orig_nelts - orig_op_end));
...
at which point I realized that in theory extra_elts could be negative,
in which case the resize might reduce the amount of memory for
exp->elts, after which memmove would read out of bounds.  Not sure if
that is realistic or not.

Thanks,
- Tom

> gdb/ChangeLog
> 2020-12-02  Tom Tromey  <tom@tromey.com>
> 
> 	PR ada/26999
> 	* ada-lang.c (replace_operator_with_call): Rewrite.
> ---
>  gdb/ChangeLog  |  5 +++++
>  gdb/ada-lang.c | 31 +++++++++++++------------------
>  2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 906671155d0..7d062294aa5 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -4000,28 +4000,23 @@ replace_operator_with_call (expression_up *expp, int pc, int nargs,
>  			    int oplen, struct symbol *sym,
>  			    const struct block *block)
>  {
> -  /* A new expression, with 6 more elements (3 for funcall, 4 for function
> -     symbol, -oplen for operator being replaced).  */
> -  struct expression *newexp = (struct expression *)
> -    xzalloc (sizeof (struct expression)
> -	     + EXP_ELEM_TO_BYTES ((*expp)->nelts + 7 - oplen));
> +  /* We want to add 6 more elements (3 for funcall, 4 for function
> +     symbol, -OPLEN for operator being replaced) to the
> +     expression.  */
>    struct expression *exp = expp->get ();
> +  int save_nelts = exp->nelts;
> +  exp->nelts = exp->nelts + 7 - oplen;
> +  exp->resize (exp->nelts);
>  
> -  newexp->nelts = exp->nelts + 7 - oplen;
> -  newexp->language_defn = exp->language_defn;
> -  newexp->gdbarch = exp->gdbarch;
> -  memcpy (newexp->elts, exp->elts, EXP_ELEM_TO_BYTES (pc));
> -  memcpy (newexp->elts + pc + 7, exp->elts + pc + oplen,
> -	  EXP_ELEM_TO_BYTES (exp->nelts - pc - oplen));
> +  memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
> +	   EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
>  
> -  newexp->elts[pc].opcode = newexp->elts[pc + 2].opcode = OP_FUNCALL;
> -  newexp->elts[pc + 1].longconst = (LONGEST) nargs;
> +  exp->elts[pc].opcode = exp->elts[pc + 2].opcode = OP_FUNCALL;
> +  exp->elts[pc + 1].longconst = (LONGEST) nargs;
>  
> -  newexp->elts[pc + 3].opcode = newexp->elts[pc + 6].opcode = OP_VAR_VALUE;
> -  newexp->elts[pc + 4].block = block;
> -  newexp->elts[pc + 5].symbol = sym;
> -
> -  expp->reset (newexp);
> +  exp->elts[pc + 3].opcode = exp->elts[pc + 6].opcode = OP_VAR_VALUE;
> +  exp->elts[pc + 4].block = block;
> +  exp->elts[pc + 5].symbol = sym;
>  }
>  
>  /* Type-class predicates */
> 

Re: [PATCH] Fix struct expression regression

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom>   memmove (exp->elts + new_op_end, exp->elts + orig_op_end,
Tom>            EXP_ELEM_TO_BYTES (orig_nelts - orig_op_end));
Tom> ...
Tom> at which point I realized that in theory extra_elts could be negative,
Tom> in which case the resize might reduce the amount of memory for
Tom> exp->elts, after which memmove would read out of bounds.  Not sure if
Tom> that is realistic or not.

It seems to me that this must be a pre-existing latent bug, because the
new patch just reuses the existing rewriting logic -- it just changes it
to work in-place.

I suspect the bug can't occur in practice, because
replace_operator_with_call is only called for certain opcodes, and I
think none of these can be as long as the replacement:

    case OP_VAR_VALUE:
[...]
    case BINOP_ADD:
    case BINOP_SUB:
    case BINOP_MUL:
    case BINOP_DIV:
    case BINOP_REM:
    case BINOP_MOD:
    case BINOP_CONCAT:
    case BINOP_BITWISE_AND:
    case BINOP_BITWISE_IOR:
    case BINOP_BITWISE_XOR:
    case BINOP_EQUAL:
    case BINOP_NOTEQUAL:
    case BINOP_LESS:
    case BINOP_GTR:
    case BINOP_LEQ:
    case BINOP_GEQ:
    case BINOP_EXP:
    case UNOP_NEG:
    case UNOP_PLUS:
    case UNOP_LOGICAL_NOT:
    case UNOP_ABS:

Tom

[gdb/ada] Handle shrink resize in replace_operator_with_call

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

[ was: Re: [PATCH] Fix struct expression regression ]

On 12/4/20 11:13 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom>   memmove (exp->elts + new_op_end, exp->elts + orig_op_end,
> Tom>            EXP_ELEM_TO_BYTES (orig_nelts - orig_op_end));
> Tom> ...
> Tom> at which point I realized that in theory extra_elts could be negative,
> Tom> in which case the resize might reduce the amount of memory for
> Tom> exp->elts, after which memmove would read out of bounds.  Not sure if
> Tom> that is realistic or not.
> 
> It seems to me that this must be a pre-existing latent bug, because the
> new patch just reuses the existing rewriting logic -- it just changes it
> to work in-place.
> 
> I suspect the bug can't occur in practice, because
> replace_operator_with_call is only called for certain opcodes, and I
> think none of these can be as long as the replacement:
> 
>     case OP_VAR_VALUE:
> [...]
>     case BINOP_ADD:
>     case BINOP_SUB:
>     case BINOP_MUL:
>     case BINOP_DIV:
>     case BINOP_REM:
>     case BINOP_MOD:
>     case BINOP_CONCAT:
>     case BINOP_BITWISE_AND:
>     case BINOP_BITWISE_IOR:
>     case BINOP_BITWISE_XOR:
>     case BINOP_EQUAL:
>     case BINOP_NOTEQUAL:
>     case BINOP_LESS:
>     case BINOP_GTR:
>     case BINOP_LEQ:
>     case BINOP_GEQ:
>     case BINOP_EXP:
>     case UNOP_NEG:
>     case UNOP_PLUS:
>     case UNOP_LOGICAL_NOT:
>     case UNOP_ABS:

Ack.  I propose to fix this, in this follow-up patch.

I'll put this through testing.

Thanks,
- Tom


[-- Attachment #2: 0006-gdb-ada-Handle-shrink-resize-in-replace_operator_with_call.patch --]
[-- Type: text/x-patch, Size: 1580 bytes --]

[gdb/ada] Handle shrink resize in replace_operator_with_call

In replace_operator_with_call, we resize the elts array like this:
...
  exp->nelts = exp->nelts + 7 - oplen;
  exp->resize (exp->nelts);
...

Although all the current callers ensure that the new size is bigger, it could
also be smaller, in which case the following memmove possibly reads out of
bounds:
...
   memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
           EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
...

Fix this by doing the resize after the memmove in case the new size is
smaller.

Tested on x86_64-linux.

gdb/ChangeLog:

2020-12-05  Tom de Vries  <tdevries@suse.de>

	* ada-lang.c (replace_operator_with_call): Handle shrink resize.

---
 gdb/ada-lang.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7d062294aa..8a1d9df541 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4005,11 +4005,15 @@ replace_operator_with_call (expression_up *expp, int pc, int nargs,
      expression.  */
   struct expression *exp = expp->get ();
   int save_nelts = exp->nelts;
-  exp->nelts = exp->nelts + 7 - oplen;
-  exp->resize (exp->nelts);
+  int extra_elts = 7 - oplen;
+  exp->nelts += extra_elts;
 
+  if (extra_elts > 0)
+    exp->resize (exp->nelts);
   memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
 	   EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
+  if (extra_elts < 0)
+    exp->resize (exp->nelts);
 
   exp->elts[pc].opcode = exp->elts[pc + 2].opcode = OP_FUNCALL;
   exp->elts[pc + 1].longconst = (LONGEST) nargs;

Re: [gdb/ada] Handle shrink resize in replace_operator_with_call

[Tom Tromey]

Tom> Ack.  I propose to fix this, in this follow-up patch.

I'm checking mine in now.

Tom> +  int extra_elts = 7 - oplen;
Tom> +  exp->nelts += extra_elts;
 
Tom> +  if (extra_elts > 0)
Tom> +    exp->resize (exp->nelts);
Tom>    memmove (exp->elts + pc + 7, exp->elts + pc + oplen,
Tom>  	   EXP_ELEM_TO_BYTES (save_nelts - pc - oplen));
Tom> +  if (extra_elts < 0)
Tom> +    exp->resize (exp->nelts);
 
Looks reasonable.

Tom