On Fri, Apr 04, 2014 at 03:41:38PM +0200, Lukas Slebodnik wrote:
On (04/04/14 15:18), Jakub Hrozek wrote:
>On Thu, Apr 03, 2014 at 07:11:37PM +0200, Jakub Hrozek wrote:
>> On Thu, Mar 20, 2014 at 05:53:31PM +0100, Lukas Slebodnik wrote:
>> > On (20/03/14 17:21), Jakub Hrozek wrote:
>> > >On Thu, Mar 20, 2014 at 05:00:00PM +0100, Sumit Bose wrote:
>> > >> On Thu, Mar 20, 2014 at 04:20:59PM +0100, Lukas Slebodnik wrote:
>> > >> > ehlo,
>> > >> >
>> > >> > debug_prg_name is used in debug_fn and it was allocated
under
>> > >> > talloc context "kr". The variable "kr"
was removed before the last debug
>> > >> > messages in function main. It is very little change that it
will be overridden.
>> > >> >
>> > >> > It is possible to see this issue with exported environment
variable
>> > >> > TALLOC_FREE_FILL=255
>> > >>
>> > >> I'm pretty sure the patch works as expected and fixes the
isssue. But I
>> > >> wonder if a different approach might be better? I think it does
not make
>> > >> sense to allocate debug_prg_name on a given talloc context but
that it
>> > >> would be better to just allocate it on NULL. This is e.g. done in
the
>> > >> ldap_child (here a talloc_free() is missing on exit but this would
be a
>> > >> different story). Then debug_prg_name can even be allocate before
kr
>> > >> and the debug messages for a failed allocation of kr can use the
right
>> > >> program name and not "sssd".
>> > >
>> > >I agree, because also given that krb5_child is a short lived process,
>> > >we don't care too much about possible leaks.
>> > No problem.
>> >
>> > New version attached.
>> >
>> > LS
>>
>> This version works for me, do you want to also add a talloc_free() on
>> exit to be clean or do you also consider this not needed for short-lived
>> processes?
>
>Thinking again, it would be nicer to explicitly free the string on child
>exit. Yes, the leak it's not a big deal for a short-lived process but it
>would read better.
With the first version of patch string was freed, because it was alocated
under "kr" context.
Now, you should decide which version do you want to push :-)
LS
I think it's easier to explain with a patch :-)