Re: [Xen-devel] [PATCH] Fix: 'xl vncviewer' accesses port 0 by any invalid domid

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-07-17 at 14:35 +0800, Chunyan Liu wrote:
> Currently, with command:
>   xl vncviewer invalid_domid
> it always brings user to the domU using vncport 5900.
> The invalid domid could be an non-existing one or Dom0.
> It's better to report error in this case.
> 
> This patch corrects two places:
> * libxl_domain_qualifier_to_domid() in tools/libxl/libxl_utils.c
>   blindly converts the input string id to a uint32_t. It doesn't
>   check if the domid is valid or not, so it will depends on later
>   processing to find the error. Correct it with checking the domid
>   validness.
> * libxl_vncviewer_exec: when vncport is NULL, it still continues
>   and will show vncport 5900. So, with 'xl vncviewer 0' it also
>   wrongly shows domU using vncport 5900. Correct it to report error
>   if vncport is NULL.
> 
> Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>
> ---
>  tools/libxl/libxl.c       | 7 ++++++-
>  tools/libxl/libxl_utils.c | 5 +++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index a9205d1..22602e8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1719,8 +1719,13 @@ int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t 
> domid, int autopass)
>      vnc_port = libxl__xs_read(gc, XBT_NULL,
>                              libxl__sprintf(gc,
>                              "/local/domain/%d/console/vnc-port", domid));
> -    if ( vnc_port )
> +    if ( vnc_port ) {
>          port = atoi(vnc_port) - 5900;
> +    } else {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
> +                   "Cannot get vnc-port of domain %d", domid);
> +        goto x_fail;
> +    }


This would be simpler as

        if (!vnc_port) {
            LOG(ERROR, "Cannot... domain %d\n", domid);
            goto x_fail
        }

        port = atoi...

        vnc_listen = libxl_

Rather than putting the error case in an else.

Also note the coding style (no spaces inside the expr of an if ()) and
the use of LOG() instead of LIBXL__LOG.

 
>      vnc_listen = libxl__xs_read(gc, XBT_NULL,
>                                  libxl__sprintf(gc,
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 1fdf5ea..c689de3 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -100,6 +100,11 @@ int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, 
> const char *name,
>          }
>      }
>      *domid = strtoul(name, NULL, 10);
> +
> +    /* this could be an invalid domid */
> +    if (!libxl_domid_to_name(ctx, *domid))
> +        return -1;

We also need to be mindful of the domain disappearing (i.e. being
destroyed) after this check is done (unless there was some locking,
which I don't think there is around this sort of thing)

All that means is that the "later processing" you refer to in the commit
message still needs to cope with this case. Was that code already
correct or are you masking the issue in 99% of the cases with this?

Given the potential for masking races I'm wondering if we might be
better to omit this check and force ourselves to make the rest of the
code robust.

Ian.


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