[PATCH] sdap_get_generic_ext_send: check if we a re still connected

[Sumit Bose]

Hi,

this patch tries to fix https://fedorahosted.org/sssd/ticket/2126 . Se
commit message for details.

bye,
Sumit

[Sumit Bose]

On Tue, Oct 22, 2013 at 10:46:04AM +0200, Sumit Bose wrote:
> Hi,
>
> this patch tries to fix https://fedorahosted.org/sssd/ticket/2126 . Se
> commit message for details.

sorry, but the check in the first patch is too strict. When doing a
rootDSE lookup as the very first LDAP request sh->connected is still
false because we only set it to true after the first successful request.

New version attached.

bye,
Sumit
> bye,
> Sumit
> From 2e9b617c2b0e08f3f34ea3de44d5f47c758b7f9d Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Fri, 18 Oct 2013 15:54:22 +0200
> Subject: [PATCH] sdap_get_generic_ext_send: check if we a re still connected
>
> At the beginning of a LDAP request we check if we are connecte and have
> a valid sdap handle. But for some requests more than one LDAP operation,
> typically a search, is needed. Due to the asynchronous handling of LDAP
> request it might be possible that a second request might detect a server
> error and close the connection while the first request just finished one
> LDAP search and wants to start a new LDAP search.
>
> This patch tries to make sure that there is a valid sdap handle before
> sending a LDAP search to the server.
> Fixes https://fedorahosted.org/sssd/ticket/2126
>
> src/providers/ldap/sdap_async.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> index 09275dd..a9d63ea 100644
> --- a/src/providers/ldap/sdap_async.c
> +++ b/src/providers/ldap/sdap_async.c
> @@ -1167,6 +1167,13 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
>      state->cb_data = cb_data;
>      state->clientctrls = clientctrls;
>
> if (state->sh == NULL || !state->sh->connected || state->sh->ldap == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          ("Trying LDAP search while not connected.\n"));
>    tevent_req_error(req, EIO);
>    tevent_req_post(req, ev);
>    return req;
>
> }
>
> /* Be extra careful and never allow paging for BASE searches,
> even if requested.
>
> -- 
> 1.8.3.1
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Tue, Oct 22, 2013 at 11:36:16AM +0200, Sumit Bose wrote:
> On Tue, Oct 22, 2013 at 10:46:04AM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this patch tries to fix https://fedorahosted.org/sssd/ticket/2126 . Se
> > commit message for details.
>
> sorry, but the check in the first patch is too strict. When doing a
> rootDSE lookup as the very first LDAP request sh->connected is still
> false because we only set it to true after the first successful request.
>
> New version attached.
>
> bye,
> Sumit
> > bye,
> > Sumit
> > From 2e9b617c2b0e08f3f34ea3de44d5f47c758b7f9d Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Fri, 18 Oct 2013 15:54:22 +0200
> > Subject: [PATCH] sdap_get_generic_ext_send: check if we a re still connected
> >
> > At the beginning of a LDAP request we check if we are connecte and have
> > a valid sdap handle. But for some requests more than one LDAP operation,
> > typically a search, is needed. Due to the asynchronous handling of LDAP
> > request it might be possible that a second request might detect a server
> > error and close the connection while the first request just finished one
> > LDAP search and wants to start a new LDAP search.
> >
> > This patch tries to make sure that there is a valid sdap handle before
> > sending a LDAP search to the server.
> > Fixes https://fedorahosted.org/sssd/ticket/2126
> >
> > src/providers/ldap/sdap_async.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > index 09275dd..a9d63ea 100644
> > --- a/src/providers/ldap/sdap_async.c
> > +++ b/src/providers/ldap/sdap_async.c
> > @@ -1167,6 +1167,13 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> >      state->cb_data = cb_data;
> >      state->clientctrls = clientctrls;
> >
> > if (state->sh == NULL || !state->sh->connected || state->sh->ldap == NULL) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          ("Trying LDAP search while not connected.\n"));
> >    tevent_req_error(req, EIO);
> >    tevent_req_post(req, ev);
> >    return req;
> >
> > }
> >
> > /* Be extra careful and never allow paging for BASE searches,
> > even if requested.
> >
> > -- 
> > 1.8.3.1
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I couldn't reproduce the original error but all basic LDAP operations I
tried keep working fine.  The code looks good to me, too.

ACK

[Jakub Hrozek]

On Tue, Oct 22, 2013 at 06:53:37PM +0200, Jakub Hrozek wrote:
> On Tue, Oct 22, 2013 at 11:36:16AM +0200, Sumit Bose wrote:
> > On Tue, Oct 22, 2013 at 10:46:04AM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch tries to fix https://fedorahosted.org/sssd/ticket/2126 . Se
> > > commit message for details.
> >
> > sorry, but the check in the first patch is too strict. When doing a
> > rootDSE lookup as the very first LDAP request sh->connected is still
> > false because we only set it to true after the first successful request.
> >
> > New version attached.
> >
> > bye,
> > Sumit
> > > bye,
> > > Sumit
> > > From 2e9b617c2b0e08f3f34ea3de44d5f47c758b7f9d Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Fri, 18 Oct 2013 15:54:22 +0200
> > > Subject: [PATCH] sdap_get_generic_ext_send: check if we a re still connected
> > >
> > > At the beginning of a LDAP request we check if we are connecte and have
> > > a valid sdap handle. But for some requests more than one LDAP operation,
> > > typically a search, is needed. Due to the asynchronous handling of LDAP
> > > request it might be possible that a second request might detect a server
> > > error and close the connection while the first request just finished one
> > > LDAP search and wants to start a new LDAP search.
> > >
> > > This patch tries to make sure that there is a valid sdap handle before
> > > sending a LDAP search to the server.
> > > Fixes https://fedorahosted.org/sssd/ticket/2126
> > >
> > > src/providers/ldap/sdap_async.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
> > > index 09275dd..a9d63ea 100644
> > > --- a/src/providers/ldap/sdap_async.c
> > > +++ b/src/providers/ldap/sdap_async.c
> > > @@ -1167,6 +1167,13 @@ sdap_get_generic_ext_send(TALLOC_CTX *memctx,
> > >      state->cb_data = cb_data;
> > >      state->clientctrls = clientctrls;
> > >
> > > if (state->sh == NULL || !state->sh->connected || state->sh->ldap == NULL) {
> > >    DEBUG(SSSDBG_CRIT_FAILURE,
> > >          ("Trying LDAP search while not connected.\n"));
> > >    tevent_req_error(req, EIO);
> > >    tevent_req_post(req, ev);
> > >    return req;
> > >
> > > }
> > >
> > > /* Be extra careful and never allow paging for BASE searches,
> > > even if requested.
> > >
> > > -- 
> > > 1.8.3.1
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> I couldn't reproduce the original error but all basic LDAP operations I
> tried keep working fine.  The code looks good to me, too.
>
> ACK

Pushed to master, sssd-1-11 and sssd-1-9