| Message ID | 20200811212318.708290-1-ckuehl@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] block: Raise an error when backing file parameter is an empty string | expand |
Am 11.08.2020 um 23:23 hat Connor Kuehl geschrieben: > Providing an empty string for the backing file parameter like so: > > qemu-img create -f qcow2 -b '' /tmp/foo > > allows the flow of control to reach and subsequently fail an assert > statement because passing an empty string to > > bdrv_get_full_backing_filename_from_filename() > > simply results in NULL being returned without an error being raised. > > To fix this, let's check for an empty string when getting the value from > the opts list. > > Reported-by: Attila Fazekas <afazekas@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <ckuehl@redhat.com> In 'qemu-img rebase' we accept an empty string to mean "no backing file". I guess it would be a bit more consistent to do the same here, though of course there is no real reason to use -b '' in 'create'. So I don't really mind an error either, I just wanted to mention it. > v2: > - Removed 4 spaces to resolve pylint warning > - Updated format to be 'iotests.imgfmt' instead > of hardcoding 'qcow2' > - Use temporary file instead of '/tmp/foo' > - Give a size parameter to qemu-img > - Run test for qcow2, qcow, qed and *not* raw > > block.c | 4 ++++ > tests/qemu-iotests/298 | 49 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/298.out | 5 ++++ We have multiple series using 298. You were first, though, so in case of doubt, the others will have to rebase. > tests/qemu-iotests/group | 1 + > 4 files changed, 59 insertions(+) > create mode 100755 tests/qemu-iotests/298 > create mode 100644 tests/qemu-iotests/298.out > > diff --git a/block.c b/block.c > index d9ac0e07eb..1f72275b87 100644 > --- a/block.c > +++ b/block.c > @@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char *fmt, > "same filename as the backing file"); > goto out; > } > + if (backing_file[0] == '\0') { > + error_setg(errp, "Expected backing file name, got empty string"); > + goto out; > + } > } > > backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT); > diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298 > new file mode 100755 > index 0000000000..879dae2d8e > --- /dev/null > +++ b/tests/qemu-iotests/298 > @@ -0,0 +1,49 @@ > +#!/usr/bin/env python3 > +# > +# Copyright (C) 2020 Red Hat, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > + > + > +# Regression test for avoiding an assertion when the 'backing file' > +# parameter (-b) is set to an empty string for qemu-img create > +# > +# qemu-img create -f qcow2 -b '' /tmp/foo > +# > +# This ensures the invalid parameter is handled with a user- > +# friendly message instead of a failed assertion. > + > +import iotests > + > +class TestEmptyBackingFilename(iotests.QMPTestCase): > + > + > + def test_empty_backing_file_name(self): > + with iotests.FilePath('test.img') as img_path: > + actual = iotests.qemu_img_pipe( > + 'create', > + '-f', iotests.imgfmt, > + '-b', '', > + img_path, > + '1M' > + ) > + expected = f'qemu-img: {img_path}: Expected backing file name,' \ > + ' got empty string' > + > + self.assertEqual(actual.strip(), expected.strip()) > + > + > +if __name__ == '__main__': > + iotests.main(supported_fmts=['qed', 'qcow', 'qcow2']) This looks like a test case that would be better served by not using QMPTestCase, but just printing the qemu-img output and having the message compared against the reference output. In fact, there is already 049 for testing some qemu-img create options and we could just add a line there (or multiple lines to cover other backing file related error cases, too). Putting it there would both simplify the test code and keep 298 free for the other series. > diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out > new file mode 100644 > index 0000000000..ae1213e6f8 > --- /dev/null > +++ b/tests/qemu-iotests/298.out > @@ -0,0 +1,5 @@ > +. > +---------------------------------------------------------------------- > +Ran 1 tests > + > +OK > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index 025ed5238d..6f80c884a1 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -306,6 +306,7 @@ > 295 rw > 296 rw > 297 meta > +298 img auto quick > 299 auto quick > 301 backing quick > 302 quick None of the above is really a reason to reject the patch. I guess this is more of a "are you sure? (y/n)" before I apply it. :-) Kevin
On 8/12/20 1:58 AM, Kevin Wolf wrote: > This looks like a test case that would be better served by not using > QMPTestCase, but just printing the qemu-img output and having the > message compared against the reference output. > > In fact, there is already 049 for testing some qemu-img create options > and we could just add a line there (or multiple lines to cover other > backing file related error cases, too). > > Putting it there would both simplify the test code and keep 298 free for > the other series. > > None of the above is really a reason to reject the patch. I guess this > is more of a "are you sure? (y/n)" before I apply it. :-) Hi Kevin! Thanks for the review :-) I think it'd be best for my own edification to address your comments here instead of applying this now. I'll send a v3. Connor > > Kevin >
diff --git a/block.c b/block.c index d9ac0e07eb..1f72275b87 100644 --- a/block.c +++ b/block.c @@ -6117,6 +6117,10 @@ void bdrv_img_create(const char *filename, const char *fmt, "same filename as the backing file"); goto out; } + if (backing_file[0] == '\0') { + error_setg(errp, "Expected backing file name, got empty string"); + goto out; + } } backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT); diff --git a/tests/qemu-iotests/298 b/tests/qemu-iotests/298 new file mode 100755 index 0000000000..879dae2d8e --- /dev/null +++ b/tests/qemu-iotests/298 @@ -0,0 +1,49 @@ +#!/usr/bin/env python3 +# +# Copyright (C) 2020 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + + + +# Regression test for avoiding an assertion when the 'backing file' +# parameter (-b) is set to an empty string for qemu-img create +# +# qemu-img create -f qcow2 -b '' /tmp/foo +# +# This ensures the invalid parameter is handled with a user- +# friendly message instead of a failed assertion. + +import iotests + +class TestEmptyBackingFilename(iotests.QMPTestCase): + + + def test_empty_backing_file_name(self): + with iotests.FilePath('test.img') as img_path: + actual = iotests.qemu_img_pipe( + 'create', + '-f', iotests.imgfmt, + '-b', '', + img_path, + '1M' + ) + expected = f'qemu-img: {img_path}: Expected backing file name,' \ + ' got empty string' + + self.assertEqual(actual.strip(), expected.strip()) + + +if __name__ == '__main__': + iotests.main(supported_fmts=['qed', 'qcow', 'qcow2']) diff --git a/tests/qemu-iotests/298.out b/tests/qemu-iotests/298.out new file mode 100644 index 0000000000..ae1213e6f8 --- /dev/null +++ b/tests/qemu-iotests/298.out @@ -0,0 +1,5 @@ +. +---------------------------------------------------------------------- +Ran 1 tests + +OK diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 025ed5238d..6f80c884a1 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -306,6 +306,7 @@ 295 rw 296 rw 297 meta +298 img auto quick 299 auto quick 301 backing quick 302 quick
Providing an empty string for the backing file parameter like so: qemu-img create -f qcow2 -b '' /tmp/foo allows the flow of control to reach and subsequently fail an assert statement because passing an empty string to bdrv_get_full_backing_filename_from_filename() simply results in NULL being returned without an error being raised. To fix this, let's check for an empty string when getting the value from the opts list. Reported-by: Attila Fazekas <afazekas@redhat.com> Fixes: https://bugzilla.redhat.com/1809553 Signed-off-by: Connor Kuehl <ckuehl@redhat.com> --- v2: - Removed 4 spaces to resolve pylint warning - Updated format to be 'iotests.imgfmt' instead of hardcoding 'qcow2' - Use temporary file instead of '/tmp/foo' - Give a size parameter to qemu-img - Run test for qcow2, qcow, qed and *not* raw block.c | 4 ++++ tests/qemu-iotests/298 | 49 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/298.out | 5 ++++ tests/qemu-iotests/group | 1 + 4 files changed, 59 insertions(+) create mode 100755 tests/qemu-iotests/298 create mode 100644 tests/qemu-iotests/298.out