Re: Possible bug in NFSv4 with krb5p security?

Andrey Simonnenko wrote:
> 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;
> }
> }
> 
All looks fine to me. (Before my mailer messed with the whitespace;-)

Thanks, rick
ps: I will commit it in April, unless someone else does so sooner.

> 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);
> }
> ---------------------------------
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe_at_freebsd.org"