Make sure pointers are non-NULL before dereferencing them, and
add test suite coverage for the crashers doing so fixes.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1379218

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 src/qemu/qemu_domain.c                             | 35 ++++++++--------
 .../qemuargv2xml-nomachine-aarch64.args            | 11 +++++
 .../qemuargv2xml-nomachine-aarch64.xml             | 39 ++++++++++++++++++
 .../qemuargv2xml-nomachine-ppc64.args              | 11 +++++
 .../qemuargv2xml-nomachine-ppc64.xml               | 47 +++++++++++++++++++++
 .../qemuargv2xml-nomachine-x86_64.args             | 11 +++++
 .../qemuargv2xml-nomachine-x86_64.xml              | 48 ++++++++++++++++++++++
 tests/qemuargv2xmltest.c                           |  4 ++
 8 files changed, 188 insertions(+), 18 deletions(-)
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args
 create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b202d02f9..30ea3e592 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6601,8 +6601,9 @@ qemuDomainIsQ35(const virDomainDef *def)
 bool
 qemuDomainMachineIsQ35(const char *machine)
 {
-    return (STRPREFIX(machine, "pc-q35") ||
-            STREQ(machine, "q35"));
+    return (machine &&
+            (STREQ(machine, "q35") ||
+             STRPREFIX(machine, "pc-q35")));
 }
 
 
@@ -6616,11 +6617,12 @@ qemuDomainIsI440FX(const virDomainDef *def)
 bool
 qemuDomainMachineIsI440FX(const char *machine)
 {
-    return (STREQ(machine, "pc") ||
-            STRPREFIX(machine, "pc-0.") ||
-            STRPREFIX(machine, "pc-1.") ||
-            STRPREFIX(machine, "pc-i440") ||
-            STRPREFIX(machine, "rhel"));
+    return (machine &&
+            (STREQ(machine, "pc") ||
+             STRPREFIX(machine, "pc-0.") ||
+             STRPREFIX(machine, "pc-1.") ||
+             STRPREFIX(machine, "pc-i440") ||
+             STRPREFIX(machine, "rhel")));
 }
 
 
@@ -6689,7 +6691,8 @@ qemuDomainIsS390CCW(const virDomainDef *def)
 bool
 qemuDomainMachineIsS390CCW(const char *machine)
 {
-    return STRPREFIX(machine, "s390-ccw");
+    return (machine &&
+            STRPREFIX(machine, "s390-ccw"));
 }
 
 
@@ -6708,11 +6711,9 @@ qemuDomainMachineIsVirt(const char *machine,
         arch != VIR_ARCH_AARCH64)
         return false;
 
-    if (STRNEQ(machine, "virt") &&
-        !STRPREFIX(machine, "virt-"))
-        return false;
-
-    return true;
+    return (machine &&
+            (STREQ(machine, "virt") ||
+             STRPREFIX(machine, "virt-")));
 }
 
 
@@ -6730,11 +6731,9 @@ qemuDomainMachineIsPSeries(const char *machine,
     if (!ARCH_IS_PPC64(arch))
         return false;
 
-    if (STRNEQ(machine, "pseries") &&
-        !STRPREFIX(machine, "pseries-"))
-        return false;
-
-    return true;
+    return (machine &&
+            (STREQ(machine, "pseries") ||
+             STRPREFIX(machine, "pseries-")));
 }
 
 
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args
new file mode 100644
index 000000000..b17c0d0c2
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args
@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-aarch64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml
new file mode 100644
index 000000000..eb8f9db80
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml
@@ -0,0 +1,39 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='aarch64' machine='virt'>hvm</type>
+  </os>
+  <features>
+    <gic version='2'/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-aarch64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='ide'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
+    </disk>
+    <controller type='ide' index='0'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+    </video>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args
new file mode 100644
index 000000000..ac618775e
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args
@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml
new file mode 100644
index 000000000..1e987f645
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml
@@ -0,0 +1,47 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='ppc64' machine='pseries'>hvm</type>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-ppc64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='ide'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'>
+      <model name='spapr-pci-host-bridge'/>
+      <target index='0'/>
+    </controller>
+    <controller type='ide' index='0'/>
+    <input type='keyboard' bus='usb'/>
+    <input type='mouse' bus='usb'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <memballoon model='none'/>
+    <panic model='pseries'/>
+  </devices>
+</domain>
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args
new file mode 100644
index 000000000..22bc4fb1c
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args
@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml
new file mode 100644
index 000000000..71a36f083
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml
@@ -0,0 +1,48 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-0.11'>hvm</type>
+  </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-system-x86_64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='ide'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='ide' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <memballoon model='none'/>
+  </devices>
+</domain>
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 1adbcfef6..5d261afa6 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -288,6 +288,10 @@ mymain(void)
     DO_TEST("machine-deakeywrap-off-argv");
     DO_TEST("machine-keywrap-none-argv");
 
