Re: Possible bug in NFSv4 with krb5p security?

On Tue, Feb 19, 2013 at 08:52:49PM -0500, Rick Macklem wrote:
> > 
> > I cannot find how to get information about maximum buffer size for
> > the getpwnam_r() function. This information should be returned by
> > sysconf(_SC_GETPW_R_SIZE_MAX), but since it does not work on FreeBSD
> > it is necessary to guess its size. Original value is 128 and it works
> > for somebody, 1024 works for your environment, but it can fail for
> > another environment.
> > 
> > SUSv4 specifies "Storage referenced by the structure is allocated from
> > the memory provided with the buffer parameter", but then tells about
> > groups
> > in EXAMPLE for getpwnam_r() "Note that sysconf(_SC_GETPW_R_SIZE_MAX)
> > may
> > return -1 if there is no hard limit on the size of the buffer needed
> > to
> > store all the groups returned".
> > 
> > malloc() can give overhead, but that function can try to call
> > getpwnam_r()
> > with buffer allocated from stack and if getpwnam_r() failed with
> > ERANGE
> > use dynamically allocated buffer.
> > 
> > #define PWBUF_SIZE_INI (2 * MAXLOGNAME + 2 * MAXPATHLEN +
> > _PASSWORD_LEN + 1)
> > #define PWBUF_SIZE_INC 128
> > 
> > char bufs[2 * MAXLOGNAME + MAXPATHLEN + PASSWORD_LEN + 1 + 32];
> > 
> > error = getpwnam_r(lname, &pwd, bufs, sizeof(bufs), &pw);
> > if (pw != NULL) {
> > *uidp = pw->pw_uid;
> > return (GSS_S_COMPLETE);
> > } else if (error != ERANGE)
> > return (GSS_S_FAILURE);
> > 
> > size = PWBUF_SIZE_INI;
> > for (;;) {
> > size += PWBUF_SIZE_INC;
> > buf = malloc(size);
> > if (buf == NULL)
> > return (GSS_S_FAILURE);
> > error = getpwnam_r(lname, &pwd, buf, size, &pw);
> > free(buf);
> > if (pw != NULL) {
> > *uidp = pw->pw_uid;
> > return (GSS_S_COMPLETE);
> > } else {
> > if (error == ERANGE &&
> > size <= SIZE_MAX - PWBUF_SIZE_INC)
> > continue;
> > return (GSS_S_FAILURE);
> > }
> > }
> 
> Just my opinion, but I think the above is a good approach.
> (ie. First trying a fairly large buffer on the stack that
>  will succeed 99.99% of the time, but check for ERANGE and
>  loop trying progressively larger malloc'd buffers until
>  it stops reporting ERANGE.)
> 
> I suspect the overheads behind getpwnam_r() are larger than
> the difference between using a buffer on the stack vs malloc,
> so I think it should use a fairly large buffer the first time.
> 
> Personally, I might have coded it as a single do { } while(),
> with the first attempt in it, but that's just personal stylistic
> taste. (You can check for buf != bufs before doing a free() of it.)
> And, if you wanted to be clever, the code could use a static "bufsiz_hint",
> which is set to the largest size needed sofar and that is used as
> the initial malloc size. That way it wouldn't loop as much for a
> site with huge passwd entries. (An entire bio in the GECOS field or ???)
> 

Thanks for the review.

Another variant.  This is a program that can be used for verifying
correctness of the function, just change PWBUF_SIZE_* values and put
some printfs to see when buffer is reallocated.  sizehint is updated
only when malloc() succeeded.

---------------------------------
#include <sys/param.h>
#include <sys/limits.h>

#include <gssapi/gssapi.h>

#include <errno.h>
#include <limits.h>
#include <pwd.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static int
getpwnam_r_func(const char *name, uid_t *uidp)
{
#define PWBUF_SIZE_INI (2 * MAXLOGNAME + MAXPATHLEN + _PASSWORD_LEN)
#define PWBUF_SIZE_INC 128

	static size_t sizehint = PWBUF_SIZE_INI;

	struct passwd pwd;
	struct passwd *pw;
	char *buf;
	size_t size;
	int error;
	char lname[MAXLOGNAME];
	char bufs[PWBUF_SIZE_INI];

	strncpy(lname, name, sizeof(lname));

	buf = bufs;
	size = sizeof(bufs);
	for (;;) {
		error = getpwnam_r(lname, &pwd, buf, size, &pw);
		if (buf != bufs)
			free(buf);
		if (pw != NULL) {
			*uidp = pw->pw_uid;
			return (GSS_S_COMPLETE);
		} else if (error != ERANGE || size > SIZE_MAX - PWBUF_SIZE_INC)
			return (GSS_S_FAILURE);
		if (size != sizehint)
			size = sizehint;
		else
			size += PWBUF_SIZE_INC;
		buf = malloc(size);
		if (buf == NULL)
			return (GSS_S_FAILURE);
		sizehint = size;
	}
}

int
main(void)
{
	const char *str[] = { "man", "root", "q", "bin", NULL };
	uid_t uid;
	u_int i;

	for (i = 0; str[i] != NULL; ++i) {
		printf("%-20s\t", str[i]);
		if (getpwnam_r_func(str[i], &uid) == GSS_S_COMPLETE)
			printf("%u\n", uid);
		else
			printf("not found\n");
	}
	return (0);
}
---------------------------------