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

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

[Boris Ostrovsky]

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.

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. 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;
-- 
1.8.1.4

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2670 bytes --]

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.
> 
Wow! Given how much he likes error handling, I can't wait to see Ian
Jackson jumping on this! :-PP

> 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. Both set *result to NULL.
> 
Nice. Or not. :-/

Anyway, given exactly those ellipses, wouldn't it be safer to go the
other way round? I mean something like:

> @@ -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;
> +        }
>
  if (user == NULL) {
      if (ret == EINTR || ret == EIO || ret == EMFILE ||
          ret == ENFILE || ret == ENOMEM)
          return ERROR_FAIL;
      else
          return ERROR_NOTFOUND;
  }

Also, considering libxl attitude toward out-of-memory errors, should we
deal with ENOMEM in a special way?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

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 .

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

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

[Ian Campbell]

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

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

[Wei Liu]

On Wed, Nov 25, 2015 at 04:56:09PM -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.
> 

This is Linux manpage.

Actually http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html
doesn't specify such behaviour.  And FreeBSD doesn't share Linux's view either.

Wei.

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

[Ian Jackson]

Boris Ostrovsky writes ("[PATCH] libxl: Be more careful with error handling in libxl__dm_runas_helper()"):
> 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.

I can't see anything in the SuS docs (which Ian C referred to) saying
that *result is updated even on error.  So I think you need to check
the return value first, and only then *result.

>   ERRORS
>       0 or ENOENT or ESRCH or EBADF or EPERM or ...
>             The given name or uid was not found.
> 
> 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. Both set *result to NULL.

I don't know where all that stuff about ENOENT comes from.  I'm
tempted to say this is a bug in your C library.  But I don't mind
treating ENOENT or ESRCH as ERROR_NOTFOUND.

I do mind treating EBADF or EPERM that way.

> While at it, also make sure we don't get stuck on ERANGE.

If you are going to do anything to this, you should use an
exponentially increasing buffer size.  Bear in mind that getpwnam_r is
already inside the trust boundary.

Thanks,
Ian.

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

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Be more careful with error handling in libxl__dm_runas_helper()"):
> On Thu, 2015-11-26 at 10:03 +0000, Ian Campbell wrote:
> > 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?

Maybe that is the bug in the OS Boris is having this trouble with.  If
so then setting errno first would be a better workaround.

Ian.

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

[Ian Campbell]

On Thu, 2015-11-26 at 17:47 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxl: Be more careful with
> error handling in libxl__dm_runas_helper()"):
> > On Thu, 2015-11-26 at 10:03 +0000, Ian Campbell wrote:
> > > 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?
> 
> Maybe that is the bug in the OS Boris is having this trouble with.  If
> so then setting errno first would be a better workaround.

I can certainly imagine that easily happening, I misread the description in
the link above in exactly this way myself when I first started reviewing
Stefano's original patch.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Ian Campbell]

