Bug in the deluser applet
tito
farmatito at tiscali.it
Wed Feb 18 15:08:44 UTC 2015
On Wednesday 18 February 2015 15:13:21 you wrote:
> 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?
So other instances of programs that want to change
file know that it is in use (file+ existing) and wait?
> > 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;
> > }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20150218/cfeb49fd/attachment-0001.html>
More information about the busybox
mailing list