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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 7/17/2014 at 11:17 PM, in message
<1405610239.1977.13.camel@xxxxxxxxxxxxxxxxxxxxxx>, Ian Campbell
<Ian.Campbell@xxxxxxxxxx> wrote: 
> 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. 
>  

Thanks. Will update.

>   
> >      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? 
>  
Didn't find problem in other commands, generally can get expected result:
will report error due to some API failed in the command internals.

Considering the possible races, I'll omit the check. Thanks.

- Chunyan

> 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