[RFA,1/2] Fix regressions for multi breakpoints command line setting/clearing

  When setting commands for several breakpoints
(such as with  'commands 1 2'),
the '1 2' is passed to commands_command_1 as const char* arg.
This arg can however be freed, as this is in a just read input line
that might be freed by the call to read_command_lines.
This patch fixes the problem by ensuring that arg is always
a locally allocated string managed via the std::string new_arg.
Note that this was the logic before the regression was introduced :
the use after free was introduced when (partially) undoing the patch
done by Pedro in 896b6bda6904765f36692d76a37b99c0412ca9ae.

Note that such problem could also (or should?) be fixed by reworking
the way read_command_lines manages the memory of input lines, so as
to solve this globally, and not at all places where possibly a
'being handled' line of input might be re-allocated.
Tom is looking at this, but in the meantime,
this patch just goes back to the previous way to avoid the
error (and be able to have the regression tests for the
functional regression working).
Without the patch, the test fails the following way:
  ...
  commands 2 3
  Type commands for breakpoint(s) 2 3, one per line.
  End with a line saying just "end".
  >  print 1234321
  >end
  No breakpoint number 4321.
  ...
(and under valgrind, the above reports access to freed memory).

breakpoint.c is also modified to fix the regression introduced
when clearing commands of several breakpoint by giving an empty
list of commands, by just typing "end".
GDB should read an empty list of command once, but it reads
it for each breakpoint, as an empty list of command is NULL.

gdb/ChangeLog

2018-08-02  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* breakpoint.c (commands_command_1): New boolean cmd_read
	to detect cmd was already read. Always allocate a new_arg
	c_str to avoid accessing arg after some calls to
	read_command_line as this can free arg memory.
---
 gdb/breakpoint.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> Note that such problem could also (or should?) be fixed by reworking
Philippe> the way read_command_lines manages the memory of input lines, so as
Philippe> to solve this globally, and not at all places where possibly a
Philippe> 'being handled' line of input might be re-allocated.
Philippe> Tom is looking at this, but in the meantime,
Philippe> this patch just goes back to the previous way to avoid the
Philippe> error (and be able to have the regression tests for the
Philippe> functional regression working).

Yeah, I'd rather do the deeper fix, because otherwise we will have an
API that's difficult to use correctly.

But if I can't get it finished soon, I'll approve this.

Tom
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> 2018-08-02  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

Philippe> 	* breakpoint.c (commands_command_1): New boolean cmd_read
Philippe> 	to detect cmd was already read. Always allocate a new_arg
Philippe> 	c_str to avoid accessing arg after some calls to
Philippe> 	read_command_line as this can free arg memory.

Hi.  I have been meaning to update this to account for the fix I
recently put in, but I've been very ill and so I haven't been able to.

Philippe> +  else
Philippe> +    {
Philippe> +      new_arg = arg;
Philippe> +      arg = new_arg.c_str ();
Philippe> +    }

I think it should work ok now if you just remove this hunk.

Tom
  

On Tue, 2018-08-21 at 12:00 -0600, Tom Tromey wrote
> Philippe> +  else
> Philippe> +    {
> Philippe> +      new_arg = arg;
> Philippe> +      arg = new_arg.c_str ();
> Philippe> +    }
> 
> I think it should work ok now if you just remove this hunk.
Effectively, that bit of code is not needed anymore, thanks to the
fix you did for the use after free.

I will update the patch (and the test according to the comments
of Pedro), and resubmit an RFA.

Thanks

Philippe