This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Stop leaking extra_string
- From: Pedro Alves <palves at redhat dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Fri, 22 Mar 2013 17:59:38 +0000
- Subject: Re: [RFA] Stop leaking extra_string
- References: <514A08A5 dot 6020504 at redhat dot com> <514A0A9E dot 1090105 at redhat dot com> <514A358F dot 3080403 at redhat dot com>
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