[SSSD] [PATCH] LDAP: sdap_auth_send() properly initialize ppolicy
Sumit Bose
sbose at redhat.com
Fri Jun 27 09:02:26 UTC 2014
On Fri, Jun 27, 2014 at 10:22:23AM +0200, Jakub Hrozek wrote:
> 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..
Thank you for the hint. I always thought that the state is not
initialized because I remember a discussion long time ago that it is not
done on purpose for performance reasons. But it looks that libtevent does
initialize this since about 5 years ago.
I checked the usage of ppolicy again and so far found no reason why it
should be set in the case of a timeout. I will continue investigating.
bye,
Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list