[Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Kevin Wolf]

Block drivers that don't support creating images don't have a size option. Fail
gracefully instead of segfaulting when trying to access the option's value.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index ebd1501..8df2a5f 100644
--- a/block.c
+++ b/block.c
@@ -2900,7 +2900,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
                     char *options, uint64_t img_size, int flags)
 {
     QEMUOptionParameter *param = NULL, *create_options = NULL;
-    QEMUOptionParameter *backing_fmt, *backing_file;
+    QEMUOptionParameter *backing_fmt, *backing_file, *size;
     BlockDriverState *bs = NULL;
     BlockDriver *drv, *proto_drv;
     BlockDriver *backing_drv = NULL;
@@ -2983,7 +2983,8 @@ int bdrv_img_create(const char *filename, const char *fmt,
 
     // The size for the image must always be specified, with one exception:
     // If we are using a backing file, we can obtain the size from there
-    if (get_option_parameter(param, BLOCK_OPT_SIZE)->value.n == -1) {
+    size = get_option_parameter(param, BLOCK_OPT_SIZE);
+    if (size && size->value.n == -1) {
         if (backing_file && backing_file->value.s) {
             uint64_t size;
             char buf[32];
-- 
1.7.5.2

Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Stefan Hajnoczi]

On Wed, Jun 1, 2011 at 1:05 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Block drivers that don't support creating images don't have a size option. Fail
> gracefully instead of segfaulting when trying to access the option's value.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)

A command-line to reproduce the crash would be nice.

I noticed this line above your fix:
set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);

If I follow correctly there should be an "Unknown option 'size'" error
message before set_option_parameter_int() returns -1 (which we ignore)
and then crash.

Perhaps we should just catch the error when set_option_parameter_int() fails?

Stefan

Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Kevin Wolf]

Am 02.06.2011 00:34, schrieb Stefan Hajnoczi:
> On Wed, Jun 1, 2011 at 1:05 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Block drivers that don't support creating images don't have a size option. Fail
>> gracefully instead of segfaulting when trying to access the option's value.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> A command-line to reproduce the crash would be nice.

qemu-img create -f bochs nbd:foo 32M

It doesn't happen with a file protocol any more since we merge the
create options of the protocol with those of the format (was introduced
with Sheepdog).

> I noticed this line above your fix:
> set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> 
> If I follow correctly there should be an "Unknown option 'size'" error
> message before set_option_parameter_int() returns -1 (which we ignore)
> and then crash.

Right, this is what happens.

> Perhaps we should just catch the error when set_option_parameter_int() fails?

We could do that, but the segfault isn't really related to a failing
set_option_parameter_int() but to the failing get_option_parameter(). I
think it's good style not to rely on the error handling of an unrelated
action some lines above.

Adding some error handling there, too, wouldn't hurt, though.

Kevin

Re: [Qemu-devel] [PATCH] bdrv_img_create: Fix segfault

[Stefan Hajnoczi]

On Mon, Jun 6, 2011 at 11:00 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 02.06.2011 00:34, schrieb Stefan Hajnoczi:
>> On Wed, Jun 1, 2011 at 1:05 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Block drivers that don't support creating images don't have a size option. Fail
>>> gracefully instead of segfaulting when trying to access the option's value.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> A command-line to reproduce the crash would be nice.
>
> qemu-img create -f bochs nbd:foo 32M
>
> It doesn't happen with a file protocol any more since we merge the
> create options of the protocol with those of the format (was introduced
> with Sheepdog).

Interesting, I was just looking at the format + protocol
create_options merging because I tried to replace QEMUOptionParameter
with QemuOpts.  The idea was to make the block layer use QemuOpts and
then introduce a BlockDriver open_options QemuOptsList so that image
format parameters like image streaming or copy-on-read can be
specified on launch.

Here is where I got to:

http://repo.or.cz/w/qemu/stefanha.git/commitdiff/b49babb2c8b476a36357cfd7276ca45a11039ca5

The main thing stopping me from dropping QEMUOptionParameter
completely is this merging behavior.  Any suggestions?

>> I noticed this line above your fix:
>> set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
>>
>> If I follow correctly there should be an "Unknown option 'size'" error
>> message before set_option_parameter_int() returns -1 (which we ignore)
>> and then crash.
>
> Right, this is what happens.
>
>> Perhaps we should just catch the error when set_option_parameter_int() fails?
>
> We could do that, but the segfault isn't really related to a failing
> set_option_parameter_int() but to the failing get_option_parameter(). I
> think it's good style not to rely on the error handling of an unrelated
> action some lines above.

It's also bad to keep executing code after something has broken, which
is the case when set_option_parameter_int() fails but we ignore the
return value.

Thanks for explaining, I now understand your fix better and am happy with it.

Stefan