[SSSD] [PATCH] krb5_child: Fix use after free

Sumit Bose sbose at redhat.com
Tue Apr 8 09:22:32 UTC 2014


On Mon, Apr 07, 2014 at 10:36:39PM +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:
> >> 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.
> >
> 1st:
>     Jakub suggested it in mail
>     https://lists.fedorahosted.org/pipermail/sssd-devel/2014-April/019002.html
> 2nd:
>     it is in ldap_child.

I'm sorry, I think I started to confusion by mentioning ldap_child.c but
not being explicit about which aspect I liked and which not.

I do not like the talloc_steal() in ldap_child.c as well. Imo it should
be possible to add a DEBUG statement at any place in the code after
DEBUG_INIT is called and the only safe way to do this is when
debug_prg_name is allocate on NULL and is not the child of any other
talloc context.

While thinking even more about it. If we would allocate debug_prg_name
in DEBUG_INIT we would be able the hide the knowledge about this
variable from the main code. In this case we would need to DEBUG_DONE or
DEBUG_SHUTDOWN to properly free the value as well.  But this would be a
different ticket if people like the idea.

bye,
Sumit

> 
> sh-4.2$ grep -B6 -n talloc_steal src/providers/ldap/ldap_child.c
> 478-    main_ctx = talloc_new(NULL);
> 479-    if (main_ctx == NULL) {
> 480-        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new failed.\n");
> 481-        talloc_free(discard_const(debug_prg_name));
> 482-        goto fail;
> 483-    }
> 484:    talloc_steal(main_ctx, debug_prg_name);
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list