Re: [RFA] Fix use after free introduced by $_hit_bpnum/$_hit_locno variables.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Mon, 2022-11-21 at 10:10 -0500, Simon Marchi wrote:
> 
> On 11/20/22 05:44, Philippe Waroquiers via Gdb-patches wrote:
> > If the commands of the bpstat bs contain commands such as step or next or
> > continue, the BS and its commands are freed by execute_control_command.
> > 
> > So, we cannot remember the BS that was printed. Instead, remember
> > the bpnum and locno.
> > 
> > Regtested on debian/amd64 and re-run a few tests under valgrind.
> > ---
> >  gdb/breakpoint.c | 62 ++++++++++++++++++++++++++++--------------------
> >  1 file changed, 36 insertions(+), 26 deletions(-)
> > 
> > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> > index 7f6400db624..5b691673a0e 100644
> > --- a/gdb/breakpoint.c
> > +++ b/gdb/breakpoint.c
> > @@ -4574,18 +4574,17 @@ command_line_is_silent (struct command_line *cmd)
> >    return cmd && (strcmp ("silent", cmd->line) == 0);
> >  }
> > 
> > -/* Sets the $_hit_bpnum and $_hit_locno to the bpnum and locno of bs.  */
> > +/* Sets the $_hit_bpnum and $_hit_locno to bpnum and locno.
> > +   A locno 0 is changed to 1 to e.g. let the user do
> > +     (gdb) disable $_hit_bpnum.$_hit_locno
> > +   for a single location breakpoint.  */
> > +
> >  static void
> > -set_hit_convenience_vars (bpstat *bs)
> > +set_hit_convenience_vars (int bpnum, int locno)
> >  {
> > -  const struct breakpoint *b = bs->breakpoint_at;
> > -  if (b != nullptr)
> > -    {
> > -      int locno = bpstat_locno (bs);
> > -      set_internalvar_integer (lookup_internalvar ("_hit_bpnum"), b->number);
> > -      set_internalvar_integer (lookup_internalvar ("_hit_locno"),
> > -			       (locno > 0 ? locno : 1));
> > -    }
> > +  set_internalvar_integer (lookup_internalvar ("_hit_bpnum"), bpnum);
> > +  set_internalvar_integer (lookup_internalvar ("_hit_locno"),
> > +			   (locno > 0 ? locno : 1));
> 
> I haven't had time to look at the original patch (sorry, just so many
> things to do), but the naming caught my attention.  We have both "num"
> and "no" as abbreviations for "number".  It looks a bit inconsistent.
> Was it on purpose?  I don't so much about the variable names, but the
> names exposed to the user.

There was no particular specific reason. The names were first $bkptno and $locno.
Tom gave comments about the fact that new conv vars should start with $_
and that $bkptno was not consistent with $bpnum.
We need 2 different names to avoid making an incompatible change to $bpnum behaviour.
As the semantic of $bkptno and $locno was not clear from the names,
these were changed to $_hit_bpnum  (where bpnum trailer was now chosen on purpose
to be consistent in terminology with the $bpnum set by the "breakpoint" command).

GDB (e.g. user manual and/or code) uses various conventions for
"integer" identifying something (such as "thread ID", TASKNO, INFNO or bkptno
that was already used in the mi interface). 
"locno" terminology was used because the trailing "no" seems used relatively
frequently.
This can of course still be changed if deemed better.

> 
> But the fix LGTM:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon
Thanks for the review, pushed.
Philippe