[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.