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

Sumit Bose sbose at redhat.com
Thu Jun 26 12:23:48 UTC 2014


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
-------------- next part --------------
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;
+
     if (sasl_mech) {
         state->is_sasl = true;
         subreq = sasl_bind_send(state, ev, sh, sasl_mech, sasl_user, NULL);
@@ -1370,12 +1372,12 @@ errno_t sdap_auth_recv(struct tevent_req *req,
     struct sdap_auth_state *state = tevent_req_data(req,
                                                  struct sdap_auth_state);
 
+    TEVENT_REQ_RETURN_ON_ERROR(req);
+
     if (ppolicy != NULL) {
         *ppolicy = talloc_steal(memctx, state->ppolicy);
     }
 
-    TEVENT_REQ_RETURN_ON_ERROR(req);
-
     return EOK;
 }
 
-- 
1.8.3.1



More information about the sssd-devel mailing list