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 :-)
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
