On Wed, Feb 18, 2015 at 3:37 PM, Laszlo Papp <[email protected]> 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 <[email protected]>
> 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;
Kaboom! That is undefined behavior because line is not initialized and
then going to next would try to free that. Line needs to be
initialized to zero either at the beginning or after free. However, I
would probably just use continue instead of goto next to carry on
without the operation of free as there is really nothing to free in
this branch. The code did work on my desktop, but not on my arm
target. This is the nature of UB, I am afraid. Once I changed this
goto to continue, it worked everywhere.
Good luck with integrating this fix into busybox. I am not willing to
modify as Denys usually goes around and changes patches anyway, so
just leaving the important notes. ;-)
> + } 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;
> }
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
