Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qemu savevm overwrites snapshots when tags used 0 and 1 consecutively #23

Closed
nasastry opened this issue Oct 16, 2017 · 5 comments
Closed

Comments

@nasastry
Copy link

nasastry commented Oct 16, 2017

cde:info Mirrored with LTC bug https://bugzilla.linux.ibm.com/show_bug.cgi?id=160120 </cde:info>

---Problem Description---
While creating snapshots when tag name '0' used in the very first and followed by tag name '1' then snapshot created with tag name '0' getting erased.

---Steps to Reproduce---

  1. Start the guest and connect to the qemu monitor
qemu-system-ppc64 --nographic -vga none -machine pseries,accel=kvm,kvm-type=HV -m 2G,slots=32,maxmem=16G -device virtio-blk-pci,drive=rootdisk -drive file=/home/nasastry/hostos-3.0-ppc64le.qcow2,if=none,cache=none,id=rootdisk,format=qcow2 -net nic,model=virtio -monitor telnet:127.0.0.1:1234,server,nowait

connect to monitor by

# telnet localhost 1234

  1. At the monitor issue the following commands
(qemu) info snapshots
There is no snapshot available.
(qemu) info status
VM status: running
(qemu) savevm 0
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                 VM SIZE                DATE       VM CLOCK
--        0                      338M 2017-10-16 13:44:35   00:02:07.491
(qemu) info status
VM status: running
(qemu) savevm 1
(qemu) info snapshots
List of snapshots present on all disks:
ID        TAG                 VM SIZE                DATE       VM CLOCK
--        1                      340M 2017-10-16 13:45:12   00:02:18.835
(qemu) info status
VM status: running

snapshot created with tag '0' was replaced by tag '1'.
The same overwriting doesn't happen when used '0', '2' consecutively.

# rpm -qf /usr/bin/qemu-system-ppc64
qemu-system-ppc-2.10.0-2.rel.gitc334a4e.el7.centos.ppc64le

@cdeadmin
Copy link

------- Comment From [email protected] 2017-10-16 12:50:49 EDT-------
Even if I agree it is a bit misleading, I don't think this is a bug.

A snapshot is identified by a name computed either from an id, which is basically a
numerical counter starting at 1 for qcow2), or from a tag, which is a string (provided
by the user or automagically computed).

The savevm command is documented in 'info qemu':

?savevm [TAG|ID]?
Create a snapshot of the whole virtual machine. If TAG is
provided, it is used as human readable identifier. If there is
already a snapshot with the same tag or ID, it is replaced. More
info at *note vm_snapshots::.

Back to your reproducer:

> (qemu) savevm 0

This creates a snapshot with tag '0' and id '1'.

> (qemu) savevm 1

This deletes snapshot with name '1' (ie, with id '1') and creates snapshot with tag '1' and id '1'

@cdeadmin
Copy link

------- Comment From [email protected] 2017-10-16 23:38:55 EDT-------
(In reply to comment #2)
> Even if I agree it is a bit misleading, I don't think this is a bug.

I still feel this behavior has to be documented/fixed some where.

> A snapshot is identified by a name computed either from an id, which is
> basically a
> numerical counter starting at 1 for qcow2)

Starting at '1' is in the code? or in some document? I guess code.

> , or from a tag, which is a string
> (provided
> by the user or automagically computed).
>
> The savevm command is documented in 'info qemu':
>
> ?savevm [TAG|ID]?
> Create a snapshot of the whole virtual machine. If TAG is
> provided, it is used as human readable identifier. If there is
> already a snapshot with the same tag or ID, it is replaced. More
> info at *note vm_snapshots::.
>
> Back to your reproducer:
>
> > (qemu) savevm 0
>
> This creates a snapshot with tag '0' and id '1'.

From the output of 'info snapshots' id '1' is not seen for tag '0' instead seeing an empty field.
Please refer to the below output.

ID TAG VM SIZE DATE VM CLOCK
-- 0 338M 2017-10-16 13:44:35 00:02:07.491

If an ID shown as '1' in the above then user can understand the above documented stuff i.e "If there is already a snapshot with the same tag or ID, it is replaced".

>
> > (qemu) savevm 1
>
> This deletes snapshot with name '1' (ie, with id '1') and creates snapshot
> with tag '1' and id '1'

When there is already a snapshot with id '1', why savevm created a new snapshot with the same id '1'? It has to create tag '1' and id '2'.

Opening this bugzilla to consider above points.

  • If the user is not aware of this behavior, he might loose the data saved in the snapshot with tag '0'.

@cdeadmin cdeadmin reopened this Oct 17, 2017
@cdeadmin
Copy link