+    DO_TEST("nomachine-x86_64");
+    DO_TEST("nomachine-aarch64");
+    DO_TEST("nomachine-ppc64");
+
     qemuTestDriverFree(&driver);
 
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Oct 10, 2017 at 04:21:02PM +0200, Andrea Bolognani wrote:
> Make sure pointers are non-NULL before dereferencing them, and
> add test suite coverage for the crashers doing so fixes.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1379218

Hmm, I don't think that is a good fix for the problem seen there.

We're parsing the CLI argv from an existing QEMU process and
when looking at the disk we're checking if the machine type
refers to a pseries guest.

The problem is that either the user might not have given any
-machine arg, or the -drive arg might occur *before* the
-machine arg is parsed.

Simply making the qemuDomainMachineIs* safe against NULL will
avoid the crash, but the ARGV parsing is still going to be
semantically broken.

As a more general point, we've tended to assume that machine
is always non-NULL throughout the code, because we rely on
the XML parsing to fill in defaults if omitted by the user.

I think rather than trying to fix up the assumption about
machine being non-NULL, we should restructure the ARGV
parsing into we need a 2 pass process.

In the first pass only look for the -machine arg. If no
-machine arg is given, we should fill in the default machine
for that emulator.

In the second pass process the rest of the ARGV, whereupon
we have a valid assumption that machine is non-NULL.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2017-10-10 at 15:49 +0100, Daniel P. Berrange wrote:
> I think rather than trying to fix up the assumption about
> machine being non-NULL, we should restructure the ARGV
> parsing into we need a 2 pass process.
> 
> In the first pass only look for the -machine arg. If no
> -machine arg is given, we should fill in the default machine
> for that emulator.

I can do this...

> In the second pass process the rest of the ARGV, whereupon
> we have a valid assumption that machine is non-NULL.

... but I'm not sure doing this is a good idea.

Wouldn't it be much safer to leave the newly-added NULL checks
in place so that the qemuDomainMachineIs*() functions are locally
correct instead of relying on external guarantees in order not
to crash on the user?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Oct 10, 2017 at 05:05:33PM +0200, Andrea Bolognani wrote:
> On Tue, 2017-10-10 at 15:49 +0100, Daniel P. Berrange wrote:
> > I think rather than trying to fix up the assumption about
> > machine being non-NULL, we should restructure the ARGV
> > parsing into we need a 2 pass process.
> > 
> > In the first pass only look for the -machine arg. If no
> > -machine arg is given, we should fill in the default machine
> > for that emulator.
> 
> I can do this...
> 
> > In the second pass process the rest of the ARGV, whereupon
> > we have a valid assumption that machine is non-NULL.
> 
> ... but I'm not sure doing this is a good idea.
> 
> Wouldn't it be much safer to leave the newly-added NULL checks
> in place so that the qemuDomainMachineIs*() functions are locally
> correct instead of relying on external guarantees in order not
> to crash on the user?

Are these qemuDomainMachineIs* functions the only places that makes
assumptions about machine being non-NULL though ? I wouldn't want to
encourage the idea that it is OK to have a NULL machine value, because
that would just push crashes to elsewhere in the code if people rely
on it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2017-10-10 at 16:10 +0100, Daniel P. Berrange wrote:
> > Wouldn't it be much safer to leave the newly-added NULL checks
> > in place so that the qemuDomainMachineIs*() functions are locally
> > correct instead of relying on external guarantees in order not
> > to crash on the user?
> 
> Are these qemuDomainMachineIs* functions the only places that makes
> assumptions about machine being non-NULL though ?

I guess we don't know.

And that's why assumptions should be verified locally :)

> I wouldn't want to
> encourage the idea that it is OK to have a NULL machine value, because
> that would just push crashes to elsewhere in the code if people rely
> on it.

Okay, I'll drop those hunks from the respin then.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list