[RFA] Stop leaking extra_string

[RFA] Stop leaking extra_string

[Keith Seitz]

Hi,

While reviewing my own patches, I noticed that struct 
breakpoint.extra_string is never freed. You can see this fairly easily 
in mi-breakpoint-changed.exp (or any dprintf) with valgrind (or an 
assert in base_breakpoint_dtor).

I've also added a comment to breakpoint.h to mention that this member is 
allocated.

Ok?
Keith

ChangeLog
2013-03-20  Keith Seitz  <keiths@redhat.com>

	* breakpoint.h (struct breakpoint): Add comment to
	extra_string indicating that this member is mallod'd.
	* breakpoint.c (base_breakpoint_dtor): Free extra_string.


Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.193
diff -u -p -r1.193 breakpoint.h
--- breakpoint.h	3 Feb 2013 15:57:06 -0000	1.193
+++ breakpoint.h	20 Mar 2013 14:31:28 -0000
@@ -726,7 +726,8 @@ struct breakpoint
         there is no condition.  */
      char *cond_string;

-    /* String form of extra parameters, or NULL if there are none.  */
+    /* String form of extra parameters, or NULL if there are none.
+     Malloc'd.  */
      char *extra_string;

      /* Holds the address of the related watchpoint_scope breakpoint
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.746
diff -u -p -r1.746 breakpoint.c
--- breakpoint.c	13 Mar 2013 18:34:53 -0000	1.746
+++ breakpoint.c	20 Mar 2013 14:31:29 -0000
@@ -12788,6 +12788,7 @@ base_breakpoint_dtor (struct breakpoint
  {
    decref_counted_command_line (&self->commands);
    xfree (self->cond_string);
+  xfree (self->extra_string);
    xfree (self->addr_string);
    xfree (self->filter);
    xfree (self->addr_string_range_end);

Re: [RFA] Stop leaking extra_string

[Pedro Alves]

On 03/20/2013 07:06 PM, Keith Seitz wrote:

> ChangeLog
> 2013-03-20  Keith Seitz  <keiths@redhat.com>
> 
>     * breakpoint.h (struct breakpoint): Add comment to
>     extra_string indicating that this member is mallod'd.
>     * breakpoint.c (base_breakpoint_dtor): Free extra_string.

This is fine, thanks.

When we get to this bit in addr_string_to_sals (called through
breakpoint_re_set_default):

	  if (cond_string)
	    b->cond_string = cond_string;
	  b->thread = thread;
	  b->task = task;
	  if (extra_string)
	    b->extra_string = extra_string;
	  b->condition_not_parsed = 0;

Is b->extra_string always NULL here, or could we be
leaking it here too?

-- 
Pedro Alves

Re: [RFA] Stop leaking extra_string

[Keith Seitz]

On 03/20/2013 12:14 PM, Pedro Alves wrote:

> When we get to this bit in addr_string_to_sals (called through
> breakpoint_re_set_default):
>
> 	  if (cond_string)
> 	    b->cond_string = cond_string;
> 	  b->thread = thread;
> 	  b->task = task;
> 	  if (extra_string)
> 	    b->extra_string = extra_string;
> 	  b->condition_not_parsed = 0;
>
> Is b->extra_string always NULL here, or could we be
> leaking it here too?

I don't think that is possible right now.

When extra_string is set by find_condition_and_thread, 
init_breakpoint_sal (called from ops->create_breakpoints_sal) will error 
if extra_string isn't NULL (for non-dprintf breakpoints).

So the only way to get extra_string != NULL in breakpoint_re_set is by 
setting a pending dprintf breakpoint, which doesn't even work because 
any pending breakpoint will automatically have extra_string set to NULL 
in create_breakpoint.

But this is all largely academic for two reasons: 1) Adding an xfree 
there wouldn't hurt; 2) I'm going to submit a patch to do just that 
because I am changing it so that extra_string could be set. :-)

I've committed my original patch. Thank you for taking a look at this.

Keith

Re: [RFA] Stop leaking extra_string

[Pedro Alves]

Thanks for the analysis, Keith.

On 03/20/2013 10:17 PM, Keith Seitz wrote:
> On 03/20/2013 12:14 PM, Pedro Alves wrote:
> 
>> When we get to this bit in addr_string_to_sals (called through
>> breakpoint_re_set_default):
>>
>>       if (cond_string)
>>         b->cond_string = cond_string;
>>       b->thread = thread;
>>       b->task = task;
>>       if (extra_string)
>>         b->extra_string = extra_string;
>>       b->condition_not_parsed = 0;
>>
>> Is b->extra_string always NULL here, or could we be
>> leaking it here too?
> 
> I don't think that is possible right now.
> 
> When extra_string is set by find_condition_and_thread, init_breakpoint_sal (called from ops->create_breakpoints_sal) will error if extra_string isn't NULL (for non-dprintf breakpoints).
> 
> So the only way to get extra_string != NULL in breakpoint_re_set is by setting a pending dprintf breakpoint, which doesn't even work because any pending breakpoint will automatically have extra_string set to NULL in create_breakpoint.
> 
> But this is all largely academic for two reasons: 1) Adding an xfree there wouldn't hurt; 2) I'm going to submit a patch to do just that because I am changing it so that extra_string could be set. :-)

:-)  It's fine with me to not bother.  An assert would be fine
too, and it might be better.

Still looking at addr_string_to_sals, it looks like

	  if (cond_string)
	    b->cond_string = cond_string;
	  b->thread = thread;
	  b->task = task;
	  if (extra_string)
	    b->extra_string = extra_string;

the "if (extra_string)" test looks unnecessary then.
I wonder if the "cond_string" one has any meaning.  It
reads as if the code is trying to preserve the original
condition string if resolving a pending breakpoint ends
up finding no condition was really there to begin with.

b->cond_string does leak here, though, I think?
Unlike b->extra_string, b->cond_string isn't always left NULL
when create_breakpoint creates a pending breakpoint:

      b->addr_string = copy_arg;
      if (parse_condition_and_thread)
	b->cond_string = NULL;
      else
	{
	  /* Create a private copy of condition string.  */
	  if (cond_string)
	    {
	      cond_string = xstrdup (cond_string);
	      make_cleanup (xfree, cond_string);
	    }
	  b->cond_string = cond_string;
	}
      b->extra_string = NULL;
      b->ignore_count = ignore_count;
      b->disposition = tempflag ? disp_del : disp_donttouch;
      b->condition_not_parsed = 1;

and we end up with b->condition_not_parsed set even in the
!parse_condition_and_thread case.  That means a later reset
ends up in the addr_string_to_sals bit in question, and overwrites
the b->cond_string set here then.  That doesn't look right.

Hmm, wait.  I'm having a déjà vu.  I was working on something around
pending breakpoints and the condition a while ago, but never finished
it.  Damn, I forget all the details now:

http://sourceware.org/ml/gdb-patches/2012-08/msg00092.html

> I've committed my original patch. Thank you for taking a look at this.

Thanks.

-- 
Pedro Alves