The nwfilter conf update mutex previously serialized
updates to the internal data structures for firewall
rules, and updates to the firewall itself. The latter
was recently turned into a read/write lock, and filter
instantiation allowed to proceed in parallel. It was
believed that this was ok, since each filter is created
on a separate iptables/ebtables chain.
It turns out that there is a subtle lock ordering problem
on virNWFilterObjPtr instances. __virNWFilterInstantiateFilter
will hold a lock on the virNWFilterObjPtr it is instantiating.
This in turn invokes virNWFilterInstantiate which then invokes
virNWFilterDetermineMissingVarsRec which then invokes
virNWFilterObjFindByName. This iterates over every single
virNWFilterObjPtr in the list, locking them and checking their
name. So if 2 or more threads try to instantiate a filter in
parallel, they'll all hold 1 lock at the top level in the
__virNWFilterInstantiateFilter method which will cause the
other thread to deadlock in virNWFilterObjFindByName.
The fix is to add an exclusive mutex to serialize the
execution of __virNWFilterInstantiateFilter.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 925de19ed7)
Rewrite multiple hotunplug functions to to use the
virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with an absolute
symlink, tricking the driver into changing the host OS
filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 5fc590ad9f)
Rewrite lxcDomainAttachDeviceHostdevMiscLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 1cadeafcaa)
Rewrite lxcDomainAttachDeviceHostdevStorageLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 1754c7f0ab)
Rewrite lxcDomainAttachDeviceHostdevSubsysUSBLive function
to use the virProcessRunInMountNamespace helper. This avoids
risk of a malicious guest replacing /dev with a absolute
symlink, tricking the driver into changing the host OS
filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 7fba01c15c)
Rewrite lxcDomainAttachDeviceDiskLive function to use the
virProcessRunInMountNamespace helper. This avoids risk of
a malicious guest replacing /dev with a absolute symlink,
tricking the driver into changing the host OS filesystem.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 4dd3a7d5bc)
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and
lxcDomainReboot. Otherwise, a malicious guest could use symlinks
to force the host to manipulate the wrong file in the host's namespace.
Idea by Dan Berrange, based on an initial report by Reco
<recoverym4n@gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit aebbcdd33c)
Implement virProcessRunInMountNamespace, which runs callback of type
virProcessNamespaceCallback in a container namespace. This uses a
child process to run the callback, since you can't change the mount
namespace of a thread. This implies that callbacks have to be careful
about what code they run due to async safety rules.
Idea by Dan Berrange, based on an initial report by Reco
<recoverym4n@gmail.com> at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=732394
Signed-off-by: Daniel Berrange <berrange@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 7c72ef6f55)
Add a helper function which takes a file path and ensures
that all directory components leading up to the file exist.
IOW, it strips the filename part of the path and passes
the result to virFileMakePath.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit c321bfc5c3)
The check for whether the cgroup devices ACL is available is
done quite late during LXC hotplug - in fact after the device
node is already created in the container in some cases. Better
to do it upfront so we fail immediately.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit c3eb12cace)
The LXC disk hotplug code was allowing block or character devices
to be given as disk. A disk is always a block device.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit d24e6b8b1e)
When detaching a USB device from an LXC guest we must remove
the device from the cgroup ACL. Unfortunately we were telling
the cgroup code to use the guest /dev path, not the host /dev
path, and the guest device node had already been unlinked.
This was, however, fortunate since the code passed &priv->cgroup
instead of priv->cgroup, so would have crash if the device node
were accessible.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 2c2bec94d2)
After hotplugging a USB device, the LXC driver forgot
to add the device def to the virDomainDefPtr.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit a537827d15)
The LXC code missed the 'usb' component out of the path
/dev/bus/usb/$BUSNUM/$DEVNUM, so it failed to actually
setup cgroups for the device. This was in fact lucky
because the call to virLXCSetupHostUsbDeviceCgroup
was also mistakenly passing '&priv->cgroup' instead of
just 'priv->cgroup'. So once the path is fixed, libvirtd
would then crash trying to access the bogus virCgroupPtr
pointer. This would have been a security issue, were it
not for the bogus path preventing the pointer reference
being reached.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit c364897222)
virDomainDefCompatibleDevice blocks use of USB if no USB
controller is present. This is not correct for containers
since devices can be assigned directly regardless of any
controllers.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 7a44af963e)
https://bugzilla.redhat.com/show_bug.cgi?id=1058839
Commit f9f56340 for CVE-2014-0028 almost had the right idea - we
need to check the ACL rules to filter which events to send. But
it overlooked one thing: the event dispatch queue is running in
the main loop thread, and therefore does not normally have a
current virIdentityPtr. But filter checks can be based on current
identity, so when libvirtd.conf contains access_drivers=["polkit"],
we ended up rejecting access for EVERY event due to failure to
look up the current identity, even if it should have been allowed.
Furthermore, even for events that are triggered by API calls, it
is important to remember that the point of events is that they can
be copied across multiple connections, which may have separate
identities and permissions. So even if events were dispatched
from a context where we have an identity, we must change to the
correct identity of the connection that will be receiving the
event, rather than basing a decision on the context that triggered
the event, when deciding whether to filter an event to a
particular connection.
If there were an easy way to get from virConnectPtr to the
appropriate virIdentityPtr, then object_event.c could adjust the
identity prior to checking whether to dispatch an event. But
setting up that back-reference is a bit invasive. Instead, it
is easier to delay the filtering check until lower down the
stack, at the point where we have direct access to the RPC
client object that owns an identity. As such, this patch ends
up reverting a large portion of the framework of commit f9f56340.
We also have to teach 'make check' to special-case the fact that
the event registration filtering is done at the point of dispatch,
rather than the point of registration. Note that even though we
don't actually use virConnectDomainEventRegisterCheckACL (because
the RegisterAny variant is sufficient), we still generate the
function for the purposes of documenting that the filtering
takes place.
Also note that I did not entirely delete the notion of a filter
from object_event.c; I still plan on using that for my upcoming
patch series for qemu monitor events in libvirt-qemu.so. In
other words, while this patch changes ACL filtering to live in
remote.c and therefore we have no current client of the filtering
in object_event.c, the notion of filtering in object_event.c is
still useful down the road.
* src/check-aclrules.pl: Exempt event registration from having to
pass checkACL filter down call stack.
* daemon/remote.c (remoteRelayDomainEventCheckACL)
(remoteRelayNetworkEventCheckACL): New functions.
(remoteRelay*Event*): Use new functions.
* src/conf/domain_event.h (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Drop unused parameter.
* src/conf/network_event.h (virNetworkEventStateRegisterID):
Likewise.
* src/conf/domain_event.c (virDomainEventFilter): Delete unused
function.
* src/conf/network_event.c (virNetworkEventFilter): Likewise.
* src/libxl/libxl_driver.c: Adjust caller.
* src/lxc/lxc_driver.c: Likewise.
* src/network/bridge_driver.c: Likewise.
* src/qemu/qemu_driver.c: Likewise.
* src/remote/remote_driver.c: Likewise.
* src/test/test_driver.c: Likewise.
* src/uml/uml_driver.c: Likewise.
* src/vbox/vbox_tmpl.c: Likewise.
* src/xen/xen_driver.c: Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 11f20e43f1)
The NWFilter code has as a deadlock race condition between
the virNWFilter{Define,Undefine} APIs and starting of guest
VMs due to mis-matched lock ordering.
In the virNWFilter{Define,Undefine} codepaths the lock ordering
is
1. nwfilter driver lock
2. virt driver lock
3. nwfilter update lock
4. domain object lock
In the VM guest startup paths the lock ordering is
1. virt driver lock
2. domain object lock
3. nwfilter update lock
As can be seen the domain object and nwfilter update locks are
not acquired in a consistent order.
The fix used is to push the nwfilter update lock upto the top
level resulting in a lock ordering for virNWFilter{Define,Undefine}
of
1. nwfilter driver lock
2. nwfilter update lock
3. virt driver lock
4. domain object lock
and VM start using
1. nwfilter update lock
2. virt driver lock
3. domain object lock
This has the effect of serializing VM startup once again, even if
no nwfilters are applied to the guest. There is also the possibility
of deadlock due to a call graph loop via virNWFilterInstantiate
and virNWFilterInstantiateFilterLate.
These two problems mean the lock must be turned into a read/write
lock instead of a plain mutex at the same time. The lock is used to
serialize changes to the "driver->nwfilters" hash, so the write lock
only needs to be held by the define/undefine methods. All other
methods can rely on a read lock which allows good concurrency.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
(cherry picked from commit 6e5c79a1b5)
Ever since ACL filtering was added in commit 7639736 (v1.1.1), a
user could still use event registration to obtain access to a
domain that they could not normally access via virDomainLookup*
or virConnectListAllDomains and friends. We already have the
framework in the RPC generator for creating the filter, and
previous cleanup patches got us to the point that we can now
wire the filter through the entire object event stack.
Furthermore, whether or not domain:getattr is honored, use of
global events is a form of obtaining a list of networks, which
is covered by connect:search_domains added in a93cd08 (v1.1.0).
Ideally, we'd have a way to enforce connect:search_domains when
doing global registrations while omitting that check on a
per-domain registration. But this patch just unconditionally
requires connect:search_domains, even when no list could be
obtained, based on the following observations:
1. Administrators are unlikely to grant domain:getattr for one
or all domains while still denying connect:search_domains - a
user that is able to manage domains will want to be able to
manage them efficiently, but efficient management includes being
able to list the domains they can access. The idea of denying
connect:search_domains while still granting access to individual
domains is therefore not adding any real security, but just
serves as a layer of obscurity to annoy the end user.
2. In the current implementation, domain events are filtered
on the client; the server has no idea if a domain filter was
requested, and must therefore assume that all domain event
requests are global. Even if we fix the RPC protocol to
allow for server-side filtering for newer client/server combos,
making the connect:serach_domains ACL check conditional on
whether the domain argument was NULL won't benefit older clients.
Therefore, we choose to document that connect:search_domains
is a pre-requisite to any domain event management.
Network events need the same treatment, with the obvious
change of using connect:search_networks and network:getattr.
* src/access/viraccessperm.h
(VIR_ACCESS_PERM_CONNECT_SEARCH_DOMAINS)
(VIR_ACCESS_PERM_CONNECT_SEARCH_NETWORKS): Document additional
effect of the permission.
* src/conf/domain_event.h (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Add new parameter.
* src/conf/network_event.h (virNetworkEventStateRegisterID):
Likewise.
* src/conf/object_event_private.h (virObjectEventStateRegisterID):
Likewise.
* src/conf/object_event.c (_virObjectEventCallback): Track a filter.
(virObjectEventDispatchMatchCallback): Use filter.
(virObjectEventCallbackListAddID): Register filter.
* src/conf/domain_event.c (virDomainEventFilter): New function.
(virDomainEventStateRegister, virDomainEventStateRegisterID):
Adjust callers.
* src/conf/network_event.c (virNetworkEventFilter): New function.
(virNetworkEventStateRegisterID): Adjust caller.
* src/remote/remote_protocol.x
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER)
(REMOTE_PROC_CONNECT_DOMAIN_EVENT_REGISTER_ANY)
(REMOTE_PROC_CONNECT_NETWORK_EVENT_REGISTER_ANY): Generate a
filter, and require connect:search_domains instead of weaker
connect:read.
* src/test/test_driver.c (testConnectDomainEventRegister)
(testConnectDomainEventRegisterAny)
(testConnectNetworkEventRegisterAny): Update callers.
* src/remote/remote_driver.c (remoteConnectDomainEventRegister)
(remoteConnectDomainEventRegisterAny): Likewise.
* src/xen/xen_driver.c (xenUnifiedConnectDomainEventRegister)
(xenUnifiedConnectDomainEventRegisterAny): Likewise.
* src/vbox/vbox_tmpl.c (vboxDomainGetXMLDesc): Likewise.
* src/libxl/libxl_driver.c (libxlConnectDomainEventRegister)
(libxlConnectDomainEventRegisterAny): Likewise.
* src/qemu/qemu_driver.c (qemuConnectDomainEventRegister)
(qemuConnectDomainEventRegisterAny): Likewise.
* src/uml/uml_driver.c (umlConnectDomainEventRegister)
(umlConnectDomainEventRegisterAny): Likewise.
* src/network/bridge_driver.c
(networkConnectNetworkEventRegisterAny): Likewise.
* src/lxc/lxc_driver.c (lxcConnectDomainEventRegister)
(lxcConnectDomainEventRegisterAny): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com>
We haven't had a release with network events yet, so we are free
to fix the RPC so that it actually does what we want. Doing
client-side filtering of per-network events is inefficient if a
connection is only interested in events on a single network out
of hundreds available on the server. But to do server-side
per-network filtering, the server needs to know which network
to filter on - so we need to pass an optional network over on
registration. Furthermore, it is possible to have a client with
both a global and per-network filter; in the existing code, the
server sends only one event and the client replicates to both
callbacks. But with server-side filtering, the server will send
the event twice, so we need a way for the client to know which
callbackID is sending an event, to ensure that the client can
filter out events from a registration that does not match the
callbackID from the server. Likewise, the existing style of
deregistering by eventID alone is fine; but in the new style,
we have to remember which callbackID to delete.
This patch fixes the RPC wire definition to contain all the
needed pieces of information, and hooks into the server and
client side improvements of the previous patches, in order to
switch over to full server-side filtering of network events.
Also, since we fixed this in time, all released versions of
libvirtd that support network events also support per-network
filtering, so we can hard-code that assumption into
network_event.c.
Converting domain events to server-side filtering will require
the introduction of new RPC numbers, as well as a server
feature bit that the client can use to tell whether to use
old-style (server only supports global events) or new-style
(server supports filtered events), so that is deferred to a
later set of patches.
* src/conf/network_event.c (virNetworkEventStateRegisterClient):
Assume server-side filtering.
* src/remote/remote_protocol.x
(remote_connect_network_event_register_any_args): Add network
argument.
(remote_connect_network_event_register_any_ret): Return callbackID
instead of count.
(remote_connect_network_event_deregister_any_args): Pass
callbackID instead of eventID.
(remote_connect_network_event_deregister_any_ret): Drop unused
type.
(remote_network_event_lifecycle_msg): Add callbackID.
* daemon/remote.c
(remoteDispatchConnectNetworkEventDeregisterAny): Drop unused arg,
and deal with callbackID from client.
(remoteRelayNetworkEventLifecycle): Pass callbackID.
(remoteDispatchConnectNetworkEventRegisterAny): Likewise, and
recognize non-NULL network.
* src/remote/remote_driver.c
(remoteConnectNetworkEventRegisterAny): Pass network, and track
server side id.
(remoteConnectNetworkEventDeregisterAny): Deregister by callback id.
(remoteNetworkBuildEventLifecycle): Pass remote id to event queue.
* src/remote_protocol-structs: Regenerate.
Signed-off-by: Eric Blake <eblake@redhat.com>
In order to mirror a server with per-object filtering, the client
needs to track which server callbackID is servicing the client
callback. This patch introduces the notion of a serverID, as
well as the plumbing to use it for network events, although the
actual complexity of using per-object filtering in the remote
driver is deferred to a later patch.
* src/conf/object_event.h (virObjectEventStateEventID): Add parameter.
(virObjectEventStateQueueRemote, virObjectEventStateSetRemote):
New prototypes.
(virObjectEventStateRegisterID): Move...
* src/conf/object_event_private.h: ...here, and add parameter.
(_virObjectEvent): Add field.
* src/conf/network_event.h (virNetworkEventStateRegisterClient): New
prototype.
* src/conf/object_event.c (_virObjectEventCallback): Add field.
(virObjectEventStateSetRemote): New function.
(virObjectEventStateQueue): Make wrapper around...
(virObjectEventStateQueueRemote): New function.
(virObjectEventCallbackListCount): Tweak return count when remote
id matching is used.
(virObjectEventCallbackLookup, virObjectEventStateRegisterID):
Tweak registration when remote id matching will be used.
(virObjectEventNew): Default to no remote id.
(virObjectEventCallbackListAddID): Likewise, but set remote id
when one is available.
(virObjectEventCallbackListRemoveID)
(virObjectEventCallbackListMarkDeleteID): Adjust return value when
remote id was set.
(virObjectEventStateEventID): Query existing id.
(virObjectEventDispatchMatchCallback): Require matching event id.
(virObjectEventStateCallbackID): Adjust caller.
* src/conf/network_event.c (virNetworkEventStateRegisterClient): New
function.
(virNetworkEventStateRegisterID): Update caller.
* src/conf/domain_event.c (virDomainEventStateRegister)
(virDomainEventStateRegisterID): Update callers.
* src/remote/remote_driver.c
(remoteConnectNetworkEventRegisterAny)
(remoteConnectNetworkEventDeregisterAny)
(remoteConnectDomainEventDeregisterAny): Likewise.
(remoteEventQueue): Hoist earlier to avoid forward declaration,
and add parameter. Adjust all callers.
* src/libvirt_private.syms (conf/object_event.h): Drop function.
Signed-off-by: Eric Blake <eblake@redhat.com>
Right now, the daemon side of RPC events is hard-coded to at most
one callback per eventID. But when there are hundreds of domains
or networks coupled and multiple conections, then sending every
event to every connection that wants an event, even for the
connections that only care about events for a particular object,
is inefficient. In order to track more than one callback in the
server, we need to store callbacks by more than just their
eventID. This patch rearranges the daemon side to store network
callbacks in a dynamic array, which can eventually be used for
multiple callbacks of the same eventID, although actual behavior
is unchanged without further patches to the RPC protocol. For
ease of review, domain events are saved for a later patch, as
they touch more code.
While at it, fix a bug where a malicious client could send a
negative eventID to cause network event registration to access
outside of array bounds (thankfully not a CVE, since domain
events were already doing the bounds check, and since network
events have not been released).
* daemon/libvirtd.h (daemonClientPrivate): Alter the tracking of
network events.
* daemon/remote.c (daemonClientEventCallback): New struct.
(remoteEventCallbackFree): New function.
(remoteClientInitHook, remoteRelayNetworkEventLifecycle)
(remoteClientFreeFunc)
(remoteDispatchConnectNetworkEventRegisterAny): Track network
callbacks differently.
(remoteDispatchConnectNetworkEventDeregisterAny): Enforce bounds.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1047659
If a VM dies very early during an attempted connect to the guest agent
while the locks are down the domain monitor object will be freed. The
object is then accessed later as any failure during guest agent startup
isn't considered fatal.
In the current upstream version this doesn't lead to a crash as
virObjectLock called when entering the monitor in
qemuProcessDetectVcpuPIDs checks the pointer before attempting to
dereference (lock) it. The NULL pointer is then caught in the monitor
helper code.
Before the introduction of virObjectLockable - observed on 0.10.2 - the
pointer is locked directly via virMutexLock leading to a crash.
To avoid this problem we need to differentiate between the guest agent
not being present and the VM quitting when the locks were down. The fix
reorganizes the code in qemuConnectAgent to add the check and then adds
special handling to the callers.
While working on v1.0.5-maint (the branch in use on Fedora 19)
with the host at Fedora 20, I got a failure in virstoragetest.
I traced it to the fact that we were using qemu-img to create a
qcow2 file, but qemu-img changed from creating v2 files by
default in F19 to creating v3 files in F20. Rather than leaving
it up to qemu-img, it is better to write the test to force
testing of BOTH file formats (better code coverage and all).
This patch alone does not fix all the failures in v1.0.5-maint;
for that, we must decide to either teach the older branch to
understand v3 files, or to reject them outright as unsupported.
But for upstream, making the test less dependent on changing
qemu-img defaults is always a good thing.
* tests/virstoragetest.c (testPrepImages): Simplify creation of
raw file; check if qemu supports compat and if so use it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Mitre tried to assign us two separate CVEs for the fix for
https://bugzilla.redhat.com/show_bug.cgi?id=1047577, on the
grounds that the fixes were separated by more than an hour
and thus triggered different hourly snapshots. But we
explicitly do NOT want to treat transient security bugs as
CVEs if they can only be triggered by patches in libvirt.git
but where the problem is cleaned up before a formal release.
Meanwhile, I noticed that while our wiki mentioned maintenance
branches and releases, our formal documentation did not.
* docs/downloads.html.in: Contrast hourly snapshots with
maintenance branches.
Signed-off-by: Eric Blake <eblake@redhat.com>
For a "newfd1" the coverity tools thinks that the fd is closed in
a "virCommandPassFD", but with "flags == 0" it cannot be closed.
The code itself is ok, but coverity tool thinks that there is
"double_close" of the "newfd1" and to prevent showing this error
we simply add a comment before the proper close.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Strings "file" and "context" may not be freed if "VIR_EXPAND_N" fails
and it leads into memory leak.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
A "xmlstr" string may not be assigned into a "doc" pointer and it
could cause memory leak. To fix it if the "doc" pointer is NULL and
the "xmlstr" string is not assigned we should free it.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
There could be a memory leak caused by "managed_system" string, if any
error occurs before "managed_system" is assigned into
"phyp_driver->managed_system". The "managed_system" string wouldn't be
freed at all. The better way is to free the "managed_system" instead
of the one assigned in the "phyp_driver".
This has been found by coverity.
Pointed out by John, that the "phyp_driver->xmlopt" needs to be
unreferenced as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: John Ferlan <jferlan@redhat.com>
If there is no error while executing a function "openvzParseBarrierLimit"
a "str" string where is duplicate of a "value" string isn't freed and it
leads into memory leak.
This has been found by coverity.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
I ran 'git add .' for a patch in progress, while in the middle
of running 'make check' to test my work, and was surprised when
it picked up some files I wasn't expecting.
* .gitignore: Ignore *.pem.
Signed-off-by: Eric Blake <eblake@redhat.com>
While running objecteventtest, it was found that valgrind pointed out the
following memory leak:
==125== 538 (56 direct, 482 indirect) bytes in 1 blocks are definitely lost in loss record 216 of 226
==125== at 0x4A06B6F: calloc (vg_replace_malloc.c:593)
==125== by 0x4C65D8D: virAllocVar (viralloc.c:558)
==125== by 0x4C9F055: virObjectNew (virobject.c:190)
==125== by 0x4D2B2E8: virGetDomain (datatypes.c:220)
==125== by 0x4D79180: testDomainDefineXML (test_driver.c:2962)
==125== by 0x4D4977D: virDomainDefineXML (libvirt.c:8512)
==125== by 0x4029C2: testDomainCreateXMLMixed (objecteventtest.c:226)
==125== by 0x403A21: virtTestRun (testutils.c:138)
==125== by 0x4021C2: mymain (objecteventtest.c:549)
==125== by 0x4040C2: virtTestMain (testutils.c:593)
==125== by 0x341F421A04: (below main) (libc-start.c:225)
Signed-off-by: Ján Tomko <jtomko@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1047577
When writing commit 173c291, I missed the fact virNetServerClientClose
unlocks the client object before actually clearing client->sock and thus
it is possible to hit a window when client->keepalive is NULL while
client->sock is not NULL. I was thinking client->sock == NULL was a
better check for a closed connection but apparently we have to go with
client->keepalive == NULL to actually fix the crash.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
On my Fedora 20 box with mingw cross-compiler, the build failed with:
../../src/rpc/virnetclient.c: In function 'virNetClientSetTLSSession':
../../src/rpc/virnetclient.c:745:14: error: unused variable 'oldmask' [-Werror=unused-variable]
sigset_t oldmask, blockedsigs;
^
I traced it to the fact that mingw64-winpthreads installs a header
that does #define pthread_sigmask(...) 0, which means any argument
only ever passed to pthread_sigmask is reported as unused. This
patch works around the compilation failure, with behavior no worse
than what mingw already gives us regarding the function being a
no-op.
* configure.ac (pthread_sigmask): Probe for broken mingw macro.
* src/util/virutil.h (pthread_sigmask): Rewrite to something that
avoids unused variables.
Signed-off-by: Eric Blake <eblake@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1047577
When a client closes its connection to libvirtd early during
virConnectOpen, more specifically just after making
REMOTE_PROC_CONNECT_SUPPORTS_FEATURE call to check if
VIR_DRV_FEATURE_PROGRAM_KEEPALIVE is supported without even waiting for
the result, libvirtd may crash due to a race in keep-alive
initialization. Once receiving the REMOTE_PROC_CONNECT_SUPPORTS_FEATURE
call, the daemon's event loop delegates it to a worker thread. In case
the event loop detects EOF on the connection and calls
virNetServerClientClose before the worker thread starts to handle
REMOTE_PROC_CONNECT_SUPPORTS_FEATURE call, client->keepalive will be
disposed by the time virNetServerClientStartKeepAlive gets called from
remoteDispatchConnectSupportsFeature. Because the flow is common for
both authenticated and read-only connections, even unprivileged clients
may cause the daemon to crash.
To avoid the crash, virNetServerClientStartKeepAlive needs to check if
the connection is still open before starting keep-alive protocol.
Every libvirt release since 0.9.8 is affected by this bug.
Any test suite which involves a virDomainDefPtr should
call virDomainDefCheckABIStability with itself just as
a basic sanity check that the identity-comparison always
succeeds. This would have caught the recent NULL pointer
access crash.
Make sure we cope with def->name being NULL since the
VMWare config parser produces NULL names.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
When idmap was added to LXC, we forgot to cover it in the testsuite.
The schema was missing an <element> layer, and as a result,
virt-xml-validate was failing on valid dumpxml output.
Reported by Eduard - Gabriel Munteanu on IRC.
* docs/schemas/domaincommon.rng (idmap): Include <idmap> element,
and support interleaves.
* tests/lxcxml2xmldata/lxc-idmap.xml: New file.
* tests/lxcxml2xmltest.c (mymain): Test it.
Signed-off-by: Eric Blake <eblake@redhat.com>
Ever since commit 61ac8ce, Coverity complained about
remoteNetworkBuildEventLifecycle not checking for NULL failure
to build an event, compared to other calls in the code base.
But the problem is latent from copy and paste; all 17 of our
remote*BuildEvent* functions in remote_driver.c have the same
issue - if an OOM causes an event to not be built, we happily
pass NULL to remoteEventQueue(), but that function has marked
event as a nonnull parameter. We were getting lucky (the
event queue's first use of the event happened to be a call to
virIsObjectClass(), which acts gracefully on NULL, so there
was no way to crash); but this is a latent bug waiting to bite
us due to the disregard for the nonnull attribute, as well as
a waste of resources in the event queue. Better is to just
refuse to queue NULL. The discard is silent, since the problem
only happens on OOM, and since events are already best effort -
if we fail to get an event, it's not like we have any memory
left to report the issue, nor any idea of who would benefit
from knowing we couldn't create or queue the event.
* src/remote/remote_driver.c (remoteEventQueue): Ignore NULL event.
Signed-off-by: Eric Blake <eblake@redhat.com>
Our fixes for CVE-2013-4400 were so effective at "fixing" bugs
in virt-login-shell that we ended up fixing it into a useless
do-nothing program.
Commit 3e2f27e1 picked the name LIBVIRT_SETUID_RPC_CLIENT for
the witness macro when we are doing secure compilation. But
commit 9cd6a57d checked whether the name IN_VIRT_LOGIN_SHELL,
from an earlier version of the patch series, was defined; with
the net result that virt-login-shell invariably detected that
it was setuid and failed virInitialize.
Commit b7fcc799 closed all fds larger than stderr, but in the
wrong place. Looking at the larger context, we mistakenly did
the close in between obtaining the set of namespace fds, then
actually using those fds to switch namespace, which means that
virt-login-shell will ALWAYS fail.
This is the minimal patch to fix the regressions, although
further patches are also worth having to clean up poor
semantics of the resulting program (for example, it is rude to
not pass on the exit status of the wrapped program back to the
invoking shell).
* tools/virt-login-shell.c (main): Don't close fds until after
namespace swap.
* src/libvirt.c (virGlobalInit): Use correct macro.
Signed-off-by: Eric Blake <eblake@redhat.com>
The existing check of domain snapshots validated that they
point to a domain, but did not validate that the domain
points to a connection, even though any errors blindly assume
the connection is valid. On the other hand, as mentioned in
commit 6e130ddc, any valid domain is already tied to a valid
connection, and VIR_IS_SNAPSHOT vs. VIR_IS_DOMAIN_SNAPSHOT
makes no real difference; it's best to just validate the chain
of all three. For consistency with previous patches, continue
the trend of using a common macro. For now, we don't need
virCheckDomainSnapshotGoto().
* src/datatypes.h (virCheckDomainSnapshotReturn): New macro.
(VIR_IS_SNAPSHOT, VIR_IS_DOMAIN_SNAPSHOT):
Drop unused macros.
* src/libvirt.c: Use macro throughout.
(virLibDomainSnapshotError): Drop unused macro.
Signed-off-by: Eric Blake <eblake@redhat.com>
While all errors related to invalid nwfilters appeared to be
consistent, we might as well continue the trend of using a
common macro. As in commit 6e130ddc, the difference between
VIR_IS_NWFILTER and VIR_IS_CONNECTED_NWFILTER is moot, since
reference counting means any valid nwfilter is also tied to
a valid connection. For now, we don't need virCheckNWFilterGoto().
* src/datatypes.h (virCheckNWFilterReturn): New macro.
(VIR_IS_NWFILTER, VIR_IS_CONNECTED_NWFILTER): Drop unused macros.
* src/libvirt.c: Use macro throughout.
(virLibNWFilterError): Drop unused macro.
Signed-off-by: Eric Blake <eblake@redhat.com>
