Re: [PATCH] libxl: Be more careful with error handling in libxl__dm_runas_helper()

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-11-26 at 10:03 +0000, Ian Campbell wrote:
> On Wed, 2015-11-25 at 16:56 -0500, Boris Ostrovsky wrote:
> > getpwnam_r() has fairly complicated return rules. From man pages:
> > 
> >   RETURN VALUE
> >       ...
> >       On success, getpwnam_r() and getpwuid_r() return zero, and set
> >       *result to pwd.  If no matching  password record was found, these
> >       functions return 0 and store NULL in *result. In case of error,
> >       an error number is returned, and NULL is stored in *result.
> >   ERRORS
> >       0 or ENOENT or ESRCH or EBADF or EPERM or ...
> >             The given name or uid was not found.
> 
> My reference when reviewing this is the (IMHO more canonical) http://pubs.o
> pengroup.org/onlinepubs/9699919799/functions/getpwnam.html .

Maybe we should be considering whether "Applications wishing to check for
error situations should set errno to 0 before calling getpwnam(). If
getpwnam() returns a null pointer and errno is non-zero, an error
occurred." ought to be applies to getpwnam_r too?

> 
> I suppose you are looking at the Linux and/or glibc man pages?
> 
> > While it's not clear what ellipses are meant to be, the way we
> > currently
> > treat return values from getpwnam_r() is no sufficient. In fact, two of
> > my systems behave differently when username is not found: one returns
> > ENOENT and the other returns 0.
> 
> Which two systems are these? When you say "returns" do you mean "returns
> 0
> and sets errno to XXX" or literally returns ENOENT?
> 
> >  Both set *result to NULL.
> > 
> > This patch adjusts return value management to be more in line with man
> > pages.
> > 
> > While at it, also make sure we don't get stuck on ERANGE.
> > 
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > ---
> >  tools/libxl/libxl_dm.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index a4934df..bd3daeb 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -726,7 +726,7 @@ static int libxl__dm_runas_helper(libxl__gc *gc,
> > const char *username)
> >      struct passwd pwd, *user = NULL;
> >      char *buf = NULL;
> >      long buf_size;
> > -    int ret;
> > +    int ret, retry_cnt = 0;
> >  
> >      buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> >      if (buf_size < 0) {
> > @@ -740,12 +740,17 @@ static int libxl__dm_runas_helper(libxl__gc *gc,
> > const char *username)
> >          ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
> >          if (ret == ERANGE) {
> >              buf_size += 128;
> > +            if (retry_cnt++ > 10)
> > +                    return ERROR_FAIL;
> >              continue;
> >          }
> > -        if (ret != 0)
> > -            return ERROR_FAIL;
> > -        if (user != NULL)
> > -            return 1;
> > +        if (user == NULL) {
> > +            if (!ret || (ret == ENOENT) || (ret == ESRCH) ||
> > +                (ret == EBADF) || (ret == EPERM))
> > +                return ERROR_NOTFOUND;
> > +            else
> > +                return ERROR_FAIL;
> > +        }
> >          return 0;
> >      }
> >  }
> > @@ -1261,16 +1266,16 @@ static int
> > libxl__build_device_model_args_new(libxl__gc *gc,
> >  
> >          user = GCSPRINTF("%s%d", LIBXL_QEMU_USER_BASE, guest_domid);
> >          ret = libxl__dm_runas_helper(gc, user);
> > -        if (ret < 0)
> > +        if (ret && (ret != ERROR_NOTFOUND))
> >              return ret;
> > -        if (ret > 0)
> > +        if (!ret)
> >              goto end_search;
> >  
> >          user = LIBXL_QEMU_USER_SHARED;
> >          ret = libxl__dm_runas_helper(gc, user);
> > -        if (ret < 0)
> > +        if (ret && (ret != ERROR_NOTFOUND))
> >              return ret;
> > -        if (ret > 0) {
> > +        if (!ret) {
> >              LOG(WARN, "Could not find user %s%d, falling back to %s",
> >                      LIBXL_QEMU_USER_BASE, guest_domid,
> > LIBXL_QEMU_USER_SHARED);
> >              goto end_search;
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel