[SSSD] [PATCH] LDAP: Fix leak of file descriptors

Jakub Hrozek jhrozek at redhat.com
Wed Nov 4 10:17:38 UTC 2015


On Thu, Oct 22, 2015 at 01:47:18PM +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> Details are in commit message. BTW it would be good to have at least two
> reviews.
> 
> Reproducer which I used for https://fedorahosted.org/sssd/ticket/2792:
> 
> Setup:
> * two active directories with sites; so sometimes sssd connect to server A
>   and sometims to server B.
> * block connection to one server
> 
> [root at host sssd]# iptables -n -L
> Chain INPUT (policy ACCEPT)
> target     prot opt source               destination         
> 
> Chain FORWARD (policy ACCEPT)
> target     prot opt source               destination         
> 
> Chain OUTPUT (policy ACCEPT)
> target     prot opt source               destination         
> DROP       tcp  --  0.0.0.0/0            10.12.0.158          tcp dpt:389
>   
> 
> * force sssd to go offline (-USR1) and online (-USR2)
> 
> You might be able to reproduce ti even with plan LDAP.
> It might help if set value of options that
>   dns_resolver_timeout < ldap_network_timeout
> 
> Do you have an idea how to test such fd leak?
>  (either using cmocka or integration tests)
> 
> LS

Thanks for finding the bug, this was a nasty one. One question inline..

> From 8cb857694f0a6f48b63eee50fe6b03dd3820c28e Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik <lslebodn at redhat.com>
> Date: Thu, 22 Oct 2015 10:30:12 +0200
> Subject: [PATCH] LDAP: Fix leak of file descriptors
> 
> The state "struct sss_ldap_init_state" contains socket
> created in function sss_ldap_init_send. We register callback
> sdap_async_sys_connect_timeout for handling issue with connection
> 
> The tevent request "sss_ldap_init_send" is usually (nested) subrequest
> of "struct resolve_service_state" related request created in fucntion
> fo_resolve_service_send. Function fo_resolve_service_send also register
> timeout callback fo_resolve_service_timeout to state "struct
> resolve_service_state".
> 
> It might happen that fo_resolve_service_timeout will be called before
> sss_ldap_init_send timeout and we could not handle tiemout error
> for state "struct sss_ldap_init_state" and therefore created socket
> was not closed.
> 
> We tried to release resources in function sdap_handle_release.
> But the structure "struct sdap_handle" had not been initialized yet
> with LDAP handle and therefore associated file descriptor could not be closed.
> 
> [fo_resolve_service_timeout] (0x0080): Service resolving timeout reached
> [fo_resolve_service_recv] (0x0020): TEVENT_REQ_RETURN_ON_ERROR ret[110]
> [sdap_handle_release] (0x2000): Trace: sh[0x7f6713410270], connected[0], ops[(nil)], ldap[(nil)], destructor_lock[0], release_memory
> [be_resolve_server_done] (0x1000): Server resolution failed: 14
> [be_resolve_server_recv] (0x0020): TEVENT_REQ_RETURN_ON_ERROR ret[14]
> [check_online_callback] (0x0100): Backend returned: (1, 0, <NULL>) [Provider is Offline (Success)]
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2792
> ---
>  src/util/sss_ldap.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c
> index d1bf0373f07280f4281f1e1393e2008be9b97e40..195afd508e42f8af887a88fdcff2bddb04e8cd3f 100644
> --- a/src/util/sss_ldap.c
> +++ b/src/util/sss_ldap.c
> @@ -304,6 +304,22 @@ struct sss_ldap_init_state {
>  #endif
>  };
>  
> +static int sss_ldap_init_state_destructor(void *data)
> +{
> +    struct sss_ldap_init_state *state = (struct sss_ldap_init_state *)data;
> +
> +    if (state->ldap) {
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              "calling ldap_unbind_ext for ldap:[%p] sd:[%d]\n",
> +              state->ldap, state->sd);
> +        ldap_unbind_ext(state->ldap, NULL, NULL);
> +    } else {

Do you think it would be safer to initialize state->sd to -1 after the
state is created and only close state->sd if it's > 1? I think if the
socket() call would fail, then state->sd was 0 (because tevent uses
talloc_zero to create the state) and then we might attempt to close
stdin..

> +        DEBUG(SSSDBG_TRACE_FUNC, "closing socket [%d]\n", state->sd);
> +        close(state->sd);
> +    }
> +
> +    return 0;
> +}


More information about the sssd-devel mailing list