[PATCH] fix util_lookup_group to handle large groups

[PATCH] fix util_lookup_group to handle large groups

[Eric W. Biederman]

I have recently been getting the above message on fc11 and
I have traced it down to a bug in util_lookup_group.

As of 145 util_lookup_group reads:

gid_t util_lookup_group(struct udev *udev, const char *group)
{
	char *endptr;
	int buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
	char buf[buflen];
	struct group grbuf;
	struct group *gr;
	gid_t gid = 0;

	if (strcmp(group, "root") = 0)
		return 0;
	gid = strtoul(group, &endptr, 10);
	if (endptr[0] = '\0')
		return gid;

	errno = 0;
	getgrnam_r(group, &grbuf, buf, buflen, &gr);
	if (gr != NULL)
		return gr->gr_gid;
	if (errno = 0 || errno = ENOENT || errno = ESRCH)
		err(udev, "specified group '%s' unknown\n", group);
	else
		err(udev, "error resolving group '%s': %m\n", group);
	return 0;
}

The errno value from getgrnam_r here is ERANGE which is documented as
"Insufficient buffer space supplied".

When I call get getgrnam_r with a large enough buffer everything
works.  Indicating that the problem is that sysconf is returning
a value too small.

A quick google search tells me that sysconf(_S_GETGR_R_SIZE_MAX)
is documented as:

> sysconf(_S_GETGR_R_SIZE_MAX) returns either -1 or a good
> suggested starting value for buflen.  It does not return the
> worst case possible for buflen.

In my case I have a group with about 50 users in /etc/group
and that is what triggered the problem in udev and caused
all of the udevs group lookups to fail.

The following patch which dynamically allocates the group member buffer
should fix this problem.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

---

--- udev-145/libudev/libudev-util-private.c-orig	2009-07-31 22:03:54.000000000 -0700
+++ udev-145/libudev/libudev-util-private.c	2009-07-31 22:23:21.000000000 -0700
@@ -149,8 +149,8 @@
 gid_t util_lookup_group(struct udev *udev, const char *group)
 {
 	char *endptr;
-	int buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
-	char buf[buflen];
+	int buflen;
+	char *buf;
 	struct group grbuf;
 	struct group *gr;
 	gid_t gid = 0;
@@ -161,15 +161,31 @@
 	if (endptr[0] = '\0')
 		return gid;
 
-	errno = 0;
-	getgrnam_r(group, &grbuf, buf, buflen, &gr);
-	if (gr != NULL)
-		return gr->gr_gid;
-	if (errno = 0 || errno = ENOENT || errno = ESRCH)
-		err(udev, "specified group '%s' unknown\n", group);
-	else
-		err(udev, "error resolving group '%s': %m\n", group);
-	return 0;
+	buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+	if (buflen < 0)
+		buflen = 1000;
+	buf = NULL;
+	gid = 0;
+	for (;;) {
+		buf = realloc(buf, buflen);
+		if (!buf)
+			break;
+		errno = 0;
+		getgrnam_r(group, &grbuf, buf, buflen, &gr);
+		if (gr != NULL) 
+			gid = gr->gr_gid;
+		else if (errno = ERANGE) {
+			buflen *= 2;
+			continue;
+		}
+		else if (errno = 0 || errno = ENOENT || errno = ESRCH)
+			err(udev, "specified group '%s' unknown\n", group);
+		else
+			err(udev, "error resolving group '%s': %m\n", group);
+		break;
+	}
+	free(buf);
+	return gid;
 }
 
 /* handle "[<SUBSYSTEM>/<KERNEL>]<attribute>" format */

Re: [PATCH] fix util_lookup_group to handle large groups

[Alan Jenkins]

On 8/1/09, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> I have recently been getting the above message on fc11 and
> I have traced it down to a bug in util_lookup_group.
>
> As of 145 util_lookup_group reads:
>
> gid_t util_lookup_group(struct udev *udev, const char *group)
> {
> 	char *endptr;
> 	int buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
> 	char buf[buflen];
> 	struct group grbuf;
> 	struct group *gr;
> 	gid_t gid = 0;
>
> 	if (strcmp(group, "root") = 0)
> 		return 0;
> 	gid = strtoul(group, &endptr, 10);
> 	if (endptr[0] = '\0')
> 		return gid;
>
> 	errno = 0;
> 	getgrnam_r(group, &grbuf, buf, buflen, &gr);
> 	if (gr != NULL)
> 		return gr->gr_gid;
> 	if (errno = 0 || errno = ENOENT || errno = ESRCH)
> 		err(udev, "specified group '%s' unknown\n", group);
> 	else
> 		err(udev, "error resolving group '%s': %m\n", group);
> 	return 0;
> }
>
> The errno value from getgrnam_r here is ERANGE which is documented as
> "Insufficient buffer space supplied".
>
> When I call get getgrnam_r with a large enough buffer everything
> works.  Indicating that the problem is that sysconf is returning
> a value too small.
>
> A quick google search tells me that sysconf(_S_GETGR_R_SIZE_MAX)
> is documented as:
>
>> sysconf(_S_GETGR_R_SIZE_MAX) returns either -1 or a good
>> suggested starting value for buflen.  It does not return the
>> worst case possible for buflen.
>
> In my case I have a group with about 50 users in /etc/group
> and that is what triggered the problem in udev and caused
> all of the udevs group lookups to fail.
>
> The following patch which dynamically allocates the group member buffer
> should fix this problem.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Makes sense.  Thanks, this was my mistake.  I believed the manpage for
getgrnam_r.

