Re: [PATCH 2/3] gdb/breakpoint: set the condition exp after parsing the condition successfully

[Simon Marchi <simark@simark.ca>]

On 2020-06-29 9:48 a.m., Tankut Baris Aktemur via Gdb-patches wrote:
> In 'set_breakpoint_condition', GDB resets the condition expressions
> before parsing the condition input by the user.  This leads to the
> problem of losing the condition expressions if the new condition
> does not parse successfully and is thus rejected.
> 
> For instance:
> 
>   $ gdb ./test
>   Reading symbols from ./test...
>   (gdb) start
>   Temporary breakpoint 1 at 0x114e: file test.c, line 4.
>   Starting program: test
> 
>   Temporary breakpoint 1, main () at test.c:4
>   4         int a = 10;
>   (gdb) break 5
>   Breakpoint 2 at 0x555555555155: file test.c, line 5.
> 
> Now define a condition that would evaluate to false.  Next, attempt
> to overwrite that with an invalid condition:
> 
>   (gdb) cond 2 a == 999
>   (gdb) cond 2 gibberish
>   No symbol "gibberish" in current context.
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   2       breakpoint     keep y   0x0000555555555155 in main at test.c:5
>           stop only if a == 999
> 
> It appears as if the bad condition is successfully rejected.  But if we
> resume the program, we see that we hit the breakpoint although the condition
> would evaluate to false.
> 
>   (gdb) continue
>   Continuing.
> 
>   Breakpoint 2, main () at test.c:5
>   5         a = a + 1; /* break-here */
> 
> Fix the problem by not resetting the condition expressions before
> parsing the condition input.
> 
> Suppose the fix is applied.  A similar problem could occur if the
> condition is valid, but has "junk" at the end.  In this case, parsing
> succeeds, but an error is raised immediately after.  It is too late,
> though; the condition expression is already updated.
> 
> For instance:
> 
>   $ gdb ./test
>   Reading symbols from ./test...
>   (gdb) start
>   Temporary breakpoint 1 at 0x114e: file test.c, line 4.
>   Starting program: test
> 
>   Temporary breakpoint 1, main () at test.c:4
>   4         int a = 10;
>   (gdb) break 5
>   Breakpoint 2 at 0x555555555155: file test.c, line 5.
>   (gdb) cond 2 a == 999
>   (gdb) cond 2 a == 10 if
>   Junk at end of expression
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   2       breakpoint     keep y   0x0000555555555155 in main at test.c:5
>           stop only if a == 999
>   (gdb) c
>   Continuing.
> 
>   Breakpoint 2, main () at test.c:5
>   5         a = a + 1; /* break-here */
>   (gdb)
> 
> We should not have hit the breakpoint because the condition would
> evaluate to false.
> 
> Fix this problem by updating the condition expression of the breakpoint
> after parsing the input successfully and checking that there is no
> remaining junk.
> 
> gdb/ChangeLog:
> 2020-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* breakpoint.c (set_breakpoint_condition): Update the condition
> 	expressions after checking that the input condition string parses
> 	successfully and does not contain junk at the end.
> 
> gdb/testsuite/ChangeLog:
> 2020-06-29  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>
> 
> 	* gdb.base/condbreak-bad.exp: Extend the test with scenarios
> 	that attempt to overwrite and existing condition with a condition
> 	that fails parsing and also with a condition that parses fine
> 	but contains junk at the end.
> ---
>  gdb/breakpoint.c                         | 46 +++++++------
>  gdb/testsuite/gdb.base/condbreak-bad.exp | 88 ++++++++++++++++++++++++
>  2 files changed, 112 insertions(+), 22 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 1fc2d1b8966..abda470fe21 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -834,30 +834,30 @@ void
>  set_breakpoint_condition (struct breakpoint *b, const char *exp,
>  			  int from_tty)
>  {
> -  if (is_watchpoint (b))
> -    {
> -      struct watchpoint *w = (struct watchpoint *) b;
> -
> -      w->cond_exp.reset ();
> -    }
> -  else
> +  if (*exp == 0)
>      {
> -      struct bp_location *loc;
> +      xfree (b->cond_string);
> +      b->cond_string = nullptr;
>  
> -      for (loc = b->loc; loc; loc = loc->next)
> +      if (is_watchpoint (b))
>  	{
> -	  loc->cond.reset ();
> +	  struct watchpoint *w = (struct watchpoint *) b;
>  
> -	  /* No need to free the condition agent expression
> -	     bytecode (if we have one).  We will handle this
> -	     when we go through update_global_location_list.  */
> +	  w->cond_exp.reset ();
>  	}
> -    }
> +      else
> +	{
> +	  struct bp_location *loc;
>  
> -  if (*exp == 0)
> -    {
> -      xfree (b->cond_string);
> -      b->cond_string = nullptr;
> +	  for (loc = b->loc; loc; loc = loc->next)
> +	    {
> +	      loc->cond.reset ();
> +
> +	      /* No need to free the condition agent expression
> +		 bytecode (if we have one).  We will handle this
> +		 when we go through update_global_location_list.  */
> +	    }
> +	}

Since you touch this, might as well declare the `bp_location *loc` in the for loop
and use `loc != nullptr`.

Otherwise, this LGTM.

Simon