------- Comment From [email protected] 2017-10-17 01:55:48 EDT-------
(In reply to comment #3)
> (In reply to comment #2)
> > Even if I agree it is a bit misleading, I don't think this is a bug.
>
> I still feel this behavior has to be documented/fixed some where.
>
> > A snapshot is identified by a name computed either from an id, which is
> > basically a
> > numerical counter starting at 1 for qcow2)
>
> Starting at '1' is in the code? or in some document? I guess code.
>
> > , or from a tag, which is a string
> > (provided
> > by the user or automagically computed).
> >
> > The savevm command is documented in 'info qemu':
> >
> > ?savevm [TAG|ID]?
> > Create a snapshot of the whole virtual machine. If TAG is
> > provided, it is used as human readable identifier. If there is
> > already a snapshot with the same tag or ID, it is replaced. More
> > info at *note vm_snapshots::.
> >
> > Back to your reproducer:
> >
> > > (qemu) savevm 0
> >
> > This creates a snapshot with tag '0' and id '1'.
>
> From the output of 'info snapshots' id '1' is not seen for tag '0' instead
> seeing an empty field.
> Please refer to the below output.
>
> ID TAG VM SIZE DATE VM CLOCK
> -- 0 338M 2017-10-16 13:44:35 00:02:07.491
>
> If an ID shown as '1' in the above then user can understand the above
> documented stuff i.e "If there is already a snapshot with the same tag or
> ID, it is replaced".
>

You have a point. The '--' is annoying. I'll have a look.

> >
> > > (qemu) savevm 1
> >
> > This deletes snapshot with name '1' (ie, with id '1') and creates snapshot
> > with tag '1' and id '1'
>
> When there is already a snapshot with id '1', why savevm created a new
> snapshot with the same id '1'? It has to create tag '1' and id '2'.
>

Because "if there us already a snapshot with the same tag or ID, it is replaced"... the only problem is that the id isn't shown.

> Opening this bugzilla to consider above points.
> * If the user is not aware of this behavior, he might loose the data saved
> in the snapshot with tag '0'.

The behavior is documented. This all boils down to 'info snapshots' not showing the id. I'll investigate.

aik pushed a commit that referenced this issue Jan 31, 2018
Direct leak of 160 byte(s) in 4 object(s) allocated from:
    #0 0x55ed7678cda8 in calloc (/home/elmarco/src/qq/build/x86_64-softmmu/qemu-system-x86_64+0x797da8)
    #1 0x7f3f5e725f75 in g_malloc0 /home/elmarco/src/gnome/glib/builddir/../glib/gmem.c:124
    #2 0x55ed778aa3a7 in query_option_descs /home/elmarco/src/qq/util/qemu-config.c:60:16
    #3 0x55ed778aa307 in get_drive_infolist /home/elmarco/src/qq/util/qemu-config.c:140:19
    #4 0x55ed778a9f40 in qmp_query_command_line_options /home/elmarco/src/qq/util/qemu-config.c:254:36
    #5 0x55ed76d4868c in qmp_marshal_query_command_line_options /home/elmarco/src/qq/build/qmp-marshal.c:3078:14
    #6 0x55ed77855dd5 in do_qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:104:5
    #7 0x55ed778558cc in qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:131:11
    #8 0x55ed768b592f in handle_qmp_command /home/elmarco/src/qq/monitor.c:3840:11
    #9 0x55ed7786ccfe in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105:5
    #10 0x55ed778fe37c in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323:13
    #11 0x55ed778fdde6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373:15
    #12 0x55ed7786cd83 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124:12
    #13 0x55ed768b559e in monitor_qmp_read /home/elmarco/src/qq/monitor.c:3882:5
    #14 0x55ed77714f29 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167:9
    #15 0x55ed77714fde in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179:9
    #16 0x55ed7772ffad in tcp_chr_read /home/elmarco/src/qq/chardev/char-socket.c:440:13
    #17 0x55ed7777113b in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84:12
    #18 0x7f3f5e71d90b in g_main_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3182
    #19 0x7f3f5e71e7ac in g_main_context_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3847
    #20 0x55ed77886ffc in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214:9
    #21 0x55ed778865fd in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261:5
    #22 0x55ed77886222 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515:11
    #23 0x55ed76d2a4df in main_loop /home/elmarco/src/qq/vl.c:1995:9
    #24 0x55ed76d1cb4a in main /home/elmarco/src/qq/vl.c:4914:5
    #25 0x7f3f555f6039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <[email protected]>
Reviewed-by: Eric Blake <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
aik pushed a commit that referenced this issue Jan 31, 2018
Spotted thanks to ASAN:

==25226==ERROR: AddressSanitizer: global-buffer-overflow on address 0x556715a1f120 at pc 0x556714b6f6b1 bp 0x7ffcdfac1360 sp 0x7ffcdfac1350
READ of size 1 at 0x556715a1f120 thread T0
    #0 0x556714b6f6b0 in init_disasm /home/elmarco/src/qemu/disas/s390.c:219
    #1 0x556714b6fa6a in print_insn_s390 /home/elmarco/src/qemu/disas/s390.c:294
    #2 0x55671484d031 in monitor_disas /home/elmarco/src/qemu/disas.c:635
    #3 0x556714862ec0 in memory_dump /home/elmarco/src/qemu/monitor.c:1324
    #4 0x55671486342a in hmp_memory_dump /home/elmarco/src/qemu/monitor.c:1418
    #5 0x5567148670be in handle_hmp_command /home/elmarco/src/qemu/monitor.c:3109
    #6 0x5567148674ed in qmp_human_monitor_command /home/elmarco/src/qemu/monitor.c:613
    #7 0x556714b00918 in qmp_marshal_human_monitor_command /home/elmarco/src/qemu/build/qmp-marshal.c:1704
    #8 0x556715138a3e in do_qmp_dispatch /home/elmarco/src/qemu/qapi/qmp-dispatch.c:104
    #9 0x556715138f83 in qmp_dispatch /home/elmarco/src/qemu/qapi/qmp-dispatch.c:131
    #10 0x55671485cf88 in handle_qmp_command /home/elmarco/src/qemu/monitor.c:3839
    #11 0x55671514e80b in json_message_process_token /home/elmarco/src/qemu/qobject/json-streamer.c:105
    #12 0x5567151bf2dc in json_lexer_feed_char /home/elmarco/src/qemu/qobject/json-lexer.c:323
    #13 0x5567151bf827 in json_lexer_feed /home/elmarco/src/qemu/qobject/json-lexer.c:373
    #14 0x55671514ee62 in json_message_parser_feed /home/elmarco/src/qemu/qobject/json-streamer.c:124
    #15 0x556714854b1f in monitor_qmp_read /home/elmarco/src/qemu/monitor.c:3881
    #16 0x556715045440 in qemu_chr_be_write_impl /home/elmarco/src/qemu/chardev/char.c:172
    #17 0x556715047184 in qemu_chr_be_write /home/elmarco/src/qemu/chardev/char.c:184
    #18 0x55671505a8e6 in tcp_chr_read /home/elmarco/src/qemu/chardev/char-socket.c:440
    #19 0x5567150943c3 in qio_channel_fd_source_dispatch /home/elmarco/src/qemu/io/channel-watch.c:84
    #20 0x7fb90292b90b in g_main_dispatch ../glib/gmain.c:3182
    #21 0x7fb90292c7ac in g_main_context_dispatch ../glib/gmain.c:3847
    #22 0x556715162eca in glib_pollfds_poll /home/elmarco/src/qemu/util/main-loop.c:214
    #23 0x556715163001 in os_host_main_loop_wait /home/elmarco/src/qemu/util/main-loop.c:261
    #24 0x5567151631fa in main_loop_wait /home/elmarco/src/qemu/util/main-loop.c:515
    #25 0x556714ad6d3b in main_loop /home/elmarco/src/qemu/vl.c:1950
    #26 0x556714ade329 in main /home/elmarco/src/qemu/vl.c:4865
    #27 0x7fb8fe5c9009 in __libc_start_main (/lib64/libc.so.6+0x21009)
    #28 0x5567147af4d9 in _start (/home/elmarco/src/qemu/build/s390x-softmmu/qemu-system-s390x+0xf674d9)

0x556715a1f120 is located 32 bytes to the left of global variable 'char_hci_type_info' defined in '/home/elmarco/src/qemu/hw/bt/hci-csr.c:493:23' (0x556715a1f140) of size 104
0x556715a1f120 is located 8 bytes to the right of global variable 's390_opcodes' defined in '/home/elmarco/src/qemu/disas/s390.c:860:33' (0x556715a15280) of size 40600

This fix is based on Andreas Arnez <[email protected]> upstream
commit:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=9ace48f3d7d80ce09c5df60cccb433470410b11b

2014-08-19  Andreas Arnez  <[email protected]>

       * s390-dis.c (init_disasm): Simplify initialization of
       opc_index[].  This also fixes an access after the last element
       of s390_opcodes[].

Signed-off-by: Marc-André Lureau <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
@cdeadmin
Copy link

------- Comment From [email protected] 2018-08-22 15:25:15 EDT-------
I've sent a patch set to the ML proposing a fix for this issue:

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg04658.html

In this series I've changed savevm/loadvm/delvm to not interpret the input arg
as an ID, just as a TAG. This change has little/zero impact on the existing
snapshot drivers which either didn't use the ID field at all or, as it is the
case with the QCOW2 driver, the ID is generated internally and never set by the
user anyway. Libvirt isn't affected by this change since it has its own
snapshot mechanism.

@cdeadmin
Copy link

------- Comment From [email protected] 2018-09-14 12:48:21 EDT-------
No longer making plans for future hostos-specific bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants