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

[Ian Campbell <ian.campbell@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.

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@xxxxxxxxxx>
> ---
> Â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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel