[Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Alberto Garcia]

We just fixed a bug that was causing a use-after-free when QEMU was
unable to create a temporary snapshot. This is a test case for this
scenario.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/051        | 3 +++
 tests/qemu-iotests/051.out    | 3 +++
 tests/qemu-iotests/051.pc.out | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index ee9c820d0f..25d3b2d478 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -354,6 +354,9 @@ printf %b "qemu-io $device_id \"write -P 0x33 0 4k\"\ncommit $device_id\n" |
 
 $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
 
+# Using snapshot=on with a non-existent TMPDIR
+TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index b7273505c7..793af2ab96 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -455,4 +455,7 @@ wrote 4096/4096 bytes at offset 0
 
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Testing: -drive driver=null-co,snapshot=on
+QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
+
 *** done
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index e9257fe318..ca64edae6a 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -527,4 +527,7 @@ wrote 4096/4096 bytes at offset 0
 
 read 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Testing: -drive driver=null-co,snapshot=on
+QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
+
 *** done
-- 
2.11.0

Re: [Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Kevin Wolf]

Am 10.09.2018 um 11:29 hat Alberto Garcia geschrieben:
> We just fixed a bug that was causing a use-after-free when QEMU was
> unable to create a temporary snapshot. This is a test case for this
> scenario.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Hm, it actually doesn't crash for me even without the fix. Anyway, I
don't have a good idea to make it more likely to crash and it's
certainly better than nothing.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Alberto Garcia]

On Mon 10 Sep 2018 11:57:48 AM CEST, Kevin Wolf wrote:
> Am 10.09.2018 um 11:29 hat Alberto Garcia geschrieben:
>> We just fixed a bug that was causing a use-after-free when QEMU was
>> unable to create a temporary snapshot. This is a test case for this
>> scenario.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> Hm, it actually doesn't crash for me even without the fix. Anyway, I
> don't have a good idea to make it more likely to crash and it's
> certainly better than nothing.

Yeah, I had the same problem, I could make it crash very easily last
week, and I can make it crash with the QEMU package shipped with my
distro, but I tried now with master and it doesn't crash. Well, it's
undefined behavior after all :-)

Berto

Re: [Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Eric Blake]

On 9/10/18 4:57 AM, Kevin Wolf wrote:
> Am 10.09.2018 um 11:29 hat Alberto Garcia geschrieben:
>> We just fixed a bug that was causing a use-after-free when QEMU was
>> unable to create a temporary snapshot. This is a test case for this
>> scenario.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
> 
> Hm, it actually doesn't crash for me even without the fix. Anyway, I
> don't have a good idea to make it more likely to crash and it's
> certainly better than nothing.

Does running the test under valgrind reliably see the use-after-free?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] qemu-iotests: Test snapshot=on with nonexistent TMPDIR

[Alberto Garcia]

On Mon 10 Sep 2018 05:55:15 PM CEST, Eric Blake wrote:
>> Hm, it actually doesn't crash for me even without the fix. Anyway, I
>> don't have a good idea to make it more likely to crash and it's
>> certainly better than nothing.
>
> Does running the test under valgrind reliably see the use-after-free?

Good question! :-)

Unfortunately valgrind also needs a valid TMPDIR, so if you change it in
order to reproduce the bug then valgrind won't run.

I don't know if there's a way to tell valgrind to run the specified
program with its own environment variables, but you can simply edit
QEMU's get_tmp_filename() to always return an invalid directory, and
then you get the expected result:

 Invalid read of size 8 
    at 0x859914: qobject_unref_impl (qobject.h:98)
    by 0x85F8EA: bdrv_open_inherit (block.c:2831)
    by 0x85F963: bdrv_open (block.c:2839)
    by 0x8BDD19: blk_new_open (block-backend.c:375)
    by 0x58A88A: blockdev_init (blockdev.c:599)
    by 0x58B6C4: drive_new (blockdev.c:990)
    by 0x59C004: drive_init_func (vl.c:1143)
    by 0x9A0CE3: qemu_opts_foreach (qemu-option.c:1106)
    by 0x5A4692: main (vl.c:4454)
  Address 0x1df67458 is 8 bytes inside a block of size 4,120 free'd
    at 0x4C2CDDB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x97AD5F: qdict_destroy_obj (qdict.c:459)
    by 0x97CC04: qobject_destroy (qobject.c:41)
    by 0x85996F: qobject_unref_impl (qobject.h:100)
    by 0x85F6D4: bdrv_open_inherit (block.c:2794)
    by 0x85F963: bdrv_open (block.c:2839)
      [...]

Berto