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.