Bug in the deluser applet
Laszlo Papp
lpapp at kde.org
Wed Feb 18 14:13:21 UTC 2015
On Wed, Feb 18, 2015 at 12:09 PM, Laszlo Papp <lpapp at kde.org> wrote:
> On Wed, Feb 18, 2015 at 11:00 AM, tito <farmatito at tiscali.it> wrote:
>> On Wednesday 18 February 2015 09:14:42 you wrote:
>>
>>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp <lpapp at kde.org> wrote:
>>
>>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp <lpapp at kde.org> wrote:
>>
>>> >> I thought about this, too, when writing the patch, but I rejected it
>>> >> because
>>
>>> >> it is not simpler, in fact more complex, it is also not the right layer
>>> >> for
>>
>>> >> the change. In addition, it would also be somewhat slower to execute.
>>
>>> >
>>
>>> > Just to elaborate a bit more on this as I was sending that on my phone:
>>
>>> >
>>
>>> > 1) Why do you want to make a few lines more simpler to spare a few
>>
>>> > lines to introduce bad implementation and slower operation?
>>
>>> >
>>
>>> > 2) Also, the update_passwd function already has all the structure to
>>
>>> > handle this through the file, name and member variables. It looks like
>>
>>> > the natural place to utilize the existing infrastructure.
>>
>>> >
>>
>>> > 3) Basically, you would go through the groups once in the embedded
>>
>>> > list and then you would go through again rather than just doing the
>>
>>> > thing correct in the first place at the first iteration. So basically,
>>
>>> > you would have double embedded list iteration. Even if the group list
>>
>>> > was stored in memory for _each_ user, it would still slightly be
>>
>>> > slower and I would argue that wasting memory for potentially a big
>>
>>> > file could defeat busybox's purpose in the first place. Anyway,
>>
>>> > busybox's design goal should be to be as fast as possible. Personal
>>
>>> > preferences should neither slow it down, nor make it use more memory
>>
>>> > than needed to achieve the task.
>>
>>>
>>
>>> I would actually even go further than that, namely: the same-named
>>
>>> group deletion should probably be integrated into my loop so that one
>>
>>> embedded iteration could deal with the group file changes rather than
>>
>>> two separate.
>>
>>>
>>
>>
>>
>> Hi,
>>
>>
>>
>> you can try to write a patch, but i suspect that it could be difficult
>>
>> with the current update_password interface without breaking the
>>
>> deluser from group use case.
>>
>> If you opt for doing the changes in deluser instead some useful
>>
>> get_groups code is in the id applet and could be moved to libbb as
>>
>> function. This will of course be slower as more iterations are done
>>
>> through the passwd/group files.
>
> I start to think that the best approach would be neither to use your
> idea, nor mine, but actually a separate function doing the thing fast.
> See my updated patch below which almost works, but it remains slow if
> I try to inject the new code into the "line based" read and write
> concept. Why don't we just create a new function?
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.
By the way, why does busybox use /etc/file+ and /etc/file- for this
logic as opposed to ... say: mkstemp?
> commit 813e3db073aca7a55b9f3d4650055e7d1ed768b4
> Author: Laszlo Papp <lpapp at kde.org>
> Date: Tue Feb 17 20:01:04 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..24f949b 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+");
> @@ -159,6 +163,19 @@ int FAST_FUNC update_passwd(const char *filename,
> bb_perror_msg("warning: can't lock '%s'", filename);
> lock.l_type = F_UNLCK;
>
> + if (!name && member) {
> + struct group* g;
> + while ((g = getgrent())) {
> + char **s= g->gr_mem;
> + char **d = s;
> + while (*s) {
> + if (strcmp(*s, member)) { *d = *s; ++d; }
> + ++s;
> + }
> + *d = 0;
> + }
> + }
> +
> /* Read current password file, write updated /etc/passwd+ */
> changed_lines = 0;
> while (1) {
> @@ -167,7 +184,7 @@ int FAST_FUNC update_passwd(const char *filename,
> line = xmalloc_fgetline(old_fp);
> if (!line) /* EOF/error */
> break;
> - if (strncmp(name_colon, line, user_len) != 0) {
> + if (!name_colon || strncmp(name_colon, line, user_len) != 0) {
> fprintf(new_fp, "%s\n", line);
> goto next;
> }
> @@ -176,7 +193,7 @@ int FAST_FUNC update_passwd(const char *filename,
> 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 +257,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..8219569 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;
> }
More information about the busybox
mailing list