On Thu, 2015-11-26 at 17:45 +0000, Ian Jackson wrote:
> Boris Ostrovsky writes ("[PATCH] libxl: Be more careful with error
> handling in libxl__dm_runas_helper()"):
> > 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.
> 
> I can't see anything in the SuS docs (which Ian C referred to) saying
> that *result is updated even on error.  So I think you need to check
> the return value first, and only then *result.

Actually, the fourth para of description in [0] (starting "The getpwnam_r()
function shall...") ends:

    A null pointer shall be returned at the location pointed to by result on
    error or if the requested entry is not found.

I also just realised (long after everyone else, apparently) that this
function returns (+ve) Exxx values, not -1 and setting errno.

Together with combining getpwnam and getpwnam_r in the same doc it's almost
like someone was wilfully trying to make this function difficult to figure
out...

> 
> >   ERRORS
> >       0 or ENOENT or ESRCH or EBADF or EPERM or ...
> >             The given name or uid was not found.
> > 
> > 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. Both set *result to NULL.
> 
> I don't know where all that stuff about ENOENT comes from.  I'm
> tempted to say this is a bug in your C library.  But I don't mind
> treating ENOENT or ESRCH as ERROR_NOTFOUND.
> 
> I do mind treating EBADF or EPERM that way.

FWIW the Linux manpage which Boris has referred to says under NOTES:

    Experiments on various UNIX-like systems show that lots  of      
    different values occur in this situation: 0, ENOENT, EBADF, ESRCH,
    EWOULDBLOCK, EPERM, and probably others

"various" probably includes all sorts of random UNIX-like systems outside
the small set which are actually used with Xen. I'd be inclined to agree
with not including EBADF, EPERM in this way at least until an actual
platform running Xen is found which returns such errors (not suggesting
anyone proactively checking, but just that we should be prepared to accept
the possibility when someone notices and reports it).

Ian.

[0] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html

> 
> > While at it, also make sure we don't get stuck on ERANGE.
> 
> If you are going to do anything to this, you should use an
> exponentially increasing buffer size.  Bear in mind that getpwnam_r is
> already inside the trust boundary.
> 
> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 11/27/2015 04:40 AM, Ian Campbell wrote:
> On Thu, 2015-11-26 at 17:45 +0000, Ian Jackson wrote:
>> Boris Ostrovsky writes ("[PATCH] libxl: Be more careful with error
>> handling in libxl__dm_runas_helper()"):
>>> 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.
>> I can't see anything in the SuS docs (which Ian C referred to) saying
>> that *result is updated even on error.  So I think you need to check
>> the return value first, and only then *result.
> Actually, the fourth para of description in [0] (starting "The getpwnam_r()
> function shall...") ends:
>
>      A null pointer shall be returned at the location pointed to by result on
>      error or if the requested entry is not found.


So this appears to be common with Linux implementation(s) and therefore 
is what we should use as indication of "non-success".

After that I think we can state that the entry was not found (bit not a 
"real" error occurred) if

   (!ret || (ret==ENOENT) || (ret==ESRCH) || (errno==ENOENT) || 
(errno==ESRCH))

based on this statement from man pages' NOTES:

         The formulation given above under "RETURN VALUE" is from 
POSIX.1-2001.
         It does not call  "not  found"  an error, and hence does not 
specify what
         value errno might have in this situation.  But that makes it 
impossible to
         recognize errors.  One might argue that according to POSIX 
errno should
         be left unchanged  if  an entry  is not found.

(followed by what IanC quotes below --- something that I missed during 
my first reading)

To answer the question asked earlier in this thread --- the two systems 
I tested this on are
Fedora 18 and 20.

This piece of code:

         errno = 0;
         s = getpwnam_r(argv[1], &pwd, buf, bufsize, &result);
         printf("s = %d errno = %d result = %p\n", s, errno, result);
         if (result == NULL) {
                 if (s == 0)
                         printf("Not found\n");
                 else {
                         errno = s;
                         perror("getpwnam_r");
                 }
                 exit(EXIT_FAILURE);
         }

results in:

     root@haswell> cat /etc/redhat-release
     Fedora release 18 (Spherical Cow)
     root@haswell> ./a.out foobar
     s = 0 errno = 0 result = (nil)
     Not found
     root@haswell>

and

     root@orochi-c> cat /etc/redhat-release
     Fedora release 20 (Heisenbug)
     root@orochi-c> ./a.out foobar
     s = 2 errno = 2 result = (nil)
     getpwnam_r: No such file or directory
     root@orochi-c>

which does look like a bug in F20 since the code above is taken from man 
pages' EXAMPLE and doesn't look like it works as intended (which was to 
produce "Not found" string)


-boris


>
> I also just realised (long after everyone else, apparently) that this
> function returns (+ve) Exxx values, not -1 and setting errno.
>
> Together with combining getpwnam and getpwnam_r in the same doc it's almost
> like someone was wilfully trying to make this function difficult to figure
> out...
>
>>>    ERRORS
>>>        0 or ENOENT or ESRCH or EBADF or EPERM or ...
>>>              The given name or uid was not found.
>>>
>>> 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. Both set *result to NULL.
>> I don't know where all that stuff about ENOENT comes from.  I'm
>> tempted to say this is a bug in your C library.  But I don't mind
>> treating ENOENT or ESRCH as ERROR_NOTFOUND.
>>
>> I do mind treating EBADF or EPERM that way.
> FWIW the Linux manpage which Boris has referred to says under NOTES:
>
>      Experiments on various UNIX-like systems show that lots  of
>      different values occur in this situation: 0, ENOENT, EBADF, ESRCH,
>      EWOULDBLOCK, EPERM, and probably others
>
> "various" probably includes all sorts of random UNIX-like systems outside
> the small set which are actually used with Xen. I'd be inclined to agree
> with not including EBADF, EPERM in this way at least until an actual
> platform running Xen is found which returns such errors (not suggesting
> anyone proactively checking, but just that we should be prepared to accept
> the possibility when someone notices and reports it).
>
> Ian.
>
> [0] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getpwnam.html
>
>>> While at it, also make sure we don't get stuck on ERANGE.
>> If you are going to do anything to this, you should use an
>> exponentially increasing buffer size.  Bear in mind that getpwnam_r is
>> already inside the trust boundary.
>>
>> Thanks,
>> Ian.

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

[Ian Campbell]

On Mon, 2015-11-30 at 13:24 -0500, Boris Ostrovsky wrote:
>      root@orochi-c> cat /etc/redhat-release
>      Fedora release 20 (Heisenbug)
>      root@orochi-c> ./a.out foobar
>      s = 2 errno = 2 result = (nil)
>      getpwnam_r: No such file or directory
>      root@orochi-c>
> 
> which does look like a bug in F20 since the code above is taken from man 
> pages' EXAMPLE and doesn't look like it works as intended (which was to 
> produce "Not found" string)

https://bugzilla.redhat.com/show_bug.cgi?id=988068 seems related (although
is blaming "sssd" rather than glibc it seems).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 12/01/2015 09:38 AM, Ian Campbell wrote:
> On Mon, 2015-11-30 at 13:24 -0500, Boris Ostrovsky wrote:
>>       root@orochi-c> cat /etc/redhat-release
>>       Fedora release 20 (Heisenbug)
>>       root@orochi-c> ./a.out foobar
>>       s = 2 errno = 2 result = (nil)
>>       getpwnam_r: No such file or directory
>>       root@orochi-c>
>>
>> which does look like a bug in F20 since the code above is taken from man
>> pages' EXAMPLE and doesn't look like it works as intended (which was to
>> produce "Not found" string)
> https://bugzilla.redhat.com/show_bug.cgi?id=988068 seems related (although
> is blaming "sssd" rather than glibc it seems).

This too:

https://sourceware.org/bugzilla/show_bug.cgi?id=17922

They claim that glibc v2.20 fails (i.e. returns non-zero/ENOENT) but in 
my case on fedora 21 with 2.20-8 I get 0. My "failing" fedora 20 uses 2.18.

I don't know whether it's really a bug in glibc since Linux man pages do 
allow ENOENT (and some other codes) to indicate absence of entry. (But 
then if this is not a bug in glibc then there is a bug in man pages' 
example code).

In any case, I think we need to account for this in libxl.

-boris

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

[Ian Campbell]

On Tue, 2015-12-01 at 10:32 -0500, Boris Ostrovsky wrote:
> On 12/01/2015 09:38 AM, Ian Campbell wrote:
> > On Mon, 2015-11-30 at 13:24 -0500, Boris Ostrovsky wrote:
> > >       root@orochi-c> cat /etc/redhat-release
> > >       Fedora release 20 (Heisenbug)
> > >       root@orochi-c> ./a.out foobar
> > >       s = 2 errno = 2 result = (nil)
> > >       getpwnam_r: No such file or directory
> > >       root@orochi-c>
> > > 
> > > which does look like a bug in F20 since the code above is taken from
> > > man
> > > pages' EXAMPLE and doesn't look like it works as intended (which was
> > > to
> > > produce "Not found" string)
> > https://bugzilla.redhat.com/show_bug.cgi?id=988068 seems related
> > (although
> > is blaming "sssd" rather than glibc it seems).
> 
> This too:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=17922
> 
> They claim that glibc v2.20 fails (i.e. returns non-zero/ENOENT) but in 
> my case on fedora 21 with 2.20-8 I get 0. My "failing" fedora 20 uses
> 2.18.
> 
> I don't know whether it's really a bug in glibc since Linux man pages do 
> allow ENOENT (and some other codes) to indicate absence of entry. (But 
> then if this is not a bug in glibc then there is a bug in man pages' 
> example code).
> 
> In any case, I think we need to account for this in libxl.

I'm not sure we do for an apparently transient issue in a fast moving
"bleeding edge" type distro, for a version of which is already (long, by
their standards) out of support.

Especially given the implication in the bug that it can be worked around by
frobbing with /etc/nsswitch.conf and the fact the distro is treating it as
a bug rather than as by design.

Lastly, even the Linux man pages only, to my reading, seem to countenance
ENOENT etc for getpwnam, not for getpwnam_r, the latter does have a well
defined indication of not found vs error.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel