| Message ID | 1490621195-2228-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 27.03.2017 15:26, 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) > $ 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 <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > block/rbd.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 27.03.2017 18:10, Max Reitz wrote: > On 27.03.2017 15:26, 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) >> $ 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 <armbru@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> block/rbd.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> That said, don't we have a similar issue with qemu_rbd_create()? It too doesn't check whether those options are given but I guess they're just as mandatory. Max
Max Reitz <mreitz@redhat.com> writes: > On 27.03.2017 18:10, Max Reitz wrote: >> On 27.03.2017 15:26, 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) >>> $ 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 <armbru@redhat.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> --- >>> block/rbd.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> > > That said, don't we have a similar issue with qemu_rbd_create()? It too > doesn't check whether those options are given but I guess they're just > as mandatory. Looks like it. I'll try to stick a fix into v4. Thanks!
Markus Armbruster <armbru@redhat.com> writes: > Max Reitz <mreitz@redhat.com> writes: > >> On 27.03.2017 18:10, Max Reitz wrote: >>> On 27.03.2017 15:26, 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) >>>> $ 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 <armbru@redhat.com> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>>> --- >>>> block/rbd.c | 10 +++++++--- >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too >> doesn't check whether those options are given but I guess they're just >> as mandatory. > > Looks like it. I'll try to stick a fix into v4. Hmm, ignorant question: how can I reach qemu_rbd_create() without going through qemu_rbd_parse_filename()?
On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Max Reitz <mreitz@redhat.com> writes: > > > >> On 27.03.2017 18:10, Max Reitz wrote: > >>> On 27.03.2017 15:26, 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) > >>>> $ 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 <armbru@redhat.com> > >>>> Reviewed-by: Eric Blake <eblake@redhat.com> > >>>> --- > >>>> block/rbd.c | 10 +++++++--- > >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > >>> > >>> Reviewed-by: Max Reitz <mreitz@redhat.com> > >> > >> That said, don't we have a similar issue with qemu_rbd_create()? It too > >> doesn't check whether those options are given but I guess they're just > >> as mandatory. > > > > Looks like it. I'll try to stick a fix into v4. > > Hmm, ignorant question: how can I reach qemu_rbd_create() without going > through qemu_rbd_parse_filename()? You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call qemu_rbd_parse_filename(). And in qemu_rbd_parse_filename(), it will complain if pool is not provided (and that is what causes the abort, not the missing image parameter). So I think we are safe, but a nicer error message for a missing 'image' parameter might be nice anyway.
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 <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> With an updated commit message: Reviewed-by: Jeff Cody <jcody@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 >
Jeff Cody <jcody@redhat.com> writes: > 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. Turns out the crash I observed was an artifact of my testing instrumentation. > 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 <armbru@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > With an updated commit message: > > Reviewed-by: Jeff Cody <jcody@redhat.com> Thanks!
Jeff Cody <jcody@redhat.com> writes: > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote: >> Markus Armbruster <armbru@redhat.com> writes: >> >> > Max Reitz <mreitz@redhat.com> writes: >> > >> >> On 27.03.2017 18:10, Max Reitz wrote: >> >>> On 27.03.2017 15:26, 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) >> >>>> $ 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 <armbru@redhat.com> >> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >>>> --- >> >>>> block/rbd.c | 10 +++++++--- >> >>>> 1 file changed, 7 insertions(+), 3 deletions(-) >> >>> >> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too >> >> doesn't check whether those options are given but I guess they're just >> >> as mandatory. >> > >> > Looks like it. I'll try to stick a fix into v4. >> >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going >> through qemu_rbd_parse_filename()? > > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call > qemu_rbd_parse_filename(). And in qemu_rbd_parse_filename(), it will > complain if pool is not provided (and that is what causes the abort, not the > missing image parameter). So I think we are safe, but a nicer error message > for a missing 'image' parameter might be nice anyway. I now see the "we are safe" part, but not the "want a nicer error message for a missing 'image'" part. How can 'image' be missing? qemu_rbd_parse_filename() parses a pseudo-filename of the form rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...] It fails if * the pseudo-filename doesn't start with "rbd:" * doesn't contain '/' ("Pool name is required") * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or too long (until the next commit) * a KEY=VALUE doesn't contain '=' If it succeeds, "pool" and "image" are both set in @options. Can you give me a reproducer for the error message you'd like me to improve?
On Tue, Mar 28, 2017 at 09:54:53AM +0200, Markus Armbruster wrote: > Jeff Cody <jcody@redhat.com> writes: > > > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote: > >> Markus Armbruster <armbru@redhat.com> writes: > >> > >> > Max Reitz <mreitz@redhat.com> writes: > >> > > >> >> On 27.03.2017 18:10, Max Reitz wrote: > >> >>> On 27.03.2017 15:26, 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) > >> >>>> $ 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 <armbru@redhat.com> > >> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> > >> >>>> --- > >> >>>> block/rbd.c | 10 +++++++--- > >> >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > >> >>> > >> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> > >> >> > >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too > >> >> doesn't check whether those options are given but I guess they're just > >> >> as mandatory. > >> > > >> > Looks like it. I'll try to stick a fix into v4. > >> > >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going > >> through qemu_rbd_parse_filename()? > > > > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call > > qemu_rbd_parse_filename(). And in qemu_rbd_parse_filename(), it will > > complain if pool is not provided (and that is what causes the abort, not the > > missing image parameter). So I think we are safe, but a nicer error message > > for a missing 'image' parameter might be nice anyway. > > I now see the "we are safe" part, but not the "want a nicer error > message for a missing 'image'" part. How can 'image' be missing? > > qemu_rbd_parse_filename() parses a pseudo-filename of the form > > rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...] > > It fails if > > * the pseudo-filename doesn't start with "rbd:" > > * doesn't contain '/' ("Pool name is required") > > * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or > too long (until the next commit) > > * a KEY=VALUE doesn't contain '=' > > If it succeeds, "pool" and "image" are both set in @options. > > Can you give me a reproducer for the error message you'd like me to > improve? Sure: qemu-img info rbd:mypool/:mon_host=192.168.15.180 'image' is technically present in the options, but it is an empty string. My thought was that it'd be nice to throw a similar error message for an empty string for 'image'. As it is, the rbd library catches it, so it isn't catastrophic, but a nicer message would be helpful. (My comment here assumes an empty string is not an acceptable image name for rbd)
On Tue, Mar 28, 2017 at 07:56:06AM -0400, Jeff Cody wrote: > On Tue, Mar 28, 2017 at 09:54:53AM +0200, Markus Armbruster wrote: > > Jeff Cody <jcody@redhat.com> writes: > > > > > On Mon, Mar 27, 2017 at 08:58:28PM +0200, Markus Armbruster wrote: > > >> Markus Armbruster <armbru@redhat.com> writes: > > >> > > >> > Max Reitz <mreitz@redhat.com> writes: > > >> > > > >> >> On 27.03.2017 18:10, Max Reitz wrote: > > >> >>> On 27.03.2017 15:26, 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) > > >> >>>> $ 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 <armbru@redhat.com> > > >> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> > > >> >>>> --- > > >> >>>> block/rbd.c | 10 +++++++--- > > >> >>>> 1 file changed, 7 insertions(+), 3 deletions(-) > > >> >>> > > >> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> > > >> >> > > >> >> That said, don't we have a similar issue with qemu_rbd_create()? It too > > >> >> doesn't check whether those options are given but I guess they're just > > >> >> as mandatory. > > >> > > > >> > Looks like it. I'll try to stick a fix into v4. > > >> > > >> Hmm, ignorant question: how can I reach qemu_rbd_create() without going > > >> through qemu_rbd_parse_filename()? > > > > > > You can't -- commit c7cacb3e7 forces qemu_rbd_create() to call > > > qemu_rbd_parse_filename(). And in qemu_rbd_parse_filename(), it will > > > complain if pool is not provided (and that is what causes the abort, not the > > > missing image parameter). So I think we are safe, but a nicer error message > > > for a missing 'image' parameter might be nice anyway. > > > > I now see the "we are safe" part, but not the "want a nicer error > > message for a missing 'image'" part. How can 'image' be missing? > > > > qemu_rbd_parse_filename() parses a pseudo-filename of the form > > > > rbd:POOL/IMAGE[@SNAP][:KEY=VALUE:...] > > > > It fails if > > > > * the pseudo-filename doesn't start with "rbd:" > > > > * doesn't contain '/' ("Pool name is required") > > > > * POOL, IMAGE, SNAP, the KEY=VALUE:... part or any KEY in it is empty or > > too long (until the next commit) > > > > * a KEY=VALUE doesn't contain '=' > > > > If it succeeds, "pool" and "image" are both set in @options. > > > > Can you give me a reproducer for the error message you'd like me to > > improve? > > > Sure: qemu-img info rbd:mypool/:mon_host=192.168.15.180 > > 'image' is technically present in the options, but it is an empty string. > > My thought was that it'd be nice to throw a similar error message for an > empty string for 'image'. As it is, the rbd library catches it, so it isn't > catastrophic, but a nicer message would be helpful. > > (My comment here assumes an empty string is not an acceptable image name for > rbd) I'm not concerned enough by this to want a v5. I wasn't clear about that. -Jeff
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);
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) $ 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 <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/rbd.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)