On Mon, Mar 27, 2017 at 03:26:26PM +0200, Markus Armbruster wrote:
> qemu_rbd_open() neglects to check pool and image are present.
> Reproducer:
>
> $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,pool=p
> Segmentation fault (core dumped)
This reproducer is wrong, I think. Omitting the image should be caught
earlier, but it is an error caught by the rbd_open() call.
What doesn't work is omitting the pool name, and that causes an abort()
from rados_ioctx_create(), e.g.:
$ qemu-system-x86_64 -nodefaults -drive
driver=rbd,id=rbd,image=i,server.0.port=6789,server.0.host=192.168.15.180
terminate called after throwing an instance of 'std::logic_error'
what(): basic_string::_M_construct null not valid
Aborted (core dumped)
> $ qemu-system-x86_64 -nodefaults -drive if=none,driver=rbd,image=i
> qemu-system-x86_64: -drive if=none,driver=rbd,image=i: error opening pool
> (null)
>
> Doesn't affect -drive with file=..., because qemu_rbd_parse_filename()
> always sets both pool and image.
>
> Doesn't affect -blockdev, because pool and image are mandatory in the
> QAPI schema.
>
> Fix by adding the missing checks.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
With an updated commit message:
Reviewed-by: Jeff Cody <jc...@redhat.com>
> ---
> block/rbd.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index ee13f3d..5ba2a87 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -711,6 +711,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> *options, int flags,
> name = qemu_opt_get(opts, "image");
> keypairs = qemu_opt_get(opts, "keyvalue-pairs");
>
> + if (!pool || !name) {
> + error_setg(errp, "Parameters 'pool' and 'image' are required");
> + r = -EINVAL;
> + goto failed_opts;
> + }
> +
> r = rados_create(&s->cluster, clientname);
> if (r < 0) {
> error_setg_errno(errp, -r, "error initializing");
> @@ -718,9 +724,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
> *options, int flags,
> }
>
> s->snap = g_strdup(snap);
> - if (name) {
> - pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
> - }
> + pstrcpy(s->name, RBD_MAX_IMAGE_NAME_SIZE, name);
>
> /* try default location when conf=NULL, but ignore failure */
> r = rados_conf_read_file(s->cluster, conf);
> --
> 2.7.4
>
