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

Lukas Slebodnik lslebodn at redhat.com
Thu Jun 26 17:07:34 UTC 2014


On (26/06/14 14:23), 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 ...
Agree with initialisation.

>> It is possible I overlooked something.
>
>... to make other usage not fail.
>
>New version attached.
>
>bye,
>Sumit

>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

ACK

LS



More information about the sssd-devel mailing list