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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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: signature.asc
Description: This is a digitally signed message part

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