[SSSD] [PATCHES] Do not call sss_cmd_done in function check_cache

Jakub Hrozek jhrozek at redhat.com
Thu Aug 8 20:07:58 UTC 2013


On Thu, Aug 08, 2013 at 06:32:11PM +0200, Lukas Slebodnik wrote:
> On (08/08/13 17:07), Jakub Hrozek wrote:
> >On Thu, Jun 27, 2013 at 08:57:36PM +0200, Jakub Hrozek wrote:
> >> On Thu, Jun 27, 2013 at 08:00:02PM +0200, Jakub Hrozek wrote:
> >> > On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
> >> > > On (25/06/13 15:16), Jakub Hrozek wrote:
> >> > > >On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
> >> > > >> ehlo,
> >> > > >> 
> >> > > >> Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
> >> > > >> 
> >> > > >> The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more
> >> > > >> result than 1, sssd will return error. sysdb_getpwnam has already had
> >> > > >> this check.
> >> > > >> 
> >> > > >> The second patch removes function call sss_cmd_done inside of check_cache,
> >> > > >> because function is sss_cmd_done is called in parent functions.
> >> > > >> This was a reason of sssd crash.
> >> > > >> 
> >> > > >> How to reproduce this crash.
> >> > > >> 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb
> >> > > >>   This netgroup should have the same attribute (name or nameAlias or memberOf)
> >> > > >>   as another netgroup.
> >> > > >> 2. call sudo with user, which is member of ^^^ netgroup.
> >> > > >> 
> >> > > >> Those patches fix only sssd crash, but we should find out:
> >> > > >>   Why were those netgroups stored in sysdb.
> >> > > >> 
> >> > > >> LS
> >> > > >
> >> > > >> From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001
> >> > > >> From: Lukas Slebodnik <lslebodn at redhat.com>
> >> > > >> Date: Tue, 25 Jun 2013 09:01:45 +0200
> >> > > >> Subject: [PATCH 1/2] Handle too many results from getnetgr.
> >> > > >> 
> >> > > >> ---
> >> > > >>  src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++-
> >> > > >>  1 file changed, 13 insertions(+), 1 deletion(-)
> >> > > >> 
> >> > > >> diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
> >> > > >> index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644
> >> > > >> --- a/src/responder/nss/nsssrv_netgroup.c
> >> > > >> +++ b/src/responder/nss/nsssrv_netgroup.c
> >> > > >> @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req)
> >> > > >>          }
> >> > > >>  
> >> > > >>          ret = lookup_netgr_step(step_ctx);
> >> > > >> -        if (ret != EOK) {
> >> > > >> +        switch (ret) {
> >> > > >> +        case EOK:
> >> > > >> +            break;
> >> > > >> +        case EMSGSIZE:
> >> > > >> +            state->netgr->ready = true;
> >> > > >
> >> > > >Do we need to set the entry to hash? Can't we just shortcut to
> >> > > >tevent_req_error ?
> >> > > >
> >> > > Here is pseudocode:
> >> > >     ret = get_netgroup_entry // get getgroup from hash
> >> > >     if (ret == EOK) {
> >> > >         /* snip */
> >> > >     } else if (ret == ENOENT) {
> >> > >         // netgr is stored in hash
> >> > >         /* snip */
> >> > >         ret = lookup_netgr_step(step_ctx);
> >> > >         // my changes
> >> > >     }
> >> > > 
> >> > > At first, lookup_netgr_step will return EMSGSIZE and will jump to label done
> >> > > with ret == EMSGSIZE.
> >> > > Second time, get_netgroup_entry will found netgroup in hash,
> >> > > netgroup will have both flags (found, ready) set to false.
> >> > > Flag ready will be false.
> >> > >        /* Result object is still being constructed
> >> > >         * Register for notification when it's ready
> >> > >         */
> >> > > And function will jump to done label with ret == EOK.
> >> > > There will not be any notification after object construction,
> >> > > because netgroup is created and therefore sss_cmd_done will not be called.
> >> > > SSSD will wain until expiration timeout.
> >> > > 
> >> > > With my patch, Flag ready will be true and tevent_req_error(req, ENOENT)
> >> > > will be called.
> >> > 
> >> > Yes, I understood that, I was just thinking if we can just fail without
> >> > setting the entry in the hash table.
> >> > 
> >> > But this patch gets rid of the segfault and is correct. We can think
> >> > about setting the entry or not later.
> >> > 
> >> > Ack.
> >> 
> >> Pushed to master.
> >
> >Also pushed to sssd-1-9
> 
> Only one patch was pushed to sssd-1-9
>     6e2c5a81b6af083d7909a18881971b5d907d65b1
>     Do not call sss_cmd_done in function check_cache.
> This patch fixes crash, but uninitialized netgroup will be stored in hash
> table. Another query for this netgroup will find uninitialized netgroup
> in hash table, but this netgroup will never be initialized (because of previous
> error EMSGSIZE) and therefore sss_cmd_done will not be called.
> 
> The second patch should be backported too.
>     f2022f7ba55973ae8b8baf2d4307322a180357b9
>     Handle too many results from getnetgr.
> With this patch, uninitialized netgroup will be removed from hash table
> in case of error EMSGSIZE.
> 
> It is my failure, because I was supposed to add text
> "resolves: https://fedorahosted.org/sssd/ticket/1980"
> to the second patch.
> 
> LS

Well, I should also check the full threads before just applying a
patch..

f2022f7ba55973ae8b8baf2d4307322a180357b9 applies cleanly, I'll push it
to sssd-1-9



More information about the sssd-devel mailing list