[SSSD] [PATCH] LDAP: sdap_auth_send() properly initialize ppolicy

Jakub Hrozek jhrozek at redhat.com
Fri Jun 27 08:22:23 UTC 2014


On Thu, Jun 26, 2014 at 02:23:48PM +0200, Sumit Bose wrote:
> On Thu, Jun 26, 2014 at 01:39:24PM +0200, Lukas Slebodnik wrote:
> > On (26/06/14 13:00), Sumit Bose wrote:
> > >Hi,
> > >
> > >I found this while debugging timeout issues during authentication.
> > >
> > >bye,
> > >Sumit
> > 
> > >From 03879293f434cf2aaa6db9ead75830ba4a7050a7 Mon Sep 17 00:00:00 2001
> > >From: Sumit Bose <sbose at redhat.com>
> > >Date: Thu, 26 Jun 2014 11:59:55 +0200
> > >Subject: [PATCH] LDAP: sdap_auth_send() properly initialize ppolicy
> > >
> > >In case of an error ppolicy might not be set to NULL and
> > >auth_bind_user_done() might show some unexpected debug messages.
> > >---
> > > src/providers/ldap/sdap_async_connection.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > >diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> > >index a1f78c0..7407ae7 100644
> > >--- a/src/providers/ldap/sdap_async_connection.c
> > >+++ b/src/providers/ldap/sdap_async_connection.c
> > >@@ -1282,6 +1282,8 @@ struct tevent_req *sdap_auth_send(TALLOC_CTX *memctx,
> > >     req = tevent_req_create(memctx, &state, struct sdap_auth_state);
> > >     if (!req) return NULL;
> > > 
> > >+    state->ppolicy = NULL;
> > >+
> > >     if (sasl_mech) {
> > >         state->is_sasl = true;
> > >         subreq = sasl_bind_send(state, ev, sh, sasl_mech, sasl_user, NULL);
> > This would solve problem.
> > 
> > In my opinion, it would be better to solve in in function auth_bind_user_done
> > 
> > src/providers/ldap/ldap_auth.c:
> > errno_t sdap_auth_recv(struct tevent_req *req,
> >     //snip
> >    826         ret = sdap_auth_recv(subreq, state, &ppolicy)
> >    827         talloc_zfree(subreq);
> >    828         if (ppolicy != NULL) {
> >                    variable ret should be tested instead of ppolicy
> >    829             DEBUG(SSSDBG_TRACE_ALL,"Found ppolicy data, "
> > 
> > BTW function sdap_auth_recv looks supspicious as well.
> > 
> > src/providers/ldap/sdap_async_connection.c:
> > 1366 errno_t sdap_auth_recv(struct tevent_req *req,
> > 1367                        TALLOC_CTX *memctx,
> > 1368                        struct sdap_ppolicy_data **ppolicy)
> > 1369 {
> > 1370     struct sdap_auth_state *state = tevent_req_data(req,
> > 1371                                                  struct sdap_auth_state);
> > 1372
> > 1373     if (ppolicy != NULL) {
> > 1374         *ppolicy = talloc_steal(memctx, state->ppolicy);
> > 1375     }
> > 1376
> > 1377     TEVENT_REQ_RETURN_ON_ERROR(req);
> >          this macro used to be called before talloc_steal in other recv
> >          functions.
> > 
> > 1378
> > 1379     return EOK;
> > 1380 }
> > 
> 
> Thank you, these are good points. Nevertheless I think it is a good idea
> and common practice to initialize the members of a state ...
> 
> > It is possible I overlooked something.
> 
> ... to make other usage not fail.
> 
> New version attached.
> 
> bye,
> Sumit
> > 
> > LS
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

> From 52939ac7fe32e52dac9368f8bb1f7f1a337fdc87 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Thu, 26 Jun 2014 11:59:55 +0200
> Subject: [PATCH] LDAP: sdap_auth_send() properly initialize ppolicy
> 
> In case of an error ppolicy might not be set to NULL and
> auth_bind_user_done() might show some unexpected debug messages.
> 
> Additionally this patch fixes the usage of ppolicy in case of an error.
> ---
>  src/providers/ldap/ldap_auth.c             | 12 ++++++------
>  src/providers/ldap/sdap_async_connection.c |  6 ++++--
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
> index 40f297c..20fa23a 100644
> --- a/src/providers/ldap/ldap_auth.c
> +++ b/src/providers/ldap/ldap_auth.c
> @@ -825,14 +825,14 @@ static void auth_bind_user_done(struct tevent_req *subreq)
>  
>      ret = sdap_auth_recv(subreq, state, &ppolicy);
>      talloc_zfree(subreq);
> -    if (ppolicy != NULL) {
> -        DEBUG(SSSDBG_TRACE_ALL,"Found ppolicy data, "
> -                 "assuming LDAP password policies are active.\n");
> -        state->pw_expire_type = PWEXPIRE_LDAP_PASSWORD_POLICY;
> -        state->pw_expire_data = ppolicy;
> -    }
>      switch (ret) {
>      case EOK:
> +        if (ppolicy != NULL) {
> +            DEBUG(SSSDBG_TRACE_ALL,"Found ppolicy data, "
> +                     "assuming LDAP password policies are active.\n");
> +            state->pw_expire_type = PWEXPIRE_LDAP_PASSWORD_POLICY;
> +            state->pw_expire_data = ppolicy;
> +        }
>          break;
>      case ETIMEDOUT:
>      case ERR_NETWORK_IO:
> diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> index a1f78c0..ade9879 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -1282,6 +1282,8 @@ struct tevent_req *sdap_auth_send(TALLOC_CTX *memctx,
>      req = tevent_req_create(memctx, &state, struct sdap_auth_state);
>      if (!req) return NULL;
>  
> +    state->ppolicy = NULL;
> +

I wonder if the problem you saw was really caused by this
initialization? The way I read tevent_req_create code, the memory should
already be set to NULL:

80         data = talloc_zero_size(req, data_size);
81         if (data == NULL) {
82                 talloc_free(req);
83                 return NULL;
84         }
85         talloc_set_name_const(data, type);
86 

Please note I'm not against this patch, especially the
TEVENT_REQ_RETURN_ON_ERROR is correct, I just wonder if we're maybe not
seeing the right cause..



More information about the sssd-devel mailing list