Re: [Qemu-devel] [PATCH 1/3] block: Add '-blockdev' command line option

[Eric Blake]

On 09/22/2016 10:42 AM, Kevin Wolf wrote:
> This is an option that is directly mapped to the blockdev-add QMP
> command. It works more or less like -drive, except that it doesn't
> create a BlockBackend and doesn't support legacy options.
> 
> This patch adds minimal documentation, the next patches will improve it.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  blockdev.c              | 12 +++++++++++
>  include/sysemu/sysemu.h |  1 +
>  qemu-options.hx         | 12 +++++++++++
>  vl.c                    | 57 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 82 insertions(+)
> 

> +static int blockdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    BlockdevOptions *options;

Uninitialized...

> +    Visitor *v = NULL;
> +    Error *local_err = NULL;
> +
> +    QDict *opts_dict = qemu_opts_to_qdict(opts, NULL);
> +    QObject *crumpled = qdict_crumple(opts_dict, true, &local_err);
> +    if (local_err) {
> +        goto fail;

...can fail without initializing it...

> +    }
> +
> +    v = qobject_string_input_visitor_new(crumpled);
> +    visit_type_BlockdevOptions(v, NULL, &options, &local_err);

This is so deceptively simple! It's taken us months to get to this
point, but I love the end result.  However,

...this initializes options, and may result in malloc'd memory...

> +    if (local_err) {
> +        goto fail;
> +    }
> +    visit_complete(v, opts);
> +
> +    qmp_blockdev_add(options, &local_err);
> +    if (local_err) {
> +        goto fail;
> +    }
> +
> +fail:
> +    QDECREF(opts_dict);
> +    qobject_decref(crumpled);
> +
> +    visit_free(v);
> +
> +    v = qapi_dealloc_visitor_new();
> +    visit_type_BlockdevOptions(v, NULL, &options, NULL);

...this can call the dealloc visitor on uninitialized memory, if we took
the first goto...

> +    visit_free(v);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +    return !!local_err;

...and this leaks options if you did not take the first goto.

You want NULL initialization, and a qapi_free_BlockdevOptions(options)
in here.

> +}
> +
>  static int drive_init_func(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      BlockInterfaceType *block_default_type = opaque;

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature