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

Lukas Slebodnik lslebodn at redhat.com
Thu Aug 8 16:32:11 UTC 2013


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



More information about the sssd-devel mailing list