Re: Bug in the deluser applet
On Tue, Feb 17, 2015 at 4:52 PM, Laszlo Papp wrote: > Hi, > > 1) busybox addgroup user > 2) busybox adduser -G user testuser1 > 3) busybox adduser -G user testuser2 > 4) busybox deluser testuser1 > 5) grep user /etc/passwd /etc/group > > ... > /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash > ... > /etc/group:user:x:1007:testuser,testuser2 >^^ Apologies; that is wrong copy/paste. I meant to write this: /etc/group:user:x:1007:testuser1,testuser2 ^^ > ... > > This is not how the desktop util behaves and I agree with the desktop > util in this case. A non-existent user should not remain in > /etc/group. > > Do you agree or disagree? > > Cheers, L. ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
WARNING: this is pseudo code. Do not use it as is. It makes my
/etc/group empty after the run, but it ought to show the concept I am
thinking of. As indicated, the implementation has some bug currently.
Any feedback is welcome.
commit d7075bb9195a65d11b43f516112f5b0134cbc62a
Author: Laszlo Papp
Date: Tue Feb 17 20:01:04 2015 +
Delete the user from all the groups for user deletion
diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
index a30af6f..f280813 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+");
@@ -167,7 +171,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 && strncmp(name_colon, line, user_len) != 0) {
fprintf(new_fp, "%s\n", line);
goto next;
}
@@ -175,8 +179,21 @@ int FAST_FUNC update_passwd(const char *filename,
/* We have a match with "name:"... */
cp = line + user_len; /* move past name: */
+if (!name && member) {
+struct group* g;
+while ((g = getgrent())) {
+char **s= g->gr_mem;
+char **d = s;
+while (s && *s) {
+if (strcmp(*s, member)) { *d = *s; ++d; }
+++s;
+}
+*d = 0;
+}
+}
+
#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'
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;
}
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: > Hi, > > 1) busybox addgroup user > 2) busybox adduser -G user testuser1 > 3) busybox adduser -G user testuser2 > 4) busybox deluser testuser1 > 5) grep user /etc/passwd /etc/group > > ... > /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash > ... > /etc/group:user:x:1007:testuser,testuser2 >^^ > ... > > This is not how the desktop util behaves and I agree with the desktop > util in this case. A non-existent user should not remain in > /etc/group. > > Do you agree or disagree? > > Cheers, L. Hi, i agree that this case is not covered by actual bb's deluser code. What we cover at the moment is: deluser USER (and group with same name if empty) deluser USER from GROUP The desktop util behaviour could be added tough. By looking at the pseudo code you posted in a later mail I wonder if it could be easier to change deluser rather than update_passwd which is more complex. Like: do_deluser get_user_group_list do_delgroup of primary group while_list do_deluser_from_group or do_deluser while getgrent do_deluser_from_group(user, grname) do_delgroup of primary group Ciao, Tito ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
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. On Wednesday, February 18, 2015, tito wrote: > On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: > >> Hi, > >> > >> 1) busybox addgroup user > >> 2) busybox adduser -G user testuser1 > >> 3) busybox adduser -G user testuser2 > >> 4) busybox deluser testuser1 > >> 5) grep user /etc/passwd /etc/group > >> > >> ... > >> /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash > >> ... > >> /etc/group:user:x:1007:testuser,testuser2 > >> ^^ > >> ... > >> > >> This is not how the desktop util behaves and I agree with the desktop > >> util in this case. A non-existent user should not remain in > >> /etc/group. > >> > >> Do you agree or disagree? > >> > >> Cheers, L. > > > > Hi, > > i agree that this case is not covered by actual bb's deluser code. > > What we cover at the moment is: > > > > deluser USER (and group with same name if empty) > > deluser USER from GROUP > > > > > > The desktop util behaviour could be added tough. > > By looking at the pseudo code you posted in a later > > mail I wonder if it could be easier to change > > deluser rather than update_passwd which is more complex. > > Like: > > > > do_deluser > > get_user_group_list > > do_delgroup of primary group > > while_list > > do_deluser_from_group > > or > > do_deluser > > while getgrent > > do_deluser_from_group(user, grname) > > do_delgroup of primary group > > > > Ciao, > > Tito ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On February 18, 2015 8:34:14 AM GMT+01:00, Laszlo Papp 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. > >On Wednesday, February 18, 2015, tito wrote: >> On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: >> >>> Hi, >> >>> >> >>> 1) busybox addgroup user >> >>> 2) busybox adduser -G user testuser1 >> >>> 3) busybox adduser -G user testuser2 >> >>> 4) busybox deluser testuser1 >> >>> 5) grep user /etc/passwd /etc/group >> >>> >> >>> ... >> >>> /etc/passwd:testuser2:x:1010:1007:Linux >User,,,:/home/testuser2:/bin/bash >> >>> ... >> >>> /etc/group:user:x:1007:testuser,testuser2 >> >>> ^^ >> >>> ... >> >>> >> >>> This is not how the desktop util behaves and I agree with the >desktop >> >>> util in this case. A non-existent user should not remain in >> >>> /etc/group. >> >>> >> >>> Do you agree or disagree? >> >>> >> >>> Cheers, L. >> >> >> >> Hi, >> >> i agree that this case is not covered by actual bb's deluser code. >> >> What we cover at the moment is: >> >> >> >> deluser USER (and group with same name if empty) >> >> deluser USER from GROUP >> >> >> >> >> >> The desktop util behaviour could be added tough. >> >> By looking at the pseudo code you posted in a later >> >> mail I wonder if it could be easier to change >> >> deluser rather than update_passwd which is more complex. >> >> Like: >> >> >> >> do_deluser >> >> get_user_group_list >> >> do_delgroup of primary group >> >> while_list >> >> do_deluser_from_group >> >> or >> >> do_deluser >> >> while getgrent >> >> do_deluser_from_group(user, grname) >> >> do_delgroup of primary group I'd do the latter I think. Tito, can you please cook a patch? TIA! ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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. > On Wednesday, February 18, 2015, tito wrote: >> On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: >> >>> Hi, >> >>> >> >>> 1) busybox addgroup user >> >>> 2) busybox adduser -G user testuser1 >> >>> 3) busybox adduser -G user testuser2 >> >>> 4) busybox deluser testuser1 >> >>> 5) grep user /etc/passwd /etc/group >> >>> >> >>> ... >> >>> /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash >> >>> ... >> >>> /etc/group:user:x:1007:testuser,testuser2 >> >>> ^^ >> >>> ... >> >>> >> >>> This is not how the desktop util behaves and I agree with the desktop >> >>> util in this case. A non-existent user should not remain in >> >>> /etc/group. >> >>> >> >>> Do you agree or disagree? >> >>> >> >>> Cheers, L. >> >> >> >> Hi, >> >> i agree that this case is not covered by actual bb's deluser code. >> >> What we cover at the moment is: >> >> >> >> deluser USER (and group with same name if empty) >> >> deluser USER from GROUP >> >> >> >> >> >> The desktop util behaviour could be added tough. >> >> By looking at the pseudo code you posted in a later >> >> mail I wonder if it could be easier to change >> >> deluser rather than update_passwd which is more complex. >> >> Like: >> >> >> >> do_deluser >> >> get_user_group_list >> >> do_delgroup of primary group >> >> while_list >> >> do_deluser_from_group >> >> or >> >> do_deluser >> >> while getgrent >> >> do_deluser_from_group(user, grname) >> >> do_delgroup of primary group >> >> >> >> Ciao, >> >> Tito ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote: > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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. >> On Wednesday, February 18, 2015, tito wrote: >>> On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: >>> Hi, >>> >>> 1) busybox addgroup user >>> 2) busybox adduser -G user testuser1 >>> 3) busybox adduser -G user testuser2 >>> 4) busybox deluser testuser1 >>> 5) grep user /etc/passwd /etc/group >>> >>> ... >>> /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash >>> ... >>> /etc/group:user:x:1007:testuser,testuser2 >>> ^^ >>> ... >>> >>> This is not how the desktop util behaves and I agree with the desktop >>> util in this case. A non-existent user should not remain in >>> /etc/group. >>> >>> Do you agree or disagree? >>> >>> Cheers, L. >>> >>> >>> >>> Hi, >>> >>> i agree that this case is not covered by actual bb's deluser code. >>> >>> What we cover at the moment is: >>> >>> >>> >>> deluser USER (and group with same name if empty) >>> >>> deluser USER from GROUP >>> >>> >>> >>> >>> >>> The desktop util behaviour could be added tough. >>> >>> By looking at the pseudo code you posted in a later >>> >>> mail I wonder if it could be easier to change >>> >>> deluser rather than update_passwd which is more complex. >>> >>> Like: >>> >>> >>> >>> do_deluser >>> >>> get_user_group_list >>> >>> do_delgroup of primary group >>> >>> while_list >>> >>> do_deluser_from_group >>> >>> or >>> >>> do_deluser >>> >>> while getgrent >>> >>> do_deluser_from_group(user, grname) >>> >>> do_delgroup of primary group >>> >>> >>> >>> Ciao, >>> >>> Tito ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On Wednesday 18 February 2015 09:14:42 you wrote: > On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote: > > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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. Ciao, Tito > >> On Wednesday, February 18, 2015, tito wrote: > >>> On Tuesday 17 February 2015 17:52:06 Laszlo Papp wrote: > >>> > Hi, > >>> > > >>> > 1) busybox addgroup user > >>> > 2) busybox adduser -G user testuser1 > >>> > 3) busybox adduser -G user testuser2 > >>> > 4) busybox deluser testuser1 > >>> > 5) grep user /etc/passwd /etc/group > >>> > > >>> > ... > >>> > /etc/passwd:testuser2:x:1010:1007:Linux User,,,:/home/testuser2:/bin/bash > >>> > ... > >>> > /etc/group:user:x:1007:testuser,testuser2 > >>> > ^^ > >>> > ... > >>> > > >>> > This is not how the desktop util behaves and I agree with the desktop > >>> > util in this case. A non-existent user should not remain in > >>> > /etc/group. > >>> > > >>> > Do you agree or disagree? > >>> > > >>> > Cheers, L. > >>> > >>> > >>> > >>> Hi, > >>> > >>> i agree that this case is not covered by actual bb's deluser code. > >>> > >>> What we cover at the moment is: > >>> > >>> > >>> > >>> deluser USER (and group with same name if empty) > >>> > >>> deluser USER from GROUP > >>> > >>> > >>> > >>> > >>> > >>> The desktop util behaviour could be added tough. > >>> > >>> By looking at the pseudo code you posted in a later > >>> > >>> mail I wonder if it could be easier to change > >>> > >>> deluser rather than update_passwd which is more complex. > >>> > >>> Like: > >>> > >>> > >>> > >>> do_deluser > >>> > >>> get_user_group_list > >>> > >>> do_delgroup of primary group > >>> > >>> while_list > >>> > >>> do_deluser_from_group > >>> > >>> or > >>> > >>> do_deluser > >>> > >>> while getgrent > >>> > >>> do_deluser_from_group(user, grname) > >>> > >>> do_delgroup of primary group > >>> > >>> > >>> > >>> Ciao, > >>> > >>> Tito > ___ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 11:00 AM, tito wrote:
> On Wednesday 18 February 2015 09:14:42 you wrote:
>
>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote:
>
>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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?
commit 813e3db073aca7a55b9f3d4650055e7d1ed768b4
Author: Laszlo Papp
Date: Tue Feb 17 20:01:04 2015 +
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 = xmal
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 12:09 PM, Laszlo Papp wrote:
> On Wed, Feb 18, 2015 at 11:00 AM, tito wrote:
>> On Wednesday 18 February 2015 09:14:42 you wrote:
>>
>>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote:
>>
>>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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
> Date: Tue Feb 17 20:01:04 2015 +
>
> 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 *filen
Re: Bug in the deluser applet
On Wednesday 18 February 2015 15:13:21 you wrote: > On Wed, Feb 18, 2015 at 12:09 PM, Laszlo Papp wrote: > > On Wed, Feb 18, 2015 at 11:00 AM, tito wrote: > >> On Wednesday 18 February 2015 09:14:42 you wrote: > >> > >>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote: > >> > >>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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 > > Date: Tue Feb 17 20:01:04 2015 + > > > > 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 */ > > fn
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 3:08 PM, tito wrote: > On Wednesday 18 February 2015 15:13:21 you wrote: > >> On Wed, Feb 18, 2015 at 12:09 PM, Laszlo Papp wrote: > >> > On Wed, Feb 18, 2015 at 11:00 AM, tito wrote: > >> >> On Wednesday 18 February 2015 09:14:42 you wrote: > >> >> > >> >>> On Wed, Feb 18, 2015 at 8:09 AM, Laszlo Papp wrote: > >> >> > >> >>> > On Wed, Feb 18, 2015 at 7:34 AM, Laszlo Papp 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? Is that a valid reasoning? The file ought to be opened in locked mode all the time when starting the operation, in my opinion, in which case this does not apply. Furthermore, if I precreate the "lock" file like that, it is easy to break the applet by just creating the file beforehand. man 2 flock (LOCK_EX) man 3 mkstemp > > > >> > commit 813e3db073aca7a55b9f3d4650055e7d1ed768b4 > >> > Author: Laszlo Papp > >> > Date: Tue Feb 17 20:01:04 2015 + > >> > > >> > 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 : up
Re: Bug in the deluser applet
> 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
Date: Wed Feb 18 15:20:58 2015 +
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
Re: Bug in the deluser applet
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
> Date: Wed Feb 18 15:20:58 2015 +
>
> 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
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 3:54 PM, tito 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
>
>> Date: Wed Feb 18 15:20:58 2015 +
>
>>
>
>> 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,
Re: Bug in the deluser applet
On Wednesday 18 February 2015 17:16:56 you wrote:
> On Wed, Feb 18, 2015 at 3:54 PM, tito 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
> >
> >> Date: Wed Feb 18 15:20:58 2015 +
> >
> >>
> >
> >> 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/lo
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 4:34 PM, tito wrote:
> On Wednesday 18 February 2015 17:16:56 you wrote:
>
>> On Wed, Feb 18, 2015 at 3:54 PM, tito 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
>
>> >
>
>> >> Date: Wed Feb 18 15:20:58 2015 +
>
>> >
>
>> >>
>
>> >
>
>> >> 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 ||
>> >> ENABL
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 4:40 PM, Laszlo Papp wrote:
> On Wed, Feb 18, 2015 at 4:34 PM, tito wrote:
>> On Wednesday 18 February 2015 17:16:56 you wrote:
>>
>>> On Wed, Feb 18, 2015 at 3:54 PM, tito 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
>>
>>> >
>>
>>> >> Date: Wed Feb 18 15:20:58 2015 +
>>
>>> >
>>
>>> >>
>>
>>> >
>>
>>> >> 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) /
Re: Bug in the deluser applet
On Wed, Feb 18, 2015 at 3:37 PM, Laszlo Papp 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
> Date: Wed Feb 18 15:20:58 2015 +
>
> 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
Re: Bug in the deluser applet
On Thursday 19 February 2015 17:56:27 you wrote:
> On Wed, Feb 18, 2015 at 3:37 PM, Laszlo Papp 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
> > Date: Wed Feb 18 15:20:58 2015 +
> >
> > 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. ;-)
Hi,
just to understand it correctly, the undefined behaviour is in the patch
and not in the actual code?
I suggest as our patches are mutually exclusive to post them both
in a new thread labeled with the word [PATCH] with a short explanation
of the new feature they introduce and of the pro and contras of each one
and let Denys the time to decide what he likes best, because I doubt
he will read this whole thread.
Let me know if you agree.
Ciao,
Tito
> >
Re: Bug in the deluser applet
On Thu, Feb 19, 2015 at 4:56 PM, Laszlo Papp wrote:
> On Wed, Feb 18, 2015 at 3:37 PM, Laszlo Papp 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
>> Date: Wed Feb 18 15:20:58 2015 +
>>
>> 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); }
That is actually wrong, too, and needs to be fixed as soon as
possible. The issue is that if for instance the first username is to
be removed, the comma will be left in, but it should not be. I am
submitting a new change fixing this, although it is straight-forward
how to do it, in my humble opinion.
>> +++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 |
Re: Bug in the deluser applet
>From b03ad793d1188148953fa280dda672229b9a6524 Mon Sep 17 00:00:00 2001
From: Laszlo Papp
Date: Wed, 18 Feb 2015 15:20:58 +
Subject: [PATCH] Delete the user from all the groups for user deletion
---
libbb/update_passwd.c | 57 +++
loginutils/deluser.c | 4 +++-
2 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
index a30af6f..df544f0 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,38 @@ 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;
+bool sep = false;
+snprintf(gline, sizeof(gline), "%s:%s:%i:",
g->gr_name, g->gr_passwd, g->gr_gid);
+while (*s) {
+if (strcmp(*s, member)) { if (sep) strcat(gline,
","); else sep = true; strcat(gline, *s); }
+++s;
+}
+fprintf(new_fp, "%s\n", gline);
+continue;
+} 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 +261,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
Re: Bug in the deluser applet
Denys, any feedback about this bugfix?
On Thu, Feb 19, 2015 at 6:16 PM, Laszlo Papp wrote:
> From b03ad793d1188148953fa280dda672229b9a6524 Mon Sep 17 00:00:00 2001
> From: Laszlo Papp
> Date: Wed, 18 Feb 2015 15:20:58 +
> Subject: [PATCH] Delete the user from all the groups for user deletion
>
> ---
> libbb/update_passwd.c | 57
> +++
> loginutils/deluser.c | 4 +++-
> 2 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
> index a30af6f..df544f0 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,38 @@ 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;
> +bool sep = false;
> +snprintf(gline, sizeof(gline), "%s:%s:%i:",
> g->gr_name, g->gr_passwd, g->gr_gid);
> +while (*s) {
> +if (strcmp(*s, member)) { if (sep) strcat(gline,
> ","); else sep = true; strcat(gline, *s); }
> +++s;
> +}
> +fprintf(new_fp, "%s\n", gline);
> +continue;
> +} 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 +261,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_delu
Re: Bug in the deluser applet
On Tue, Mar 3, 2015 at 2:13 PM, Laszlo Papp wrote:
> Denys, any feedback about this bugfix?
It is whitespace damaged.
>> +char *cp, *line;
>> +if (!name && member) {
>> +struct group* g;
>> +if ((g = getgrent())) {
Read the file using getgtent?
The rest of the function uses comletely different approach.
I am applying the attached patch.
diff -d -urpN busybox.5/libbb/update_passwd.c busybox.6/libbb/update_passwd.c
--- busybox.5/libbb/update_passwd.c 2015-02-16 14:42:53.0 +0100
+++ busybox.6/libbb/update_passwd.c 2015-03-12 15:23:46.359124022 +0100
@@ -62,6 +62,8 @@ static void check_selinux_update_passwd(
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.
@@ -99,12 +101,13 @@ int FAST_FUNC update_passwd(const char *
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);
+ name_colon = xasprintf("%s:", name ? name : "");
user_len = strlen(name_colon);
if (shadow)
@@ -167,6 +170,37 @@ int FAST_FUNC update_passwd(const char *
line = xmalloc_fgetline(old_fp);
if (!line) /* EOF/error */
break;
+
+ if (!name && member) {
+ /* Delete member from all groups */
+ /* line is "GROUP:PASSWD:[member1[,member2]...]" */
+ unsigned member_len = strlen(member);
+ char *list = strrchr(line, ':');
+ while (list) {
+list++;
+ next_list_element:
+if (strncmp(list, member, member_len) == 0) {
+ char c;
+ changed_lines++;
+ c = list[member_len];
+ if (c == '\0') {
+ if (list[-1] == ',')
+ list--;
+ *list = '\0';
+ break;
+ }
+ if (c == ',') {
+ overlapping_strcpy(list, list + member_len + 1);
+ goto next_list_element;
+ }
+ changed_lines--;
+}
+list = strchr(list, ',');
+ }
+ fprintf(new_fp, "%s\n", line);
+ goto next;
+ }
+
if (strncmp(name_colon, line, user_len) != 0) {
fprintf(new_fp, "%s\n", line);
goto next;
diff -d -urpN busybox.5/loginutils/deluser.c busybox.6/loginutils/deluser.c
--- busybox.5/loginutils/deluser.c 2015-02-16 14:42:53.0 +0100
+++ busybox.6/loginutils/deluser.c 2015-03-12 15:28:36.136921651 +0100
@@ -114,16 +114,22 @@ int deluser_main(int argc, char **argv)
}
} while (ENABLE_FEATURE_SHADOWPASSWDS && pfile);
- if (ENABLE_DELGROUP && do_deluser > 0) {
- /* "deluser USER" also should try to delete
- * same-named group. IOW: do "delgroup USER"
- */
+ if (do_deluser > 0) {
+ /* Delete user from all groups */
+ if (update_passwd(bb_path_group_file, NULL, NULL, name) == -1)
+return EXIT_FAILURE;
+
+ if (ENABLE_DELGROUP) {
+/* "deluser USER" also should try to delete
+ * same-named group. IOW: do "delgroup USER"
+ */
// On debian deluser is a perl script that calls userdel.
// From man userdel:
// If USERGROUPS_ENAB is defined to yes in /etc/login.defs, userdel will
// delete the group with the same name as the user.
- do_deluser = -1;
- goto do_delgroup;
+do_deluser = -1;
+goto do_delgroup;
+ }
}
return EXIT_SUCCESS;
}
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
