[SSSD] [PATCH] Explicitly ignore groups with gidNumber=0

Jan Zeleny jzeleny at redhat.com
Fri Jul 22 09:53:50 UTC 2011


Jakub Hrozek <jhrozek at redhat.com> wrote:
> On 07/22/2011 09:18 AM, Jan Zeleny wrote:
> > Jakub Hrozek<jhrozek at redhat.com>  wrote:
> >> On 07/21/2011 01:57 PM, Jan Zelený wrote:
> >>>> https://fedorahosted.org/sssd/ticket/916
> >>> 
> >>> Nack,
> >>> please look at following lines, there are parts of code which might
> >>> need update along with your changes:
> >>> 
> >>> providers/ldap/sdap_async_accounts.cz : 728
> >>> db/sysdb_ops.c : 1576
> >> 
> >> Right - the problem would be if a group changed its gid from nonzero to
> >> zero, the sysdb update would keep the original version. It's a corner
> >> case but we'd better cover it. The problem is there even with non-posix
> >> groups so I'm sending a separate fix.
> >> 
> >> Two patches attached now.
> > 
> > Nack,
> > I think you tried to fix the issue we discussed yesterday at the wrong
> > place. The place your patch 0001 is taking care of only needs comsmetic
> > change (the condition is needlesly complicated. Something like if
> > (posix_group&& OUT_OF_ID_RANGE(...)) will do, since now gid == 0 always
> > implies posix_group == true.
> 
> Patch 0001 does not deal with gid=0 implying non-posix. That's what
> patch #2 does.

I know that and I'm not arguing about that. My thought is valid, but I made a 
mistake in explanation. The condition if (posix_group || gid != 0) should be 
if (posix_group) because posix_group == true always implies gid != 0.

Also, the "else" part of your patch is redundant, see how the function 
sdap_save_group handles the gid (especially what sysdb_store_group does).

> > The issue you are trying to fix should be fixed in sysdb_ops.c in the
> > place we were looking at yesterday.
> 
> No, that would be wrong abstraction. The sysdb function does not need to
> know about posix/non-posix.

I'm sorry, I didn't express myself clearly. I was referring to the error 
where, in some corner cases, the group can be marked as non-posix, but the GID 
number will be > 0 because it was not overwritten (see the condition on line 
1576). I didn't analyze the code deeply, but I believe this can happen 
sometimes. But as I said yesterday, this is completely different issue and 
therefore can (and should) be solved separately.

Jan



More information about the sssd-devel mailing list