diff mbox

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

Message ID 20180802212613.29813-2-philippe.waroquiers@skynet.be
State New
Headers show

Commit Message

Philippe Waroquiers Aug. 2, 2018, 9:26 p.m. UTC
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(-)

Comments

Tom Tromey Aug. 3, 2018, 6:28 p.m. UTC | #1
>>>>> "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
Tom Tromey Aug. 21, 2018, 6 p.m. UTC | #2
>>>>> "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
Philippe Waroquiers Aug. 25, 2018, 7:17 p.m. UTC | #3
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
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6b6e1f6c25..7761bdb496 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1219,9 +1219,17 @@  commands_command_1 (const char *arg, int from_tty,
 		    struct command_line *control)
 {
   counted_command_line cmd;
+  /* cmd_read will be true once we have read cmd.  Note that cmd might still be
+     NULL after the call to read_command_lines if the user provides an empty
+     list of command by just typing "end".  */
+  bool cmd_read = false;
 
   std::string new_arg;
 
+  /* arg might be an input line that might be released when reading
+     new input lines for the list of commands.  So, build a new arg
+     to keep the input alive during the map_breakpoint_numbers call,
+     even if the new arg is just a copy of arg.  */
   if (arg == NULL || !*arg)
     {
       if (breakpoint_count - prev_breakpoint_count > 1)
@@ -1231,12 +1239,18 @@  commands_command_1 (const char *arg, int from_tty,
 	new_arg = string_printf ("%d", breakpoint_count);
       arg = new_arg.c_str ();
     }
+  else
+    {
+      new_arg = arg;
+      arg = new_arg.c_str ();
+    }
 
   map_breakpoint_numbers
     (arg, [&] (breakpoint *b)
      {
-       if (cmd == NULL)
+       if (!cmd_read)
 	 {
+	   gdb_assert (cmd == NULL);
 	   if (control != NULL)
 	     cmd = control->body_list_0;
 	   else
@@ -1256,6 +1270,7 @@  commands_command_1 (const char *arg, int from_tty,
 
 	       cmd = read_command_lines (str.c_str (), from_tty, 1, validator);
 	     }
+	   cmd_read = true;
 	 }
 
        /* If a breakpoint was on the list more than once, we don't need to