* [PATCH] block: Raise an error when backing file parameter is an empty string
@ 2020-06-17 18:27 Connor Kuehl
2020-06-17 18:50 ` no-reply
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Connor Kuehl @ 2020-06-17 18:27 UTC (permalink / raw)
To: kwolf, mreitz; +Cc: qemu-devel, qemu-block
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
--
2.25.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
@ 2020-06-17 18:50 ` no-reply
2020-06-17 19:11 ` Eric Blake
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: no-reply @ 2020-06-17 18:50 UTC (permalink / raw)
To: ckuehl; +Cc: kwolf, qemu-devel, qemu-block, mreitz
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
2020-06-17 18:50 ` no-reply
@ 2020-06-17 19:11 ` Eric Blake
2020-06-18 10:48 ` Alberto Garcia
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2020-06-17 19:11 UTC (permalink / raw)
To: Connor Kuehl, kwolf, mreitz; +Cc: qemu-devel, qemu-block
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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
2020-06-17 18:50 ` no-reply
2020-06-17 19:11 ` Eric Blake
@ 2020-06-18 10:48 ` Alberto Garcia
2020-07-01 22:42 ` Connor Kuehl
2020-07-13 10:10 ` Max Reitz
4 siblings, 0 replies; 7+ messages in thread
From: Alberto Garcia @ 2020-06-18 10:48 UTC (permalink / raw)
To: Connor Kuehl, kwolf, mreitz; +Cc: qemu-devel, qemu-block
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
` (2 preceding siblings ...)
2020-06-18 10:48 ` Alberto Garcia
@ 2020-07-01 22:42 ` Connor Kuehl
2020-07-10 15:57 ` Connor Kuehl
2020-07-13 10:10 ` Max Reitz
4 siblings, 1 reply; 7+ messages in thread
From: Connor Kuehl @ 2020-07-01 22:42 UTC (permalink / raw)
To: kwolf, mreitz; +Cc: qemu-devel, qemu-block
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-07-01 22:42 ` Connor Kuehl
@ 2020-07-10 15:57 ` Connor Kuehl
0 siblings, 0 replies; 7+ messages in thread
From: Connor Kuehl @ 2020-07-10 15:57 UTC (permalink / raw)
To: kwolf, mreitz; +Cc: qemu-devel, qemu-block
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
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block: Raise an error when backing file parameter is an empty string
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
` (3 preceding siblings ...)
2020-07-01 22:42 ` Connor Kuehl
@ 2020-07-13 10:10 ` Max Reitz
4 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2020-07-13 10:10 UTC (permalink / raw)
To: Connor Kuehl, kwolf; +Cc: qemu-devel, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 5061 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-13 10:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 18:27 [PATCH] block: Raise an error when backing file parameter is an empty string Connor Kuehl
2020-06-17 18:50 ` no-reply
2020-06-17 19:11 ` Eric Blake
2020-06-18 10:48 ` Alberto Garcia
2020-07-01 22:42 ` Connor Kuehl
2020-07-10 15:57 ` Connor Kuehl
2020-07-13 10:10 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).