bdrv_img_create: Fix segfault

Submitted by Kevin Wolf on June 1, 2011, 12:05 p.m.

Details

Message ID 1306929912-15484-1-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf June 1, 2011, 12:05 p.m.
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(-)

Comments

Stefan Hajnoczi June 1, 2011, 10:34 p.m.
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
Kevin Wolf June 6, 2011, 10 a.m.
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
Stefan Hajnoczi June 6, 2011, 11:10 a.m.
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

Patch hide | download patch | download mbox

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];