"The maximum needed size for buf can be found using sysconf(3) with
the _SC_GETGR_R_SIZE_MAX parameter."

Regards
Alan

Re: [PATCH] fix util_lookup_group to handle large groups

[Kay Sievers]

On Sat, Aug 1, 2009 at 01:26, Eric W. Biederman<ebiederm@xmission.com> wrote:

>> sysconf(_S_GETGR_R_SIZE_MAX) returns either -1 or a good
>> suggested starting value for buflen.  It does not return the
>> worst case possible for buflen.
>
> In my case I have a group with about 50 users in /etc/group
> and that is what triggered the problem in udev and caused
> all of the udevs group lookups to fail.
>
> The following patch which dynamically allocates the group member buffer
> should fix this problem.

Applied.

Thanks,
Kay

Re: [PATCH] fix util_lookup_group to handle large groups

[Ansgar Johannes Pflipsen]

I have recently getting errors like:

  udevd[1030]: error resolving group 'audio': Numerical result out of range

(udevadm --version => 141, uname -r => 2.6.26-TI)
(getent group audio | tr , ' ' | wc => 1 120 874)
(An older system (debian based udevadm --version => 125) works fine)

and found following patch to address the ERANGE problem of getgrnam_r.
(code taken from git.kernel.org, HEAD)

This patch probably has a race condition if sysconf returns 0:

	buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
	if (buflen < 0)			<<-- HERE, better use <		buflen = 1000;
	buf = NULL;
	gid = 0;
	for (;;) {
		buf = realloc(buf, buflen);
		if (!buf)
			break;
		errno = 0;
		getgrnam_r(group, &grbuf, buf, buflen, &gr);
		if (gr != NULL)
			gid = gr->gr_gid;
		else if (errno = ERANGE) {
			buflen *= 2;	<<-- 0 * 2 = 0
			continue;
		}

BTW, are there any special reasons not to use getgrnam like this:

gid_t util_lookup_group(struct udev *udev, const char *group)
{
	char *endptr;
	struct group *gr;

	if (strcmp(group, "root") = 0)
		return 0;
	gid = strtoul(group, &endptr, 10);
	if (endptr[0] = '\0')
		return gid;
	
	gr=getgrnam(group);
	if (gr != NULL)
		return gr->gr_gid;
	if (errno = 0 || errno = ENOENT || errno = ESRCH)
		err(udev, "specified group '%s' unknown\n", group);
	else
		err(udev, "error resolving group '%s': %m\n", group);
	return 0;
}

this avoids any buffer handling at all.
the same may apply to util_lookup_user().

Regards
Ansgar

Re: [PATCH] fix util_lookup_group to handle large groups

[Lennart Poettering]

On Mon, 24.08.09 18:50, Ansgar Johannes Pflipsen (pflipsen@ti.rwth-aachen.de) wrote:

> 		errno = 0;
> 		getgrnam_r(group, &grbuf, buf, buflen, &gr);
> 		if (gr != NULL)
> 			gid = gr->gr_gid;
> 		else if (errno = ERANGE) {
> 			buflen *= 2;	<<-- 0 * 2 = 0
> 			continue;
> 		}

One little comment here: on POSIX getrnam_r() doesn't touch
errno. Instead it returns the error value as return value.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: [PATCH] fix util_lookup_group to handle large groups

[Ansgar Johannes Pflipsen]

Some more issues:
1) multi-threading
Assuming util_lookup_group() has to be thread save
my solution wont work reliable, found 
> The getgrnam() function need not be thread-safe.
so we have to the reentrant version.

2) errno
Lennart Poettering <lennart@poettering.net> wrote:
> One little comment here: on POSIX getrnam_r() doesn't touch
> errno. Instead it returns the error value as return value.

3) sysconf(_SC_GETGR_R_SIZE_MAX)
obviously wrong documented, its return value can only be used as hint.
IMHO its worth to initiate with some usefull defaults, because
repeatedly calling getgrnam_r does'nt come for free.

4) realloc
if we additionally want to catch realloc failure
this one should do:

gid_t util_lookup_group(struct udev *udev, const char *group)
{
	char *endptr;
	int buflen;
	struct group grbuf;
	struct group *gr;
	char *mem;
	char *buf = NULL;
	gid_t gid = 0;

	if (strcmp(group, "root") = 0)
		return 0;
	gid = strtoul(group, &endptr, 10);
	if (endptr[0] = '\0')
		return gid;

	buflen = sysconf(_SC_GETGR_R_SIZE_MAX);
	if (buflen < 32)
		buflen = 1024;
	for (;mem = realloc(buf, buflen); buflen *=2 )
		{
		buf=mem;
		errno=getgrnam_r(group, &grbuf, buf, buflen, &gr);
		if (gr != NULL)
			gid = gr->gr_gid;
		else if (errno = ERANGE) 
			continue;
		else if (errno = 0 || errno = ENOENT || errno = ESRCH)
			err(udev, "specified group '%s' unknown\n", group);
		else
			err(udev, "error resolving group '%s': %m\n", group);
		break;
	}
	if (!mem)
		err(udev, "error resolving group '%s': allocating %d bytes failed\n", group,buflen);
	if (buf)
		free(buf);
	return gid;
}

sorry, no patch yet.
currently using numeric gids in udev rules to avoid ERANGE error.
BTW util_lookup_user needs to be checked too.