| Message ID | 20200617182725.951119-1-ckuehl@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Raise an error when backing file parameter is an empty string | expand |
Patchew URL: https://patchew.org/QEMU/20200617182725.951119-1-ckuehl@redhat.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC qga/commands.o
CC qga/guest-agent-command-state.o
CC qga/main.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
CC qga/commands-posix.o
CC qga/channel-posix.o
CC qga/qapi-generated/qga-qapi-types.o
---
CC qemu-img.o
AR libvhost-user.a
GEN docs/interop/qemu-ga-ref.html
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN docs/interop/qemu-ga-ref.txt
GEN docs/interop/qemu-ga-ref.7
LINK qemu-keymap
LINK ivshmem-client
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK ivshmem-server
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-nbd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-storage-daemon
AS pc-bios/optionrom/multiboot.o
AS pc-bios/optionrom/linuxboot.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-img
CC pc-bios/optionrom/linuxboot_dma.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
AS pc-bios/optionrom/kvmvapic.o
AS pc-bios/optionrom/pvh.o
LINK qemu-io
CC pc-bios/optionrom/pvh_main.o
BUILD pc-bios/optionrom/multiboot.img
LINK qemu-edid
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
BUILD pc-bios/optionrom/linuxboot.img
LINK fsdev/virtfs-proxy-helper
BUILD pc-bios/optionrom/linuxboot_dma.img
BUILD pc-bios/optionrom/kvmvapic.img
BUILD pc-bios/optionrom/multiboot.raw
BUILD pc-bios/optionrom/linuxboot.raw
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK scsi/qemu-pr-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
LINK qemu-bridge-helper
BUILD pc-bios/optionrom/linuxboot_dma.raw
LINK virtiofsd
---
BUILD pc-bios/optionrom/pvh.img
SIGN pc-bios/optionrom/linuxboot_dma.bin
SIGN pc-bios/optionrom/kvmvapic.bin
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
BUILD pc-bios/optionrom/pvh.raw
SIGN pc-bios/optionrom/pvh.bin
LINK qemu-ga
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
GEN x86_64-softmmu/config-target.h
GEN x86_64-softmmu/hmp-commands.h
GEN x86_64-softmmu/hmp-commands-info.h
---
CC x86_64-softmmu/accel/tcg/cpu-exec.o
CC x86_64-softmmu/accel/tcg/cpu-exec-common.o
CC x86_64-softmmu/accel/tcg/translate-all.o
/tmp/qemu-test/src/fpu/softfloat.c:3365:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3423:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~ ( ( (uint64_t) ( absZ1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3483:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
absZ0 &= ~(((uint64_t)(absZ1<<1) == 0) & roundNearestEven);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3606:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3760:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig &= ~ ( ( ( roundBits ^ 0x200 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:3987:21: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4003:22: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig0 &= ~ ( ( (uint64_t) ( zSig1<<1 ) == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
/tmp/qemu-test/src/fpu/softfloat.c:4273:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
zSig1 &= ~ ( ( zSig2 + zSig2 == 0 ) & roundNearestEven );
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!
8 errors generated.
/tmp/qemu-test/src/migration/ram.c:919:45: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
xbzrle_counters.encoding_rate = UINT64_MAX;
~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: fpu/softfloat.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1
make: *** [Makefile:527: x86_64-softmmu/all] Error 2
Traceback (most recent call last):
File "./tests/docker/docker.py", line 669, in <module>
sys.exit(main())
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=89bb2fbb1b464949a9cb6e758f2d6cbf', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-rji51d3m/src/docker-src.2020-06-17-14.47.05.16256:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=89bb2fbb1b464949a9cb6e758f2d6cbf
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-rji51d3m/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 3m49.789s
user 0m8.457s
The full log is available at
http://patchew.org/logs/20200617182725.951119-1-ckuehl@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 6/17/20 1:27 PM, 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 <afazekas@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <ckuehl@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 Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed 17 Jun 2020 08:27:25 PM CEST, 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 <afazekas@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <ckuehl@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
Hi Kevin & Max, Just pinging this patch for your consideration. Thank you, Connor On 6/17/20 11:27 AM, 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 <afazekas@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <ckuehl@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; > + } > } > > 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( > + 'create', > + '-f', 'qcow2', > + '-b', '', > + '/tmp/foo' > + ) > + 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']) > 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 d886fa0cb3..4bca2d9e05 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -302,3 +302,4 @@ > 291 rw quick > 292 rw auto quick > 297 meta > +298 img auto quick >
Ping On 7/1/20 3:42 PM, Connor Kuehl wrote: > Hi Kevin & Max, > > Just pinging this patch for your consideration. > > Thank you, > > Connor > > On 6/17/20 11:27 AM, 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 <afazekas@redhat.com> >> Fixes: https://bugzilla.redhat.com/1809553 >> Signed-off-by: Connor Kuehl <ckuehl@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; >> + } >> } >> 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( >> + 'create', >> + '-f', 'qcow2', >> + '-b', '', >> + '/tmp/foo' >> + ) >> + 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']) >> 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 d886fa0cb3..4bca2d9e05 100644 >> --- a/tests/qemu-iotests/group >> +++ b/tests/qemu-iotests/group >> @@ -302,3 +302,4 @@ >> 291 rw quick >> 292 rw auto quick >> 297 meta >> +298 img auto quick >> >
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 <afazekas@redhat.com> > Fixes: https://bugzilla.redhat.com/1809553 > Signed-off-by: Connor Kuehl <ckuehl@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
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; + } } 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( + 'create', + '-f', 'qcow2', + '-b', '', + '/tmp/foo' + ) + 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']) 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 d886fa0cb3..4bca2d9e05 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -302,3 +302,4 @@ 291 rw quick 292 rw auto quick 297 meta +298 img auto 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> --- 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