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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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.