From b175298be8e92eb3d7883fa3927b97db04d449be Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 13 Mar 2015 17:01:43 -0600 Subject: [PATCH] daemon: avoid memleak when ListAll returns nothing Commit 4f25146 (v1.2.8) managed to silence Coverity, but at the cost of a memory leak detected by valgrind: ==24129== 40 bytes in 5 blocks are definitely lost in loss record 355 of 637 ==24129== at 0x4A08B1C: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==24129== by 0x5084B8E: virReallocN (viralloc.c:245) ==24129== by 0x514D5AA: virDomainObjListExport (domain_conf.c:22200) ==24129== by 0x201227DB: qemuConnectListAllDomains (qemu_driver.c:18042) ==24129== by 0x51CC1B6: virConnectListAllDomains (libvirt-domain.c:6797) ==24129== by 0x14173D: remoteDispatchConnectListAllDomains (remote.c:1580) ==24129== by 0x121BE1: remoteDispatchConnectListAllDomainsHelper (remote_dispatch.h:1072) In short, every time a client calls a ListAll variant and asks for the resulting list, but there are 0 elements to return, we end up leaking the 1-entry array that holds the NULL terminator. What's worse, a read-only client can access these functions in a tight loop to cause libvirtd to eventually run out of memory; and this can be considered a denial of service attack against more privileged clients. Thankfully, the leak is so small (8 bytes per call) that you would already have some other denial of service with any guest calling the API that frequently, so an out-of-memory crash is unlikely enough that this did not warrant a CVE. * daemon/remote.c (remoteDispatchConnectListAllDomains) (remoteDispatchDomainListAllSnapshots) (remoteDispatchDomainSnapshotListAllChildren) (remoteDispatchConnectListAllStoragePools) (remoteDispatchStoragePoolListAllVolumes) (remoteDispatchConnectListAllNetworks) (remoteDispatchConnectListAllInterfaces) (remoteDispatchConnectListAllNodeDevices) (remoteDispatchConnectListAllNWFilters) (remoteDispatchConnectListAllSecrets) (remoteDispatchNetworkGetDHCPLeases): Plug leak. Signed-off-by: Eric Blake (cherry picked from commit 3c2ff5029b83c9b33be0f1607a3c61f4f5850612) --- daemon/remote.c | 55 ++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index d657a09d8a..9431cc398f 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1609,11 +1609,10 @@ remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (doms && ndomains > 0) { + if (doms && ndomains > 0) for (i = 0; i < ndomains; i++) virObjectUnref(doms[i]); - VIR_FREE(doms); - } + VIR_FREE(doms); return rv; } @@ -4533,11 +4532,10 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); - if (snaps && nsnaps > 0) { + if (snaps && nsnaps > 0) for (i = 0; i < nsnaps; i++) virObjectUnref(snaps[i]); - VIR_FREE(snaps); - } + VIR_FREE(snaps); return rv; } @@ -4602,11 +4600,10 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU virNetMessageSaveError(rerr); virObjectUnref(snapshot); virObjectUnref(dom); - if (snaps && nsnaps > 0) { + if (snaps && nsnaps > 0) for (i = 0; i < nsnaps; i++) virObjectUnref(snaps[i]); - VIR_FREE(snaps); - } + VIR_FREE(snaps); return rv; } @@ -4661,11 +4658,10 @@ remoteDispatchConnectListAllStoragePools(virNetServerPtr server ATTRIBUTE_UNUSED cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (pools && npools > 0) { + if (pools && npools > 0) for (i = 0; i < npools; i++) virObjectUnref(pools[i]); - VIR_FREE(pools); - } + VIR_FREE(pools); return rv; } @@ -4724,11 +4720,10 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (vols && nvols > 0) { + if (vols && nvols > 0) for (i = 0; i < nvols; i++) virObjectUnref(vols[i]); - VIR_FREE(vols); - } + VIR_FREE(vols); virObjectUnref(pool); return rv; } @@ -4784,11 +4779,10 @@ remoteDispatchConnectListAllNetworks(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (nets && nnets > 0) { + if (nets && nnets > 0) for (i = 0; i < nnets; i++) virObjectUnref(nets[i]); - VIR_FREE(nets); - } + VIR_FREE(nets); return rv; } @@ -4843,11 +4837,10 @@ remoteDispatchConnectListAllInterfaces(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (ifaces && nifaces > 0) { + if (ifaces && nifaces > 0) for (i = 0; i < nifaces; i++) virObjectUnref(ifaces[i]); - VIR_FREE(ifaces); - } + VIR_FREE(ifaces); return rv; } @@ -4902,11 +4895,10 @@ remoteDispatchConnectListAllNodeDevices(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (devices && ndevices > 0) { + if (devices && ndevices > 0) for (i = 0; i < ndevices; i++) virObjectUnref(devices[i]); - VIR_FREE(devices); - } + VIR_FREE(devices); return rv; } @@ -4961,11 +4953,10 @@ remoteDispatchConnectListAllNWFilters(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (filters && nfilters > 0) { + if (filters && nfilters > 0) for (i = 0; i < nfilters; i++) virObjectUnref(filters[i]); - VIR_FREE(filters); - } + VIR_FREE(filters); return rv; } @@ -5020,11 +5011,10 @@ remoteDispatchConnectListAllSecrets(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (secrets && nsecrets > 0) { + if (secrets && nsecrets > 0) for (i = 0; i < nsecrets; i++) virObjectUnref(secrets[i]); - VIR_FREE(secrets); - } + VIR_FREE(secrets); return rv; } @@ -6192,11 +6182,10 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) virNetMessageSaveError(rerr); - if (leases && nleases > 0) { + if (leases && nleases > 0) for (i = 0; i < nleases; i++) virNetworkDHCPLeaseFree(leases[i]); - VIR_FREE(leases); - } + VIR_FREE(leases); virObjectUnref(net); return rv; }