[PATCH] addgroup could assign already in use group

[tito]

Hi,
while looking at the long username stuff and malloced getpwxx functions
I've noticed a bug in addgroup resulting in assignement of a wrong
(already in use) gid. In the function xgroup_study:

static void xgroup_study(struct group *g)
{

snip

	/* Check if the desired gid is free
	 * or find the first free one */
	while (1) {
		if (!getgrgid(g->gr_gid)) {


			return; /* found free group: return */
		}
snip

		g->gr_gid++;
	}
}

The call to getgrgid can return NULL also in case of error:

       0 or ENOENT or ESRCH or EBADF or EPERM or ...
              The given name or gid was not found.

       EINTR  A signal was caught.

       EIO    I/O error.

       EMFILE The maximum number (OPEN_MAX) of files was open already in the calling process.

       ENFILE The maximum number of files was open already in the system.

       ENOMEM Insufficient memory to allocate group structure.

       ERANGE Insufficient buffer space supplied.

a simple fix would be:

	while (1) {
-		if (!getgrgid(g->gr_gid)) {
+		if (!getgrgid(g->gr_gid) && errno == 0) {

which is tested and works with glibc but this is not optimal because as man page says:

"Experiments on various UNIX-like  systems shows that lots of different values occur in this situation:"  (entry is not found)
"0, ENOENT, EBADF, ESRCH, EWOULDBLOCK, EPERM and probably others."

A better solution would be to use getgrgid_r but this increases binary size:

 diff -uNp loginutils/addgroup.c.original loginutils/addgroup.c
--- loginutils/addgroup.c.original      2014-08-13 13:56:04.000000000 +0200
+++ loginutils/addgroup.c       2014-09-20 16:32:44.193586505 +0200
@@ -32,7 +32,10 @@
 static void xgroup_study(struct group *g)
 {
        unsigned max = CONFIG_LAST_ID;
-
+       struct group  *gr;
+       struct group  grp;
+       char buffer[256];
+
        /* Make sure gr_name is unused */
        if (getgrnam(g->gr_name)) {
                bb_error_msg_and_die("%s '%s' in use", "group", g->gr_name);
@@ -53,8 +56,9 @@ static void xgroup_study(struct group *g
        /* Check if the desired gid is free
         * or find the first free one */
        while (1) {
-               if (!getgrgid(g->gr_gid)) {
-                       return; /* found free group: return */
+               if (getgrgid_r(g->gr_gid, &grp, buffer, 256, &gr) == 0) {
+                       if (gr == NULL)
+                               return; /* found free group: return */
                }
                if (option_mask32 & OPT_GID) {
                        /* -g N, cannot pick gid other than N: error */


Attached is a patch with this solution if you think it is worth the effort and size
increase to fix this corner case.

Ciao,
Tito
-------------- next part --------------
A non-text attachment was scrubbed...
Name: addgroup2.patch
Type: text/x-patch
Size: 1021 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20140920/124ed627/attachment.bin>