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(a)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.