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)
The only reason why there is no use after free is that:
-- write debug message
-- remove resources (talloc_free, close)
-- call exit with return code.
DEBUG(SSSDBG_TRACE_FUNC, "ldap_child completed successfully\n");
close(STDOUT_FILENO);
talloc_free(main_ctx);
_exit(0);
fail:
DEBUG(SSSDBG_CRIT_FAILURE, "ldap_child failed!\n");
close(STDOUT_FILENO);
talloc_free(main_ctx);
_exit(-1);
in krb5_child it is in wrong order:
-- remove resources (talloc_free, krb5_cleanup)
-- write debug message
-- call exit with return code.
done:
krb5_cleanup(kr);
talloc_free(kr);
if (ret == EOK) {
DEBUG(SSSDBG_TRACE_FUNC, "krb5_child completed successfully\n");
exit(0);
} else {
DEBUG(SSSDBG_CRIT_FAILURE, "krb5_child failed!\n");
exit(-1);
}
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
