On Thu, Oct 21, 2010 at 3:03 PM, Tito <[email protected]> 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
/* 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 <[email protected]>
* Copyright (C) 2007 by Tito Ragusa <[email protected]>
*
* 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();
}
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
