On 17.06.20 20:27, Sorry :/
Connor Kuehl wrote: > 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 <afaze...@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <cku...@redhat.com> > --- > block.c | 4 ++++ > tests/qemu-iotests/298 | 47 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/298.out | 5 ++++ > tests/qemu-iotests/group | 1 + > 4 files changed, 57 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 6dbcb7e083..b335d6bcb2 100644 > --- a/block.c > +++ b/block.c > @@ -6116,6 +6116,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; > + } Looks good. > } > > 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..1e30caebec > --- /dev/null > +++ b/tests/qemu-iotests/298 > @@ -0,0 +1,47 @@ > +#!/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): > + actual = iotests.qemu_img_pipe( (pylint complains about the indentation that follows, it says it’d prefer four spaces less. Make of that want you wish, AFAIA there’s no requirement to get a clean pylint pass for iotests yet. O:)) > + 'create', > + '-f', 'qcow2', This should be iotests.imgfmt, not specifically qcow2. > + '-b', '', > + '/tmp/foo' I’m not sure I like using /tmp/foo very much, for two reasons: First, this expects that the backing filename will be checked before something really happens with the image filename; for example on Windows, /tmp/foo is just not a valid filename, so we might end up getting an error because of that. Or, for me, I like saving mails in a directory called /tmp/foo, so maybe at some point I get an error because /tmp/foo already exists as a directory. Second, if we ever do run into a regression or some configuration where this test unexpectedly manages to create the image, it would overwrite /tmp/foo. Which maybe isn’t good, because maybe it’s a file the user wants to keep around. The fix is simple, just use some file under iotests.test_dir. I think the easiest way right now is something like: with iotests.FilePath('test.img') as img_path: actual = iotests.qemu_img_pipe('create', ..., img_path, '1M') expected = f'qemu-img: {img_path}: Expected...' self.assertEqual(...) (On that note, I’d give a size to qemu-img, so that the invocation is correct apart from the missing backing file name.) > + ) > + expected = 'qemu-img: /tmp/foo: Expected backing file name,' \ > + ' got empty string' > + > + self.assertEqual(actual.strip(), expected.strip()) > + > + > +if __name__ == '__main__': > + iotests.main(supported_fmts=['raw', 'qcow2']) raw isn’t supported, this test only works for formats that support backing files. (Which also means it should work for qed and qcow, I imagine...?) Max
signature.asc
Description: OpenPGP digital signature