* [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
@ 2011-02-11 12:38 Stefano Stabellini
2011-02-11 12:49 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2011-02-11 12:38 UTC (permalink / raw)
To: qemu-devel
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, ':');
if (h != NULL) {
blkdev->fileproto = blkdev->params;
blkdev->filename = h+1;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
2011-02-11 12:38 [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node Stefano Stabellini
@ 2011-02-11 12:49 ` Kevin Wolf
2011-02-11 12:59 ` Stefano Stabellini
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2011-02-11 12:49 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel
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.
> 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?
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
2011-02-11 12:49 ` Kevin Wolf
@ 2011-02-11 12:59 ` Stefano Stabellini
2011-02-11 13:08 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2011-02-11 12:59 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel@nongnu.org, 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.
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
2011-02-11 12:59 ` Stefano Stabellini
@ 2011-02-11 13:08 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2011-02-11 13:08 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: qemu-devel@nongnu.org
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
@ 2011-06-24 14:50 stefano.stabellini
2011-06-24 15:06 ` Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: stefano.stabellini @ 2011-06-24 14:50 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, agraf, Stefano Stabellini
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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>
---
hw/xen_disk.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 096d1c9..801da58 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -616,11 +616,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");
+ if (blkdev->params != NULL)
+ h = strchr(blkdev->params, ':');
h = strchr(blkdev->params, ':');
if (h != NULL) {
blkdev->fileproto = blkdev->params;
@@ -672,11 +674,15 @@ static int blk_init(struct XenDevice *xendev)
/* setup via xenbus -> create new block driver instance */
xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
blkdev->bs = bdrv_new(blkdev->dev);
- if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
- bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
- bdrv_delete(blkdev->bs);
- return -1;
+ if (blkdev->bs) {
+ if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
+ bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
+ bdrv_delete(blkdev->bs);
+ blkdev->bs = NULL;
+ }
}
+ if (!blkdev->bs)
+ return -1;
} else {
/* setup via qemu cmdline -> already setup for us */
xen_be_printf(&blkdev->xendev, 2, "get configured bdrv (cmdline setup)\n");
--
1.7.2.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
2011-06-24 14:50 stefano.stabellini
@ 2011-06-24 15:06 ` Peter Maydell
2011-06-24 16:34 ` Stefano Stabellini
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2011-06-24 15:06 UTC (permalink / raw)
To: stefano.stabellini; +Cc: xen-devel, qemu-devel, agraf
On 24 June 2011 15:50, <stefano.stabellini@eu.citrix.com> wrote:
> /* read xenstore entries */
> if (blkdev->params == NULL) {
> blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> + if (blkdev->params != NULL)
> + h = strchr(blkdev->params, ':');
> h = strchr(blkdev->params, ':');
This adds the if () statement but it looks like you forgot to remove
the strchr that is outside the if(), so this will still segfault...
(Also, coding style demands braces.)
You could also make that "char *h" local to this 'if' block.
> @@ -672,11 +674,15 @@ static int blk_init(struct XenDevice *xendev)
> /* setup via xenbus -> create new block driver instance */
> xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> blkdev->bs = bdrv_new(blkdev->dev);
> - if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> - bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
> - bdrv_delete(blkdev->bs);
> - return -1;
> + if (blkdev->bs) {
> + if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> + bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
> + bdrv_delete(blkdev->bs);
> + blkdev->bs = NULL;
> + }
> }
> + if (!blkdev->bs)
> + return -1;
Doesn't this error return leak the strings allocated by
xenstore_read_be_str() ?
-- PMM
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node
2011-06-24 15:06 ` Peter Maydell
@ 2011-06-24 16:34 ` Stefano Stabellini
0 siblings, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2011-06-24 16:34 UTC (permalink / raw)
To: Peter Maydell
Cc: agraf@suse.de, xen-devel@lists.xensource.com,
qemu-devel@nongnu.org, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]
On Fri, 24 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 15:50, <stefano.stabellini@eu.citrix.com> wrote:
> > /* read xenstore entries */
> > if (blkdev->params == NULL) {
> > blkdev->params = xenstore_read_be_str(&blkdev->xendev, "params");
> > + if (blkdev->params != NULL)
> > + h = strchr(blkdev->params, ':');
> > h = strchr(blkdev->params, ':');
>
> This adds the if () statement but it looks like you forgot to remove
> the strchr that is outside the if(), so this will still segfault...
> (Also, coding style demands braces.)
>
> You could also make that "char *h" local to this 'if' block.
Thank you very much for the review, I'll make the changes.
>
> > @@ -672,11 +674,15 @@ static int blk_init(struct XenDevice *xendev)
> > /* setup via xenbus -> create new block driver instance */
> > xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
> > blkdev->bs = bdrv_new(blkdev->dev);
> > - if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> > - bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
> > - bdrv_delete(blkdev->bs);
> > - return -1;
> > + if (blkdev->bs) {
> > + if (bdrv_open(blkdev->bs, blkdev->filename, qflags,
> > + bdrv_find_whitelisted_format(blkdev->fileproto)) != 0) {
> > + bdrv_delete(blkdev->bs);
> > + blkdev->bs = NULL;
> > + }
> > }
> > + if (!blkdev->bs)
> > + return -1;
>
> Doesn't this error return leak the strings allocated by
> xenstore_read_be_str() ?
Another very good point, I'll introduce an out_error label and free
everything there.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-06-24 16:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-02-11 12:38 [Qemu-devel] [PATCH] xen_disk: cope with missing xenstore "params" node Stefano Stabellini
2011-02-11 12:49 ` Kevin Wolf
2011-02-11 12:59 ` Stefano Stabellini
2011-02-11 13:08 ` Kevin Wolf
-- strict thread matches above, loose matches on Subject: below --
2011-06-24 14:50 stefano.stabellini
2011-06-24 15:06 ` Peter Maydell
2011-06-24 16:34 ` Stefano Stabellini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).