[PATCH] adduser remove some code

Tito farmatito at tiscali.it
Sat Dec 21 14:50:31 UTC 2013


Hi,
while looking at the last commits to adduser I've spotted
some code that attracted my attention:

	opt_complementary = "-1:?2:SD:u+";
	if (sizeof(pw.pw_uid) == sizeof(int)) {
		opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &pw.pw_uid);
	} else {
		unsigned uid;
		opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &uid);
		if (opts & OPT_UID) {
			pw.pw_uid = uid;
		}
	}

I really was not able to understand the need for the double getopt32 call,
but maybe I'm just not smart enough.

Nonetheless:
1) u+ in opt_complementary means that the parameter
        for this option is a nonnegative integer. It will be processed
        with xatoi_positive() - allowed range is 0..INT_MAX.

2) even if sizeof(pw.pw_uid) is not sizeof(int) and we assume that it is bigger sizeof (unsigned)
    i think that our  0 to INT_MAX sized arg would always fit.

3) we use already the logic proposed by the fix in addgroup.

So I would simplify the code like:

	opt_complementary = "-1:?2:SD:u+";
	opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &pw.pw_uid);

As this seems to easy I supect I'm overlooking something obvious
and therefore hints, critics and improvements by the list members
are welcome. (Please have mercy!)

--- loginutils/adduser.c.orig   2013-12-21 12:52:52.000000000 +0100
+++ loginutils/adduser.c        2013-12-21 12:57:59.188565203 +0100
@@ -164,16 +164,11 @@ int adduser_main(int argc UNUSED_PARAM,
 
        /* at least one and at most two non-option args */
        /* disable interactive passwd for system accounts */
+       /* uid: the parameter for this option is a nonnegative integer,*/
+       /* allowed range is 0..INT_MAX.*/
        opt_complementary = "-1:?2:SD:u+";
-       if (sizeof(pw.pw_uid) == sizeof(int)) {
-               opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &pw.pw_uid);
-       } else {
-               unsigned uid;
-               opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &uid);
-               if (opts & OPT_UID) {
-                       pw.pw_uid = uid;
-               }
-       }
+       opts = getopt32(argv, "h:g:s:G:DSHu:", &pw.pw_dir, &pw.pw_gecos, &pw.pw_shell, &usegroup, &pw.pw_uid);
+
        argv += optind;
        pw.pw_name = argv[0];
 

I've spotted one more inconsistency in the function passwd_study(struct passwd *p)
where  in a comment we assume UID_T_MAX == INT_MAX
meanwhile in the code we do;
	int max = UINT_MAX;
  
This is fixed as:

 /* recoded such that the uid may be passed in *p */
 static void passwd_study(struct passwd *p)
 {
-       int max = UINT_MAX;
+       int max = INT_MAX;
 
        if (getpwnam(p->pw_name)) {
                bb_error_msg_and_die("%s '%s' in use", "user", p->pw_name);

Ciao,
Tito
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adduser.patch
Type: text/x-patch
Size: 1255 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20131221/39575f5e/attachment.bin>


More information about the busybox mailing list