[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    [PATCH] addgroup could assign already in use group
From:       tito <farmatito () tiscali ! it>
Date:       2014-09-20 14:45:14
Message-ID: 201409201645.14614.farmatito () tiscali ! it
[Download RAW message or body]

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


["addgroup2.patch" (text/x-patch)]

Addgroup could assign a wrong (already in use) gid
as getgrgid returns NULL also on error (e.g. ERANGE).

Signed-off-by: Tito Ragusa <farmatito@tiscali.it>

--- 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 */


_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]