[libvirt] [PATCH] Fix vm define error with back compat console device

[Jim Meyering]

Cole Robinson <crobinso at redhat.com> wrote:
> If you define a domain with serial devs > 0 && parallel devs >= serial
> devs, libvirtd segfaults when trying to set up the back compat console
> device. We were using a previous loop counter where we shouldn't. The
> attached patch fixes this.
>
> Thanks,
> Cole
> diff --git a/src/domain_conf.c b/src/domain_conf.c
> index 8bb51f7..890afe0 100644
> --- a/src/domain_conf.c
> +++ b/src/domain_conf.c
> @@ -3320,8 +3320,9 @@ char *virDomainDefFormat(virConnectPtr conn,
>          if (virDomainChrDefFormat(conn, &buf, def->console, "console") < 0)
>              goto cleanup;
>      } else if (def->nserials != 0) {
> -        /* ..else for legacy compat duplicate the serial device as a console */
> -        if (virDomainChrDefFormat(conn, &buf, def->serials[n], "console") < 0)
> +        /* ..else for legacy compat duplicate the first serial device as a
> +         * console */
> +        if (virDomainChrDefFormat(conn, &buf, def->serials[0], "console") < 0)
>              goto cleanup;
>      }

Hi Cole,

ACK!
This looks like an obviously-right fix.
Thanks for the sample input file you provided,

I confirmed and used it to write a test case that
demonstrates the problem.

With the following test, running this command,

    make check -C tests TESTS=define-dev-segfault VERBOSE=yes

would fail with output including this:

  + libvirtd
  + virsh --connect qemu:///session define devs.xml
  ./define-dev-segfault: line 84:  4882 Segmentation fault  libvirtd > log 2>&1

With your patch applied, the test passes.


>From b16f407a6369cf4c3a5770c4b49515b565f29b4f Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering at redhat.com>
Date: Thu, 15 Jan 2009 19:51:39 +0100
Subject: [PATCH] exercise a bug that could make libvirtd segfault

* tests/define-dev-segfault: New file.
* tests/Makefile.am (test_scripts): Add define-dev-segfault.
---
 tests/Makefile.am         |    1 +
 tests/define-dev-segfault |   92 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 0 deletions(-)
 create mode 100755 tests/define-dev-segfault

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0486ee3..7acb745 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -59,6 +59,7 @@ test_scripts = domainschematest
 if WITH_LIBVIRTD
 test_scripts += \
 	test_conf.sh \
+	define-dev-segfault \
 	cpuset \
 	daemon-conf \
 	int-overflow \
diff --git a/tests/define-dev-segfault b/tests/define-dev-segfault
new file mode 100755
index 0000000..903568f
--- /dev/null
+++ b/tests/define-dev-segfault
@@ -0,0 +1,92 @@
+#!/bin/sh
+# Exercise a bug whereby defining a valid domain could kill libvirtd.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  libvirtd --version
+  virsh --version
+fi
+
+test -z "$srcdir" && srcdir=$(pwd)
+test -z "$abs_top_srcdir" && abs_top_srcdir=$(pwd)/..
+. "$srcdir/test-lib.sh"
+
+fail=0
+
+# Domain definition from Cole Robinson.
+cat <<\EOF > devs.xml || fail=1
+<domain type='kvm'>
+<name>D</name>
+<uuid>aaa3ae22-fed2-bfbd-ac02-3bea3bcfad82</uuid>
+<memory>262144</memory>
+<currentMemory>262144</currentMemory>
+<vcpu>1</vcpu>
+<os>
+<type arch='i686' machine='pc'>hvm</type>
+<boot dev='cdrom'/>
+</os>
+<features>
+<acpi/>
+</features>
+<clock offset='utc'/>
+<on_poweroff>destroy</on_poweroff>
+<on_reboot>restart</on_reboot>
+<on_crash>destroy</on_crash>
+<devices>
+<emulator>/usr/bin/qemu-kvm</emulator>
+<disk type='file' device='cdrom'>
+  <target dev='hdc' bus='ide'/>
+  <readonly/>
+</disk>
+<disk type='file' device='floppy'>
+  <target dev='fdb' bus='fdc'/>
+</disk>
+<disk type='file' device='cdrom'>
+<target dev='sda' bus='scsi'/>
+<readonly/>
+</disk>
+<interface type='network'>
+<mac address='54:52:00:6c:a0:ca'/>
+<source network='aaaaaa'/>
+</interface>
+<interface type='network'>
+<mac address='54:52:00:6c:bb:ca'/>
+<source network='default'/>
+</interface>
+<serial type='pty'/>
+<serial type='pty'/>
+<serial type='pty'/>
+<parallel type='pty'/>
+<parallel type='pty'/>
+<parallel type='pty'/>
+<input type='mouse' bus='ps2'/>
+<sound model='pcspk'/>
+<sound model='es1370'/>
+<hostdev mode='subsystem' type='usb'>
+<source>
+<address bus='3' device='3'/>
+</source>
+</hostdev>
+<hostdev mode='subsystem' type='usb'>
+<source>
+<vendor id='0x0483'/>
+<product id='0x2016'/>
+</source>
+</hostdev>
+</devices>
+</domain>
+EOF
+
+libvirtd > log 2>&1 & pid=$!
+sleep 1
+
+virsh --connect qemu:///session define devs.xml > out 2>&1 \
+  || fail=1
+
+kill $pid
+
+printf 'Domain D defined from devs.xml\n\n' > exp || fail=1
+
+compare exp out || fail=1
+compare /dev/null log || fail=1
+exit $fail
--
1.6.1.233.g5b4a8