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 <lsleb...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
