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

Denys Vlasenko vda.linux at googlemail.com
Sat Nov 6 21:16:42 UTC 2010


On Thu, Oct 21, 2010 at 3:03 PM, Tito <farmatito at tiscali.it> wrote:
>> > Can at least
>> >                         if (geteuid())
>> >                                 bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);
>> > be moved out of this switch statement from hell,
>> > to make it a bit easier to grok?
>>
>> No, it cannot be moved outside because it must be triggered when
>> doing deluser or delgroup (argc = 2)
>> and when doing deluser from group (argc = 3)
>> if uid != 0

There is no execution path which skips geteuid() check.
Therefore, you may just move check up, before switch.

I did ask for more comments, and now you are overdoing it:

                        /* Check if UID is 0 */
                        if (geteuid())

bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);

This can be expressed without comment:

                        if (geteuid() != 0)

bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);

!= 0 says "check that it is not 0" better than comment;
and it can't get stale, unlike comments which sometimes do.


        /* Check number of command line args */
        switch (argc) {

Doh... No need to comment obvious things.


Please review attached edited version. The logic should be the same as yours.
I only flagged possible bugs in logic, and changed comments.

Applied to git, thanks.

-- 
vda
-------------- next part --------------
/* vi: set sw=4 ts=4: */
/*
 * deluser/delgroup implementation for busybox
 *
 * Copyright (C) 1999 by Lineo, inc. and John Beppu
 * Copyright (C) 1999,2000,2001 by John Beppu <beppu at codepoet.org>
 * Copyright (C) 2007 by Tito Ragusa <farmatito at tiscali.it>
 *
 * Licensed under GPL version 2, see file LICENSE in this tarball for details.
 *
 */
#include "libbb.h"

int deluser_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int deluser_main(int argc, char **argv)
{
	/* User or group name */
	char *name;
	/* Username (non-NULL only in "delgroup USER GROUP" case) */
	char *member;
	/* Name of passwd or group file */
	const char *pfile;
	/* Name of shadow or gshadow file */
	const char *sfile;
	/* Are we deluser or delgroup? */
	bool do_deluser = (ENABLE_DELUSER && (!ENABLE_DELGROUP || applet_name[3] == 'u'));

	if (geteuid() != 0)
		bb_error_msg_and_die(bb_msg_perm_denied_are_you_root);

	name = argv[1];
	member = NULL;

	switch (argc) {
	case 3:
		if (!ENABLE_FEATURE_DEL_USER_FROM_GROUP || do_deluser)
			break;
		/* It's "delgroup USER GROUP" */
		member = name;
		name = argv[2];
		/* Fallthrough */

	case 2:
		if (do_deluser) {
			/* "deluser USER" */
			xgetpwnam(name); /* bail out if USER is wrong */
			pfile = bb_path_passwd_file;
			if (ENABLE_FEATURE_SHADOWPASSWDS)
				sfile = bb_path_shadow_file;
		} else {
 do_delgroup:
			/* "delgroup GROUP" or "delgroup USER GROUP" */
			xgetgrnam(name); /* bail out if GROUP is wrong */
			if (!member) {
				/* "delgroup GROUP".
				 * If user with tha same name exists,
				 * bail out.
				 */
//BUG: check should be done by GID, not by matching name!
//1. find GROUP's GID
//2. check that /etc/passwd doesn't have lines of the form
//   user:pwd:uid:GID:...
//3. bail out if at least one such line exists
				if (getpwnam(name) != NULL)
					bb_error_msg_and_die("'%s' still has '%s' as their primary group!", name, name);
			}
			pfile = bb_path_group_file;
			if (ENABLE_FEATURE_SHADOWPASSWDS)
				sfile = bb_path_gshadow_file;
		}

		/* Modify pfile, then sfile */
		do {
			if (update_passwd(pfile, name, NULL, member) == -1)
				return EXIT_FAILURE;
			if (ENABLE_FEATURE_SHADOWPASSWDS) {
				pfile = sfile;
				sfile = NULL;
			}
		} while (ENABLE_FEATURE_SHADOWPASSWDS && pfile);

		if (ENABLE_DELGROUP && do_deluser) {
			/* "deluser USER" also should try to delete
			 * same-named group. IOW: do "delgroup USER"
			 */
//TODO: check how it actually works in upstream.
//I suspect it is only done if group has no more members.
			do_deluser = 0;
			goto do_delgroup;
		}
		return EXIT_SUCCESS;
	}
	/* Reached only if number of command line args is wrong */
	bb_show_usage();
}


More information about the busybox mailing list