Bug in the deluser applet

Laszlo Papp lpapp at kde.org
Wed Feb 18 16:16:56 UTC 2015


On Wed, Feb 18, 2015 at 3:54 PM, tito <farmatito at tiscali.it> wrote:
> On Wednesday 18 February 2015 16:37:59 you wrote:
>
>> > Actually, the locking and swapping logic would be common anyway which
>
>> > already resides in update_passwd, so if that reusable component is not
>
>> > moved to a separate function to be reused, we could use put our
>
>> > conditional magic into the while loop. I am preparing with that change
>
>> > now.
>
>>
>
>> Please check the patch below. I tested it and it works for me. The
>
>> coding style may not suit the existing busybox coding style, but I do
>
>> not mind that for now as I will use the patch as is for now in my
>
>> local fork anyhow. Any (non-stylistic) feedback is welcome.
>
>>
>
>> commit 22a7da560af82c33ca9334039522e9a03aab29b3
>
>> Author: Laszlo Papp <laszlo.papp at polatis.com>
>
>> Date: Wed Feb 18 15:20:58 2015 +0000
>
>>
>
>> Delete the user from all the groups for user deletion
>
>>
>
>> diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
>
>> index a30af6f..42998a9 100644
>
>> --- a/libbb/update_passwd.c
>
>> +++ b/libbb/update_passwd.c
>
>> @@ -62,6 +62,8 @@ static void check_selinux_update_passwd(const char
>> *username)
>
>> only if CONFIG_PASSWD=y and applet_name[0] == 'p' like in passwd
>
>> or if CONFIG_CHPASSWD=y and applet_name[0] == 'c' like in chpasswd
>
>>
>
>> + 8) delete a user from all groups : update_passwd(FILE, NULL, NULL,
>> MEMBER)
>
>> +
>
>> This function does not validate the arguments fed to it
>
>> so the calling program should take care of that.
>
>>
>
>> @@ -81,7 +83,7 @@ int FAST_FUNC update_passwd(const char *filename,
>
>> FILE *new_fp;
>
>> char *fnamesfx;
>
>> char *sfx_char;
>
>> - char *name_colon;
>
>> + char *name_colon = 0;
>
>> unsigned user_len;
>
>> int old_fd;
>
>> int new_fd;
>
>> @@ -99,13 +101,15 @@ int FAST_FUNC update_passwd(const char *filename,
>
>> if (filename == NULL)
>
>> return ret;
>
>>
>
>> - check_selinux_update_passwd(name);
>
>> + if (name) check_selinux_update_passwd(name);
>
>>
>
>> /* New passwd file, "/etc/passwd+" for now */
>
>> fnamesfx = xasprintf("%s+", filename);
>
>> sfx_char = &fnamesfx[strlen(fnamesfx)-1];
>
>> - name_colon = xasprintf("%s:", name);
>
>> - user_len = strlen(name_colon);
>
>> + if (name) {
>
>> + name_colon = xasprintf("%s:", name);
>
>> + user_len = strlen(name_colon);
>
>> + }
>
>>
>
>> if (shadow)
>
>> old_fp = fopen(filename, "r+");
>
>> @@ -162,21 +166,37 @@ int FAST_FUNC update_passwd(const char *filename,
>
>> /* Read current password file, write updated /etc/passwd+ */
>
>> changed_lines = 0;
>
>> while (1) {
>
>> - char *cp, *line;
>
>> -
>
>> - line = xmalloc_fgetline(old_fp);
>
>> - if (!line) /* EOF/error */
>
>> - break;
>
>> - if (strncmp(name_colon, line, user_len) != 0) {
>
>> - fprintf(new_fp, "%s\n", line);
>
>> - goto next;
>
>> - }
>
>> -
>
>> - /* We have a match with "name:"... */
>
>> - cp = line + user_len; /* move past name: */
>
>> + char *cp, *line;
>
>> + if (!name && member) {
>
>> + struct group* g;
>
>> + if ((g = getgrent())) {
>
>> + char gline[LINE_MAX];
>
>> + char **s= g->gr_mem;
>
>> + snprintf(gline, sizeof(gline), "%s:%s:%i:",
>
>> g->gr_name, g->gr_passwd, g->gr_gid);
>
>> + while (*s) {
>
>> + if (strcmp(*s, member)) { if (s != g->gr_mem)
>
>> strcat(gline, ","); strcat(gline, *s); }
>
>> + ++s;
>
>> + }
>
>> + fprintf(new_fp, "%s\n", gline);
>
>> + goto next;
>
>> + } else {
>
>> + break;
>
>> + }
>
>> + } else {
>
>> + line = xmalloc_fgetline(old_fp);
>
>> + if (!line) /* EOF/error */
>
>> + break;
>
>> + if (!name_colon || strncmp(name_colon, line, user_len) != 0) {
>
>> + fprintf(new_fp, "%s\n", line);
>
>> + goto next;
>
>> + }
>
>> +
>
>> + /* We have a match with "name:"... */
>
>> + cp = line + user_len; /* move past name: */
>
>> + }
>
>>
>
>> #if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
>
>> - if (member) {
>
>> + if (name && member) {
>
>> /* It's actually /etc/group+, not /etc/passwd+ */
>
>> if (ENABLE_FEATURE_ADDUSER_TO_GROUP
>
>> && applet_name[0] == 'a'
>
>> @@ -240,7 +260,7 @@ int FAST_FUNC update_passwd(const char *filename,
>
>>
>
>> if (changed_lines == 0) {
>
>> #if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
>
>> - if (member) {
>
>> + if (name && member) {
>
>> if (ENABLE_ADDGROUP && applet_name[0] == 'a')
>
>> bb_error_msg("can't find %s in %s",
>
>> name, filename);
>
>> if (ENABLE_DELGROUP && applet_name[0] == 'd')
>
>> diff --git a/loginutils/deluser.c b/loginutils/deluser.c
>
>> index 01a9386..a3f5f3a 100644
>
>> --- a/loginutils/deluser.c
>
>> +++ b/loginutils/deluser.c
>
>> @@ -82,6 +82,9 @@ int deluser_main(int argc, char **argv)
>
>> do_delgroup:
>
>> /* "delgroup GROUP" or "delgroup USER GROUP" */
>
>> if (do_deluser < 0) { /* delgroup after deluser? */
>
>> + pfile = bb_path_group_file;
>
>> + if (update_passwd(pfile, NULL, NULL, name) == -1)
>
>> + return EXIT_FAILURE;
>
>> gr = getgrnam(name);
>
>> if (!gr)
>
>> return EXIT_SUCCESS;
>
>> @@ -99,7 +102,6 @@ int deluser_main(int argc, char **argv)
>
>> }
>
>> //endpwent();
>
>> }
>
>> - pfile = bb_path_group_file;
>
>> if (ENABLE_FEATURE_SHADOWPASSWDS)
>
>> sfile = bb_path_gshadow_file;
>
>> }
>
>>
>
>
>
> Hi,
>
> just for fun a different approach, this seems to work
>
> without touching update_passwd.

The real question is: why would you not touch that? All the locking
and swapping are done in there properly and that is the fastest way to
do it because you avoid the needless embedded (!) iterations. It is
not convoluted code either, so I see no reason not to apply the fast
code.

> Probably it could be
>
> shrinked and refactored even more. Doesn't work
>
> with bb's internal pwdgrp as it triggers some bug,
>
> will post in a separate thread about that.
>
>
>
> Ciao,
>
> Tito
>
>
>
> --- loginutils/deluser.c.original 2015-02-18 15:12:10.810954596 +0100
>
> +++ loginutils/deluser.c 2015-02-18 16:47:34.186322005 +0100
>
> @@ -37,7 +37,8 @@
>
> const char *sfile;
>
> /* Are we deluser or delgroup? */
>
> int do_deluser = (ENABLE_DELUSER && (!ENABLE_DELGROUP || applet_name[3] ==
> 'u'));
>
> -
>
> + struct group *gr;
>
> + struct passwd *pw;
>
> #if !ENABLE_LONG_OPTS
>
> const int opt_delhome = 0;
>
> #else
>
> @@ -69,8 +70,6 @@
>
> case 2:
>
> if (do_deluser) {
>
> /* "deluser USER" */
>
> - struct passwd *pw;
>
> -
>
> pw = xgetpwnam(name); /* bail out if USER is wrong */
>
> pfile = bb_path_passwd_file;
>
> if (ENABLE_FEATURE_SHADOWPASSWDS)
>
> @@ -78,7 +77,6 @@
>
> if (opt_delhome)
>
> remove_file(pw->pw_dir, FILEUTILS_RECUR);
>
> } else {
>
> - struct group *gr;
>
> do_delgroup:
>
> /* "delgroup GROUP" or "delgroup USER GROUP" */
>
> if (do_deluser < 0) { /* delgroup after deluser? */
>
> @@ -90,7 +88,6 @@
>
> }
>
> if (!member) {
>
> /* "delgroup GROUP" */
>
> - struct passwd *pw;
>
> /* Check if the group is in use */
>
> while ((pw = getpwent()) != NULL) {
>
> if (pw->pw_gid == gr->gr_gid)
>
> @@ -116,6 +113,26 @@
>
> if (ENABLE_DELGROUP && do_deluser > 0) {
>
> /* "deluser USER" also should try to delete
>
> + * the USER from all groups in which he is member.
>
> + */
>
> + gid_t *groups = NULL;
>
> + int n = 64;
>
> +retry:
>
> + groups = xrealloc(groups, n * sizeof(gid_t));
>
> + if(getgrouplist(name, pw->pw_gid, groups, &n) == -1)
>
> + goto retry;
>
> +
>
> + while (--n >= 1) { /* our gid is the last group */
>
> + gr = getgrgid(groups[n]);
>
> + if (!gr)
>
> + continue;
>
> + update_passwd(bb_path_group_file, gr->gr_name, NULL, name);
>
> + if (ENABLE_FEATURE_SHADOWPASSWDS)
>
> + update_passwd(bb_path_gshadow_file, gr->gr_name, NULL, name);
>
> + }
>
> + if (ENABLE_FEATURE_CLEAN_UP)
>
> + free(groups);
>
> + /* "deluser USER" also should try to delete
>
> * same-named group. IOW: do "delgroup USER"
>
> */
>
> // On debian deluser is a perl script that calls userdel.
>
>
>
>
> _______________________________________________
> busybox mailing list
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox


More information about the busybox mailing list