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
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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel