[libvirt] [PATCH] util: ensure safe{read, write, zero} return is checked
by Eric Blake
Based on a warning from coverity. The safe* functions
guarantee complete transactions on success, but don't guarantee
freedom from failure.
* src/util/util.h (saferead, safewrite, safezero): Add
ATTRIBUTE_RETURN_CHECK.
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Ignore
some failures.
(remoteIOReadBuffer): Adjust error messages on read failure.
* daemon/event.c (virEventHandleWakeup): Ignore read failure.
---
Coverity claimed that safewrite was checked 37 out of 46 times,
but only flagged one instance of a CHECKED_RETURN report,
against remote_driver.c. I didn't spot the 9 unchecked calls
that coverity's numbers would imply. Meanwhile, adding
the attribute ensures that we don't forget this in the future.
Please double-check if my use of ignore_value on some of the
previously unchecked calls makes sense, or if we need a more
invasive patch to deal with real failure.
daemon/event.c | 5 +++--
src/remote/remote_driver.c | 13 +++++++++----
src/util/util.h | 9 ++++++---
3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/daemon/event.c b/daemon/event.c
index 2218a3e..36e9dd3 100644
--- a/daemon/event.c
+++ b/daemon/event.c
@@ -1,8 +1,8 @@
/*
* event.c: event loop for monitoring file handles
*
+ * Copyright (C) 2007, 2010 Red Hat, Inc.
* Copyright (C) 2007 Daniel P. Berrange
- * Copyright (C) 2007 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -35,6 +35,7 @@
#include "event.h"
#include "memory.h"
#include "util.h"
+#include "ignore-value.h"
#define EVENT_DEBUG(fmt, ...) DEBUG(fmt, __VA_ARGS__)
@@ -630,7 +631,7 @@ static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED,
{
char c;
virEventLock();
- saferead(fd, &c, sizeof(c));
+ ignore_value (saferead(fd, &c, sizeof(c)));
virEventUnlock();
}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index def4617..47d8c56 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -7809,7 +7809,9 @@ remoteIOReadBuffer(virConnectPtr conn,
char errout[1024] = "\0";
if (priv->errfd != -1) {
- saferead(priv->errfd, errout, sizeof(errout));
+ if (saferead(priv->errfd, errout, sizeof(errout)) < 0) {
+ strncpy (errout, _("unknown reason"), sizeof errout);
+ }
}
virReportSystemError(errno,
@@ -7818,7 +7820,9 @@ remoteIOReadBuffer(virConnectPtr conn,
} else {
char errout[1024] = "\0";
if (priv->errfd != -1) {
- saferead(priv->errfd, errout, sizeof(errout));
+ if (saferead(priv->errfd, errout, sizeof(errout)) < 0) {
+ strncpy (errout, _("unknown reason"), sizeof errout);
+ }
}
errorf (in_open ? NULL : conn,
@@ -8431,7 +8435,8 @@ remoteIOEventLoop(virConnectPtr conn,
if (fds[1].revents) {
DEBUG0("Woken up from poll by other thread");
- saferead(priv->wakeupReadFD, &ignore, sizeof(ignore));
+ ignore_value (saferead(priv->wakeupReadFD, &ignore,
+ sizeof(ignore)));
}
if (ret < 0) {
@@ -8575,7 +8580,7 @@ remoteIO(virConnectPtr conn,
priv->waitDispatch = thiscall;
/* Force other thread to wakup from poll */
- safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore));
+ ignore_value (safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)));
DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall);
/* Go to sleep while other thread is working... */
diff --git a/src/util/util.h b/src/util/util.h
index cc05abe..34b77fa 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -2,6 +2,7 @@
/*
* utils.h: common, generic utility functions
*
+ * Copyright (C) 2010 Red Hat, Inc.
* Copyright (C) 2006, 2007 Binary Karma
* Copyright (C) 2006 Shuveb Hussain
*
@@ -31,9 +32,11 @@
#include <sys/select.h>
#include <sys/types.h>
-int saferead(int fd, void *buf, size_t count);
-ssize_t safewrite(int fd, const void *buf, size_t count);
-int safezero(int fd, int flags, off_t offset, off_t len);
+int saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK;
+ssize_t safewrite(int fd, const void *buf, size_t count)
+ ATTRIBUTE_RETURN_CHECK;
+int safezero(int fd, int flags, off_t offset, off_t len)
+ ATTRIBUTE_RETURN_CHECK;
enum {
VIR_EXEC_NONE = 0,
--
1.6.6.1
14 years, 10 months
[libvirt] [PATCH] Fix typo in QEMU migration command name
by Daniel P. Berrange
The QMP code was running query-migration instead of query-migrate.
This doesn't work so well
* src/qemu/qemu_monitor_json.c: s/query-migration/query-migrate/
---
src/qemu/qemu_monitor_json.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3a94dd0..9c0245c 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1108,7 +1108,7 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon,
unsigned long long *total)
{
int ret;
- virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-migration",
+ virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-migrate",
NULL);
virJSONValuePtr reply = NULL;
--
1.6.5.2
14 years, 10 months
[libvirt] [PATCH] Add TCK script for host PCI device hotplug/unplug
by Daniel P. Berrange
This adds support for testing host PCI device hotplug/unplug in
libvirt drivers. This requires that the host have a usable
IOMMU & hardware virt, and that the device in question can be
reset safely.
Since the TCK can't assume there are any host PCI devices that
can be safely messed around with, the person running the test
suite must first list one or more devices in the config file.
If no devices are listed, the test will automatically skip
all parts, rather than failing.
* conf/default.cfg: Config entry to specify PCI devices that the
TCK can mess around with
* lib/Sys/Virt/TCK.pm: API for getting a host PCI device from
the config
* scripts/domain/250-pci-host-hotplug.t: Test case for PCI
device hotplug/unplug.
---
conf/default.cfg | 15 ++++
lib/Sys/Virt/TCK.pm | 18 +++++
scripts/domain/250-pci-host-hotplug.t | 117 +++++++++++++++++++++++++++++++++
3 files changed, 150 insertions(+), 0 deletions(-)
create mode 100644 scripts/domain/250-pci-host-hotplug.t
diff --git a/conf/default.cfg b/conf/default.cfg
index f9c2b5c..5b85cbc 100644
--- a/conf/default.cfg
+++ b/conf/default.cfg
@@ -112,3 +112,18 @@ host_usb_devices = (
# }
# Can list more than one USB device if many are available
)
+
+
+# Host PCI devices that the test suite can safely mess around with
+# without risk of breaking the host OS. Using a NIC device is the
+# most reliable option. Definitely don't try listing a VGA device.
+host_pci_devices = (
+# Slot is only required entry, all others default to 0
+# {
+# domain = 0000
+# bus = 00
+# slot = 07
+# function = 0
+# }
+# Can list more than one PCI device if many are available
+)
diff --git a/lib/Sys/Virt/TCK.pm b/lib/Sys/Virt/TCK.pm
index ce03935..f101937 100644
--- a/lib/Sys/Virt/TCK.pm
+++ b/lib/Sys/Virt/TCK.pm
@@ -787,4 +787,22 @@ sub get_host_usb_device {
return ($bus, $device, $vendor, $product);
}
+sub get_host_pci_device {
+ my $self = shift;
+ my $devindex = @_ ? shift : 0;
+
+ my $devs = $self->config("host_pci_devices", []);
+
+ if ($devindex > $#{$devs}) {
+ return ();
+ }
+
+ my $domain = $self->config("host_pci_devices/[$devindex]/domain", 0);
+ my $bus = $self->config("host_pci_devices/[$devindex]/bus", 0);
+ my $slot = $self->config("host_pci_devices/[$devindex]/slot");
+ my $function = $self->config("host_pci_devices/[$devindex]/function", 0);
+
+ return ($domain, $bus, $slot, $function);
+}
+
1;
diff --git a/scripts/domain/250-pci-host-hotplug.t b/scripts/domain/250-pci-host-hotplug.t
new file mode 100644
index 0000000..9ccd036
--- /dev/null
+++ b/scripts/domain/250-pci-host-hotplug.t
@@ -0,0 +1,117 @@
+# -*- perl -*-
+#
+# Copyright (C) 2009 Red Hat
+# Copyright (C) 2009 Daniel P. Berrange
+#
+# This program is free software; You can redistribute it and/or modify
+# it under the GNU General Public License as published by the Free
+# Software Foundation; either version 2, or (at your option) any
+# later version
+#
+# The file "LICENSE" distributed along with this file provides full
+# details of the terms and conditions
+#
+
+=pod
+
+=head1 NAME
+
+domain/250-pci-host-hotplug.t - verify hot plug & unplug of a host PCI device
+
+=head1 DESCRIPTION
+
+The test case validates that it is possible to hotplug a pci
+host device to a running domain, and then unplug it again.
+This requires that the TCK configuration file have at least
+one host PCI device listed.
+
+This first searches for the node device, then detachs it from
+the host OS. Next it performs a reset. Then does the hotplug
+and unplug, before finally reattaching to the host.
+
+=cut
+
+use strict;
+use warnings;
+
+use Test::More tests => 10;
+
+use Sys::Virt::TCK;
+use Test::Exception;
+
+my $tck = Sys::Virt::TCK->new();
+my $conn = eval { $tck->setup(); };
+BAIL_OUT "failed to setup test harness: $@" if $@;
+END {
+ $tck->cleanup if $tck;
+}
+
+
+my $xml = $tck->generic_domain("tck")->as_xml;
+
+diag "Creating a new transient domain";
+my $dom;
+ok_domain(sub { $dom = $conn->create_domain($xml) }, "created transient domain object");
+
+
+my ($domain, $bus, $slot, $function) = $tck->get_host_pci_device();
+
+SKIP: {
+ # Must have one item be non-zero
+ unless ($domain || $bus || $slot || $function) {
+ skip "No host pci device available in configuration file", 9;
+ }
+
+ my $nodedev;
+ my @devs = $conn->list_node_devices("pci");
+ foreach my $dev (@devs) {
+ my $thisxml = $dev->get_xml_description();
+
+ my $xp = XML::XPath->new(xml => $thisxml);
+
+ #warn $thisxml;
+ my $ndomain = $xp->find("string(/device/capability[\@type='pci']/domain[text()])")->value();
+ my $nbus = $xp->find("string(/device/capability[\@type='pci']/bus[text()])")->value();
+ my $nslot = $xp->find("string(/device/capability[\@type='pci']/slot[text()])")->value();
+ my $nfunction = $xp->find("string(/device/capability[\@type='pci']/function[text()])")->value();
+
+ if ($ndomain == $domain &&
+ $nbus == $bus &&
+ $nslot == $slot &&
+ $nfunction == $function) {
+ $nodedev = $dev;
+ last;
+ }
+ }
+
+ ok(defined $nodedev, "found PCI device $domain:$bus:$slot.$function on host");
+
+ lives_ok(sub { $nodedev->dettach() }, "detached device from host OS");
+ lives_ok(sub { $nodedev->reset() }, "reset the host PCI device");
+
+ my $devxml =
+ "<hostdev mode='subsystem' type='pci' managed='no'>\n" .
+ " <source>\n" .
+ " <address domain='$domain' bus='$bus' slot='$slot' function='$function'/>\n" .
+ " </source>\n" .
+ "</hostdev>\n";
+
+ my $initialxml = $dom->get_xml_description;
+
+ diag "Attaching the new dev $devxml";
+ lives_ok(sub { $dom->attach_device($devxml); }, "PCI dev has been attached");
+
+ my $newxml = $dom->get_xml_description;
+ ok($newxml =~ m|<hostdev|, "new XML has extra PCI dev present");
+
+ diag "Detaching the new dev $devxml";
+ lives_ok(sub { $dom->detach_device($devxml); }, "PCI dev has been detached");
+
+ lives_ok(sub { $nodedev->reset() }, "reset the host PCI device");
+ lives_ok(sub { $nodedev->reattach() }, "reattached device to host OS");
+
+ my $finalxml = $dom->get_xml_description;
+
+ is($initialxml, $finalxml, "final XML has removed the disk")
+}
+
--
1.6.5.2
14 years, 10 months
[libvirt] [PATCH] uml: avoid crash on partial read
by Eric Blake
Coverity detected a potential dereference of uninitialized memory
if recvfrom got cut short.
* src/uml/uml_driver.c (umlMonitorCommand): Validate complete read
prior to dereferencing res.
---
The patch borrows ideas from macvtap.c, the only other file in
libvirt that currently uses recvfrom.
I did not analyze whether this is a security hole, where a
malicious UDP packet could intentionally force the dereferencing
of uninitialized memory to misbehave in a controlled manner.
src/uml/uml_driver.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index bbea429..eec239f 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -733,14 +733,24 @@ static int umlMonitorCommand(virConnectPtr conn,
}
do {
+ ssize_t nbytes;
addrlen = sizeof(addr);
- if (recvfrom(priv->monitor, &res, sizeof res, 0,
- (struct sockaddr *)&addr, &addrlen) < 0) {
+ nbytes = recvfrom(priv->monitor, &res, sizeof res, 0,
+ (struct sockaddr *)&addr, &addrlen) < 0;
+ if (nbytes < 0) {
+ if (errno == EAGAIN || errno == EINTR)
+ continue;
virReportSystemError(errno,
_("cannot read reply %s"),
cmd);
goto error;
}
+ if (nbytes < sizeof res) {
+ virReportSystemError(errno,
+ _("incomplete reply %s"),
+ cmd);
+ goto error;
+ }
if (VIR_REALLOC_N(retdata, retlen + res.length) < 0) {
virReportOOMError();
--
1.6.6.1
14 years, 10 months
[libvirt] [PATCH] don't let a bogus packet trigger over-allocation and segfault
by Jim Meyering
Another not-really-urgent fix:
>From 4b56f03dee82657be3af5c79c826ae3091fbf522 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 3 Mar 2010 16:50:02 +0100
Subject: [PATCH] don't let a bogus packet trigger over-allocation and segfault
* src/xen/proxy_internal.c (xenProxyDomainDumpXML): An invalid packet
could include a too-large "ans.len" value, which would make us allocate
too much memory and then copy data from beyond the end of "ans",
possibly evoking a segfault. Ensure that the value we use is no
larger than the remaining portion of "ans".
Also, change unnecessary memmove to memcpy (src and dest obviously
do not overlap, so no need to use memmove).
---
src/xen/proxy_internal.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c
index 8e1c226..bd234ec 100644
--- a/src/xen/proxy_internal.c
+++ b/src/xen/proxy_internal.c
@@ -978,26 +978,27 @@ xenProxyDomainDumpXML(virDomainPtr domain, int flags ATTRIBUTE_UNUSED)
memset(&req, 0, sizeof(req));
req.command = VIR_PROXY_DOMAIN_XML;
req.data.arg = domain->id;
req.len = sizeof(req);
ret = xenProxyCommand(domain->conn, &req, &ans, 0);
if (ret < 0) {
return(NULL);
}
- if (ans.len <= sizeof(virProxyPacket)) {
+ if (ans.len <= sizeof(virProxyPacket)
+ || ans.len > sizeof (ans) - sizeof(virProxyPacket)) {
virProxyError(domain->conn, VIR_ERR_OPERATION_FAILED, __FUNCTION__);
return (NULL);
}
xmllen = ans.len - sizeof(virProxyPacket);
if (VIR_ALLOC_N(xml, xmllen+1) < 0) {
virReportOOMError();
return NULL;
}
- memmove(xml, &ans.extra.dinfo, xmllen);
+ memcpy(xml, &ans.extra.dinfo, xmllen);
xml[xmllen] = '\0';
return(xml);
}
/**
* xenProxyDomainGetOSType:
* @domain: a domain object
--
1.7.0.1.464.g0adc7
14 years, 10 months
[libvirt] [PATCH] virFileReadLimFD: diagnose maxlen <= 0, rather than passing it on...
by Jim Meyering
Not urgent.
>From 35a24209934cb983ca52e74f580b3ea963d92f57 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 3 Mar 2010 11:42:11 +0100
Subject: [PATCH] virFileReadLimFD: diagnose maxlen <= 0, rather than passing it on...
to saferead_lim, which interprets it as a size_t.
* src/util/util.c (virFileReadLimFD): Do not malfunction when
maxlen < -1. Return -1,EINVAL in that case. Handle maxlen==0
in the same manner.
---
src/util/util.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 34c585d..7a3a3c4 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1030,10 +1030,17 @@ saferead_lim (int fd, size_t max_len, size_t *length)
/* A wrapper around saferead_lim that maps a failure due to
exceeding the maximum size limitation to EOVERFLOW. */
-int virFileReadLimFD(int fd, int maxlen, char **buf)
+int
+virFileReadLimFD(int fd, int maxlen, char **buf)
{
size_t len;
- char *s = saferead_lim (fd, maxlen+1, &len);
+ char *s;
+
+ if (maxlen <= 0) {
+ errno = EINVAL;
+ return -1;
+ }
+ s = saferead_lim (fd, maxlen+1, &len);
if (s == NULL)
return -1;
if (len > maxlen || (int)len != len) {
--
1.7.0.1.464.g0adc7
14 years, 10 months
[libvirt] [PATCH] qemu restore: don't let corrupt input provoke unwarranted OOM
by Jim Meyering
As the log says, we must not use a just-read (and unverified)
header.xml_len as the size of buffer to allocate.
While looking at this, I noticed a small problem with virFileReadLimFD.
Passing it a limit < -1 would cause int->size_t trouble with saferead_lim.
Separate patch coming up.
>From a4906ba24b576f3219234c519c88d102df2173b2 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 3 Mar 2010 11:27:16 +0100
Subject: [PATCH] qemu restore: don't let corrupt input provoke unwarranted OOM
* src/qemu/qemu_driver.c (qemudDomainRestore): A corrupt save file
(in particular, a too-large header.xml_len value) would cause an
unwarranted out-of-memory error. Do not trust the just-read
header.xml_len. Instead, merely use that as a hint, and
read/allocate up to that number of bytes from the file.
Also verify that header.xml_len is positive; if it were negative,
passing it to virFileReadLimFD could cause trouble.
---
src/qemu/qemu_driver.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7f7c459..5c9d003 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4725,44 +4725,45 @@ static int qemudDomainRestore(virConnectPtr conn,
if (saferead(fd, &header, sizeof(header)) != sizeof(header)) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("failed to read qemu header"));
goto cleanup;
}
if (memcmp(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)) != 0) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("image magic is incorrect"));
goto cleanup;
}
if (header.version > QEMUD_SAVE_VERSION) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
_("image version is not supported (%d > %d)"),
header.version, QEMUD_SAVE_VERSION);
goto cleanup;
}
- if (VIR_ALLOC_N(xml, header.xml_len) < 0) {
- virReportOOMError();
+ if (header.xml_len <= 0) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ _("invalid XML length: %d"), header.xml_len);
goto cleanup;
}
- if (saferead(fd, xml, header.xml_len) != header.xml_len) {
+ if (virFileReadLimFD(fd, header.xml_len, &xml) != header.xml_len) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("failed to read XML"));
goto cleanup;
}
/* Create a domain from this XML */
if (!(def = virDomainDefParseString(driver->caps, xml,
VIR_DOMAIN_XML_INACTIVE))) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("failed to parse XML"));
goto cleanup;
}
if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0)
goto cleanup;
if (!(vm = virDomainAssignDef(driver->caps,
&driver->domains,
def))) {
--
1.7.0.1.464.g0adc7
14 years, 10 months
[libvirt] [PATCH 0/1] - for comments/help only, nearly working root-squash-domain-save
by Laine Stump
(This patch requires that you first apply the virFileOperation patches
that I sent to the list yesterday.)
I'm hoping someone else's keen eye can see what I'm failing to see
here.
The patch following this message makes domain save "work" on
root-squash NFS (both the libvirt header and the QEMU state are saved
to the file), but the files that it saves don't restore properly, even
when simply saving to a local directory w/o forking - the guest screen
comes up black, or reboots immediately, or some other such thing. I've
looked at the data files and the code, and can't see what difference
is causing this - at least the header written by libvirt seems to be
the same, and the part written by qemu starts in the same place and
looks similar.
If anyone's got an idea, please let me know! I'd like to get this
figured out and a working patch submitted before the day is done.
14 years, 10 months
[libvirt] [PATCH 0/1] Support domain restore from root-squash NFS
by Laine Stump
Most of the explanation is in the commit message with the patch
proper. When reveiwing please take pains to point out any places where
I may not be diligent enough cleaning up loose ends, especially in
terms of assuring that the child process always terminates properly no
matter what happens.
The (unrelated) race condition in qemudDomainRestore() remains - I
solved it locally by putting a sleep(10) just before
qemuMonitorStartCPUs(), but it needs something with a bit higher
comfort factor ;-)
14 years, 10 months
