[PATCH] 2nd attempt at deluser/delgroup size reduction and improvements

[Tito]

On Sunday 07 November 2010 13:52:58 you wrote:
> On Sun, Nov 7, 2010 at 10:25 AM, Tito <farmatito at tiscali.it> wrote:
> >> I am saying that "delgroup foo" (delgroup, not deluser)
> >> should not check whether *user named foo* exists;
> >> but should check that deleting group foo doesn't
> >> leave users with "deleted" GIDs.
> >
> >> Example:
> >>
> >> /etc/passwd
> >> haldaemon:x:68:490:HAL daemon:/:/sbin/nologin
> >> foo:x:496:1234::/:/sbin/nologin
> >>
> >> /etc/group
> >> foo:490:
> >>
> >> What "standard" delgroup foo will do? I suspect it will
> >> complain that haldaemon user's primary GID is 490
> >> and therefore group foo can't be deleted.
> >
> > adduser prova
> > Adding user `prova' ...
> > Adding new group `prova' (1006) ...
> > Adding new user `prova' (1004) with group `prova' ...
> > adduser prova2 --ingroup prova
> > Adding user `prova2' ...
> > Adding new user `prova2' (1005) with group `prova' ...
> 
> And what grep prova /etc/passwd /etc/group shows
> after these?
> 
> > Test case 1: Removing user prova:
> >
> > deluser prova
> 
> Awww, my brainzzzz... Why do you delete *a user*?
> We are trying to determine what is the correct behavior
> of *delgroup*, right?

Yes, we do but we have to take into account 
all corner cases.

> > Removing user `prova' ...
> > Warning: group `prova' has no more members.
> 
> Seems like an erroneous message. Group prova
> should still have at least prova2 user.

No, it is referring to members of group prova 
like prova:1006:pippo,pluto
primary groups will not show up here
but only in /etc/passwd as
user:uid:GID:...... 

> 
> Looks like code only checks /etc/group line
> whether it contains additional usernames:
> prova:x:1006:<any names here?>

No. It walks /etc/passwd to see
if there is any user with GID=1006
as it is possible to delete
a group even if it has members
(like prova:1006:pippo,pluto)
and real delgroup will not complain
(see later)

> but not /etc/passwd for users with GID=1006.
> I'd say this is a bug.
> 
> > userdel: Cannot remove group prova which is a primary group for another user.
> 
> Right, this means that prova2 line looks like this:
> prova2:x:1005:1006:...........

Yes
.
> > Done.
> > grep prova /etc/group
> > prova:x:1006:

Infact group prova was not deleted as referenced
in /etc/passwd by user prova2 (gid=1006)
> >
> > Test case 2: Removing user prova2:
> >
> > deluser prova2
> > Removing user `prova2' ...
> > Warning: group `prova' has no more members.
> > Done.
> 
> Attention. See _which_ group it deleted here?

None. as there is no group with name prova2.

> It did not try to delete a group with the same name as user,
> it looked at GID (2006), found which group it is (prova),
> and then deleted it.

No, it just warned that group is empty as it always does (in the sense of no members).

I'm aware now that the name matters
only when trying to delgroup an UPG
when doing deluser, in this case
first check if there is a group with same name
as the user and if there is one
check if it is used only by the user
(walking /etc/passwd)

> I was right in the second bug comment, we don't do
> this correctly too.
> 
> > grep prova /etc/group
> > prova:x:1006:
> 
> So you are saying group still exists?

Yes.

> > Test case 3: Removing group prova:
> >
> > delgroup prova
> > /usr/sbin/delgroup: `prova' still has `prova' as their primary group!
> 
> Wait a sec. User prova should already be deleted, no?
> I'm confused...

Test cases 1,2,3,4 all use the same setup:
user prova with group prova
user prova2 with group prova
 
> > Test case 4: Removing group prova after removal of user prova:
> >
> > deluser prova
> > Removing user `prova' ...
> > Warning: group `prova' has no more members.
> > userdel: Cannot remove group prova which is a primary group for another user.
> > Done.

now we have  user prova2 with group prova

> > delgroup prova
> > /usr/sbin/delgroup: `prova2' still has `prova' as their primary group!
> 
> Yes, now we see that they don't have our BUG #1.
> They correctly check passwd.

Yes, more or less.
 
> 
> 
> >> If you remove haldaemon line from /etc/passwd,
> >> delgroup foo will succeed despite the fact that _user_ foo exists.
> >> Because in this example, _user_ foo and _group_ foo
> >> are completely unrelated.
> >>
> >> Our version gets this wrong, I think.
> >
> > In some cases of course it does. To fix this
> > will be rather expensive in size:
> >
> > if (doing_deluser) {
> >        get gid
> >        delete_user
> >        walk through passwd file
> 
> No, why would you want the passwd file?
> it seems they just try "delgroup <group_with_gid_of_deleted_user>"
> 
> >        if users with same gid
> >                complain and do nothing to group file
> 
> This will be done as part of "delgroup <group_with_gid_of_deleted_user>"
> operation.
> 
> >        else
> >                if username == groupname // is UserPrivateGroup
> >                        delete_group
> 
> Where is the evidence it works like that, *by name*?
> 
> > if (doing_delgroup) {
> >        get_gid
> >        // NO - check if group is empty
> >        walk through passwd file
> >        if users with same gid
> >                complain and do nothing to group file
> > }
> 
> This is correct.
> 
> 
> > case argc2
> >        if (deluser) {
> >                setup deluser
> >                gid = get_gid
> >        } else {
> > do_delgroup:
> >        /* delgroup */
> >                setup delgroup
> >                if (no_gid)
> >                        gid = get_gid
> >                // NO - check if group is empty
> >                walk through passwd file
> >                if users with same gid > 0
> >                        complain and do nothing to group file
> >                else if (doing_delgroup after deluser /* needs a flag ? */) {
> >                        if username != groupname // not UserPrivateGroup
> >                                do_nothing to group file
> >                }
> >        }
> >
> >        do_deletions_loop
> >
> >        if (deluser ) {
> >                        set_a_flag
> >                        goto do_delgroup
> >        }
> 
> Not entirely correct, see comments above.

So far what I'm sure about is:

1) delgroup can delete groups with members without erroring out (agroup:100:member1,member2)
2) delgroup checks if gid of group is referenced in /etc/passwd   as primary group of an user and errors out if gid is found
3) deluser tries to delgroup the UPG:
	a) group with same name as user is found: do sanity checks and then remove
	b) group with same name as user is not found: do nothing and exit succes.

Attached you find test version of deluser.c that should now do it the right
way even if not displaying the same messages as the real version.
I've tested it a little but it needs more cleanup and size reduction.
Please take a look at it and help me to find the obvious bugs
I'm not able to see right now.

Ciao,
Tito
-------------- next part --------------
A non-text attachment was scrubbed...
Name: deluser.c
Type: text/x-csrc
Size: 2953 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20101107/8561c7a7/attachment.c>