[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] libxl: Fix bug in libxl_cdrom_insert, make more robust against bad xenstore data



On Fri, Nov 23, 2012 at 10:05 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

> * Handling non-existent nodes in the backend xenstore entries more
> gracefully

I wonder if it should error out here rather than assume a default -- the
nodes in question should have been written by libxl itself (I think, I
don't think these are backend provided keys) so if they are missing
something has gone pretty wrong in the library, I think it's worth at
least a log message and more than likely an error return.

I would have returned an error, but the fuction returns void.  :-)  That leaves us the options of:
1. Erroring and calling exit() in the middle of the function
2. Just doing a warning
3. Modifying the function to return success / failure

#3 seems like the it shouldn't be too very difficult.  If that sounds good to you I'll re-post this patch with only the bits you've acked, then post another patch with #3.

 -George

 

> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2201,14 +2201,14 @@ static void libxl__device_disk_from_xs_b
>          disk->removable = 0;
>
>      tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/mode", be_path));
> -    if (!strcmp(tmp, "w"))
> +    if (tmp && !strcmp(tmp, "w"))
>          disk->readwrite = 1;
>      else
>          disk->readwrite = 0;
>
>      tmp = libxl__xs_read(gc, XBT_NULL,
>                           libxl__sprintf(gc, "%s/device-type", be_path));
> -    disk->is_cdrom = !strcmp(tmp, "cdrom");
> +    disk->is_cdrom = tmp && !strcmp(tmp, "cdrom");
>
>      disk->format = LIBXL_DISK_FORMAT_UNKNOWN;
>  }
> @@ -2353,6 +2353,7 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
>
>      libxl__device device;
>      const char * path;
> +    char * tmp;
>
>      flexarray_t *insert = NULL;
>
> @@ -2383,8 +2384,11 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
>      disks = libxl_device_disk_list(ctx, domid, &num);
>      for (i = 0; i < num; i++) {
>          if (disks[i].is_cdrom && !strcmp(disk->vdev, disks[i].vdev))
> -            /* found */
> +        {
> +            /* Found.  Set backend type appropriately. */
> +            disk->backend=disks[i].backend;
>              break;
> +        }
>      }
>      if (i == num) {
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual device not found");
> @@ -2410,6 +2414,17 @@ int libxl_cdrom_insert(libxl_ctx *ctx, u
>
>      path = libxl__device_backend_path(gc, &device);
>
> +    /* Sanity check: make sure the backend exists before writing here */
> +    tmp = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/frontend", path));
> +    if (!tmp)
> +    {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Internal error: %s does not exist",
> +            libxl__sprintf(gc, "%s/frontend", path));
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +
>      insert = flexarray_make(gc, 4, 1);
>
>      flexarray_append_pair(insert, "type",
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.