Patchwork xen_disk: cope with missing xenstore "params" node

login
register
mail settings
Submitter Stefano Stabellini
Date Feb. 11, 2011, 12:59 p.m.
Message ID <alpine.DEB.2.00.1102111250090.2826@kaball-desktop>
Download mbox | patch
Permalink /patch/82753/
State New
Headers show

Comments

Stefano Stabellini - Feb. 11, 2011, 12:59 p.m.
On Fri, 11 Feb 2011, Kevin Wolf wrote:
> Am 11.02.2011 13:38, schrieb Stefano Stabellini:
> > When disk is a cdrom and the drive is empty the "params" node in
> > xenstore might be missing completely: cope with it instead of
> > segfaulting.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > 
> > diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> > index 134ac33..e553c4c 100644
> > --- a/hw/xen_disk.c
> > +++ b/hw/xen_disk.c
> > @@ -577,12 +577,13 @@ static int blk_init(struct XenDevice *xendev)
> >  {
> >      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
> >      int index, qflags, have_barriers, info = 0;
> > -    char *h;
> > +    char *h = NULL;
> >  
> >      /* read xenstore entries */
> >      if (blkdev->params == NULL) {
> >  	blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> > -        h = strchr(blkdev->params, ':');
> > +        if (blkdev->params != NULL)
> > +            h = strchr(blkdev->params, ':');
> 
> The coding style requires braces here.
> 

Good point, I'll do.

> >  	if (h != NULL) {
> >  	    blkdev->fileproto = blkdev->params;
> >  	    blkdev->filename  = h+1;
> 
> Let me add some more context:
> 
>     if (h != NULL) {
>         blkdev->fileproto = blkdev->params;
>         blkdev->filename  = h+1;
>         *h = 0;
>     } else {
>         blkdev->fileproto = "<unset>";
>         blkdev->filename  = blkdev->params;
>     }
> 
> So in the NULL case we now have blkdev->filename = NULL. Doesn't this
> just move the crash a few lines downwards when bdrv_open() tries to use
> NULL as its filename?

There is a check on blkdev->params being NULL few lines after so we just
return.
Maybe an explicit return -1 like in the appended patch here would be
better?
Kevin Wolf - Feb. 11, 2011, 1:08 p.m.
Am 11.02.2011 13:59, schrieb Stefano Stabellini:
> On Fri, 11 Feb 2011, Kevin Wolf wrote:
>> Am 11.02.2011 13:38, schrieb Stefano Stabellini:
>>> When disk is a cdrom and the drive is empty the "params" node in
>>> xenstore might be missing completely: cope with it instead of
>>> segfaulting.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>>
>>> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
>>> index 134ac33..e553c4c 100644
>>> --- a/hw/xen_disk.c
>>> +++ b/hw/xen_disk.c
>>> @@ -577,12 +577,13 @@ static int blk_init(struct XenDevice *xendev)
>>>  {
>>>      struct XenBlkDev *blkdev = container_of(xendev, struct XenBlkDev, xendev);
>>>      int index, qflags, have_barriers, info = 0;
>>> -    char *h;
>>> +    char *h = NULL;
>>>  
>>>      /* read xenstore entries */
>>>      if (blkdev->params == NULL) {
>>>  	blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
>>> -        h = strchr(blkdev->params, ':');
>>> +        if (blkdev->params != NULL)
>>> +            h = strchr(blkdev->params, ':');
>>
>> The coding style requires braces here.
>>
> 
> Good point, I'll do.
> 
>>>  	if (h != NULL) {
>>>  	    blkdev->fileproto = blkdev->params;
>>>  	    blkdev->filename  = h+1;
>>
>> Let me add some more context:
>>
>>     if (h != NULL) {
>>         blkdev->fileproto = blkdev->params;
>>         blkdev->filename  = h+1;
>>         *h = 0;
>>     } else {
>>         blkdev->fileproto = "<unset>";
>>         blkdev->filename  = blkdev->params;
>>     }
>>
>> So in the NULL case we now have blkdev->filename = NULL. Doesn't this
>> just move the crash a few lines downwards when bdrv_open() tries to use
>> NULL as its filename?
> 
> There is a check on blkdev->params being NULL few lines after so we just
> return.

Thanks, I missed that one.

> Maybe an explicit return -1 like in the appended patch here would be
> better?
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 134ac33..fc0de14 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -582,6 +582,9 @@ static int blk_init(struct XenDevice *xendev)
>      /* read xenstore entries */
>      if (blkdev->params == NULL) {
>  	blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> +        if (blkdev->params == NULL) {
> +            return -1;
> +        }
>          h = strchr(blkdev->params, ':');
>  	if (h != NULL) {
>  	    blkdev->fileproto = blkdev->params;

Yes, I think this is more explicit, and therefore easier to read.

Kevin

Patch

diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 134ac33..fc0de14 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -582,6 +582,9 @@  static int blk_init(struct XenDevice *xendev)
     /* read xenstore entries */
     if (blkdev->params == NULL) {
 	blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
+        if (blkdev->params == NULL) {
+            return -1;
+        }
         h = strchr(blkdev->params, ':');
 	if (h != NULL) {
 	    blkdev->fileproto = blkdev->params;