[patch] Fix double free on error while inserting the breakpoint

[patch] Fix double free on error while inserting the breakpoint

[Jan Kratochvil]

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

Hi,

SEGV reproducer:

x86 requires to build GDB with -lmcheck to make the crash reproducible.
Therefore no testsuite testcase is provided.
./gdb -nx -ex start -ex 'set breakpoint always-inserted on' -ex 'b *0' -ex 'delete 2' ./gdb

(Found on ia64 without -lmcheck.)


Regards,
Jan

[-- Attachment #2: gdb-breakpoint-free.patch --]
[-- Type: text/plain, Size: 1340 bytes --]

2008-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix double free on error inserting the breakpoint instruction.
	* breakpoint.c (create_breakpoints): Move the
	update_global_location_list call to ...
	(break_command_really): ... here together with the second local call
	both unified after all the cleanups.

--- gdb/breakpoint.c	22 Nov 2008 04:41:45 -0000	1.362
+++ gdb/breakpoint.c	22 Nov 2008 20:10:07 -0000
@@ -5257,8 +5257,6 @@ create_breakpoints (struct symtabs_and_l
 			 cond_string, type, disposition,
 			 thread, ignore_count, ops, from_tty);
     }
-
-  update_global_location_list (1);
 }
 
 /* Parse ARG which is assumed to be a SAL specification possibly
@@ -5579,7 +5577,6 @@ break_command_really (char *arg, char *c
       b->condition_not_parsed = 1;
       b->ops = ops;
 
-      update_global_location_list (1);
       mention (b);
     }
   
@@ -5591,6 +5588,11 @@ break_command_really (char *arg, char *c
   discard_cleanups (breakpoint_chain);
   /* But cleanup everything else. */
   do_cleanups (old_chain);
+
+  /* Have already BREAKPOINT_CHAIN discarded as we may get an exception while
+     inserting the breakpoints which would double-free the resources both by
+     BREAKPOINT_CHAIN now and during DELETE_BREAKPOINT in the future.  */
+  update_global_location_list (1);
 }
 
 /* Set a breakpoint. 

Re: [patch] Fix double free on error while inserting the breakpoint

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> x86 requires to build GDB with -lmcheck to make the crash reproducible.

FWIW, I reproduced this with valgrind.

Jan> 2008-11-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	Fix double free on error inserting the breakpoint instruction.
Jan> 	* breakpoint.c (create_breakpoints): Move the
Jan> 	update_global_location_list call to ...
Jan> 	(break_command_really): ... here together with the second local call
Jan> 	both unified after all the cleanups.

I like this but I am unsure whether it is ok to move the call to
update_global_location_list past the call to mention.

Jan> +  /* Have already BREAKPOINT_CHAIN discarded as we may get an exception while
Jan> +     inserting the breakpoints which would double-free the resources both by
Jan> +     BREAKPOINT_CHAIN now and during DELETE_BREAKPOINT in the future.  */
Jan> +  update_global_location_list (1);

I found this comment pretty hard to follow.  I think the code would be
pretty clear without it.

Tom

Re: [patch] Fix double free on error while inserting the breakpoint

[Jan Kratochvil]

On Thu, 23 Apr 2009 01:11:25 +0200, Tom Tromey wrote:
> I like this but I am unsure whether it is ok to move the call to
> update_global_location_list past the call to mention.

As in this case the breakpoint is pending and thus it was created by
set_raw_breakpoint_without_location and so update_global_location_list is
a nop for it.  So I do not think it is a problem to move it.


> Jan> +  /* Have already BREAKPOINT_CHAIN discarded as we may get an exception while
> Jan> +     inserting the breakpoints which would double-free the resources both by
> Jan> +     BREAKPOINT_CHAIN now and during DELETE_BREAKPOINT in the future.  */
> Jan> +  update_global_location_list (1);
> 
> I found this comment pretty hard to follow.  I think the code would be
> pretty clear without it.

The ordering of
  discard_cleanups (breakpoint_chain);
vs.
  update_global_location_list (1);
I find important to note there.  At least tried if the new comment will pass.


Thanks,
Jan


2009-04-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix double free on error inserting the breakpoint instruction.
	* breakpoint.c (create_breakpoints): Move the
	update_global_location_list call to ...
	(break_command_really): ... here together with the second local call
	both unified after all the cleanups.

--- gdb/breakpoint.c	31 Mar 2009 16:44:17 -0000	1.390
+++ gdb/breakpoint.c	23 Apr 2009 20:13:26 -0000
@@ -5458,8 +5458,6 @@ create_breakpoints (struct symtabs_and_l
 			 cond_string, type, disposition,
 			 thread, task, ignore_count, ops, from_tty, enabled);
     }
-
-  update_global_location_list (1);
 }
 
 /* Parse ARG which is assumed to be a SAL specification possibly
@@ -5800,7 +5798,6 @@ break_command_really (char *arg, char *c
       b->ops = ops;
       b->enable_state = enabled ? bp_enabled : bp_disabled;
 
-      update_global_location_list (1);
       mention (b);
     }
   
@@ -5812,6 +5809,9 @@ break_command_really (char *arg, char *c
   discard_cleanups (breakpoint_chain);
   /* But cleanup everything else. */
   do_cleanups (old_chain);
+
+  /* error call may happen here - have BREAKPOINT_CHAIN already discarded.  */
+  update_global_location_list (1);
 }
 
 /* Set a breakpoint. 

Re: [patch] Fix double free on error while inserting the breakpoint

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Tom> I like this but I am unsure whether it is ok to move the call to
Tom> update_global_location_list past the call to mention.

Jan> As in this case the breakpoint is pending and thus it was created by
Jan> set_raw_breakpoint_without_location and so update_global_location_list is
Jan> a nop for it.  So I do not think it is a problem to move it.

Ok, that is convincing :)

Jan> 2009-04-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	Fix double free on error inserting the breakpoint instruction.
Jan> 	* breakpoint.c (create_breakpoints): Move the
Jan> 	update_global_location_list call to ...
Jan> 	(break_command_really): ... here together with the second local call
Jan> 	both unified after all the cleanups.

Looks good to me, please check it in.
Thanks.

Tom

Re: [patch] Fix double free on error while inserting the breakpoint

[Jan Kratochvil]

On Thu, 23 Apr 2009 23:25:32 +0200, Tom Tromey wrote:
> Looks good to me, please check it in.

Checked-in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2009-04/msg00172.html

2009-04-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix double free on error inserting the breakpoint instruction.
	* breakpoint.c (create_breakpoints): Move the
	update_global_location_list call to ...
	(break_command_really): ... here together with the second local call
	both unified after all the cleanups.

--- src/gdb/breakpoint.c	2009/03/31 16:44:17	1.390
+++ src/gdb/breakpoint.c	2009/04/23 22:38:24	1.391
@@ -5458,8 +5458,6 @@
 			 cond_string, type, disposition,
 			 thread, task, ignore_count, ops, from_tty, enabled);
     }
-
-  update_global_location_list (1);
 }
 
 /* Parse ARG which is assumed to be a SAL specification possibly
@@ -5800,7 +5798,6 @@
       b->ops = ops;
       b->enable_state = enabled ? bp_enabled : bp_disabled;
 
-      update_global_location_list (1);
       mention (b);
     }
   
@@ -5812,6 +5809,9 @@
   discard_cleanups (breakpoint_chain);
   /* But cleanup everything else. */
   do_cleanups (old_chain);
+
+  /* error call may happen here - have BREAKPOINT_CHAIN already discarded.  */
+  update_global_location_list (1);
 }
 
 /* Set a breakpoint.