On Tue, Apr 08, 2014 at 11:02:34AM +0200, Lukas Slebodnik wrote:
On (07/04/14 22:21), Sumit Bose wrote:
>On Mon, Apr 07, 2014 at 09:20:50PM +0200, Lukas Slebodnik wrote:
>> On (07/04/14 21:01), Sumit Bose wrote:
>> >On Mon, Apr 07, 2014 at 08:39:07PM +0200, Jakub Hrozek wrote:
>> >> On Mon, Apr 07, 2014 at 08:35:20PM +0200, Lukas Slebodnik wrote:
>> >> > On (07/04/14 20:30), Jakub Hrozek wrote:
>> >> > >On Mon, Apr 07, 2014 at 08:03:32PM +0200, Lukas Slebodnik
wrote:
>> >> > >> On (07/04/14 18:53), Jakub Hrozek wrote:
>> >> > >> >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 :-)
>> >> > >>
>> >> > >> You didn't test your patch :-)
>> >> > >
>> >> > >I left that to you :-)
>> >> > >
>> >> > >See another proposal below:
>> >> > >
>> >> > >>
>> >> > >> diff --git a/src/providers/krb5/krb5_child.c
b/src/providers/krb5/krb5_child.c
>> >> > >> index
764f6ac7bf57b1f7d882961b7c6fa518818aaf23..aec0d9389dd4f3ae005b73ff12ca8293cee7560f 100644
>> >> > >> --- a/src/providers/krb5/krb5_child.c
>> >> > >> +++ b/src/providers/krb5/krb5_child.c
>> >> > >> @@ -2013,6 +2013,7 @@ int main(int argc, const char
*argv[])
>> >> > >> DEBUG(SSSDBG_CRIT_FAILURE, "talloc
failed.\n");
>> >> > >> exit(-1);
>> >> > >> }
>> >> > >> + talloc_steal(kr, debug_prg_name);
>> >> > >>
>> >> > >> if (debug_fd != -1) {
>> >> > >> ret = set_debug_file_from_fd(debug_fd);
>> >> > >>
>> >> > >> // snip
>> >> > >> done:
>> >> > >> krb5_cleanup(kr);
>> >> > >> talloc_free(kr);
>> >> > >> ^^^^^^^^^^^^^^^
>> >> > >> //debug_prg_name is freed
>> >> > >>
>> >> > >> if (ret == EOK) {
>> >> > >> DEBUG(SSSDBG_TRACE_FUNC, "krb5_child
completed successfully\n");
>> >> > >> ^^^^^^
>> >> > >> // use after free
>> >> > > talloc_free(tmp_str);
>> >> > >> exit(0);
>> >> > >> } else {
>> >> > >> DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child
failed!\n");
>> >> > >> ^^^^^^
>> >> > >> // use after free
>> >> > > talloc_free(tmp_str);
>> >> > >> exit(-1);
>> >> > >> }
>> >> > >>
>> >> > >
>> >> > >Or just:
>> >> > > done:
>> >> > > krb5_cleanup(kr);
>> >> > > talloc_free(kr);
>> >> > > if (ret == EOK) {
>> >> > > DEBUG(SSSDBG_TRACE_FUNC, "krb5_child completed
successfully\n");
>> >> > > talloc_free(tmp_str);
>> >> > > rv = 0;
>> >> > > } else {
>> >> > > DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child
failed!\n");
>> >> > > talloc_free(tmp_str);
>> >> > > rv = -1;
>> >> > > }
>> >> > > exit(rv);
>> >> >
>> >> > And now, you can compare your solution with the first patch from
this thread
>> >> > and then try to read replies to the 1st mail from this thread :-)
>> >>
>> >> Except first patch didn't allocate on NULL? I think that was the
meat of
>> >> Sumit's comments..
>> >
>> >yes, just allocate on NULL and free it explicitly in the end to make
>> >valgrind happy. The reason for suggesting NULL is that debug_prg_name is
>> >global and I think putting it under a specific talloc-hierarchy does not
>> >make sense.
>> >
>> but there is no difference. You allocate debug_prg_name on NULL and then steal
>> to another talloc context. (like in ldap_child)
>
>Why do you have to steal it? Just allocate on NULL and free it in the
>end.
>
>bye,
>Sumit
>
Attached patch use the same approach as in ldap_child.
To bring this thread to an end, I think it is ok for the time being to
do the same as ldap_child and I'll think the patch is good code-wise.
I'll run some test and will then give the final ACK.
bye,
Sumit
LS