From b9dacdd4d992ba1e5aab2e0189cf64b36a1a7e13 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) Signed-off-by: Eric Blake Conflicts: daemon/remote.c - context with older cleanup styles --- daemon/remote.c | 55 ++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index c93c97a326..fa5e9f5e2a 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1576,11 +1576,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++) virDomainFree(doms[i]); - VIR_FREE(doms); - } + VIR_FREE(doms); return rv; } @@ -4532,11 +4531,10 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, virNetMessageSaveError(rerr); if (dom) virDomainFree(dom); - if (snaps && nsnaps > 0) { + if (snaps && nsnaps > 0) for (i = 0; i < nsnaps; i++) virDomainSnapshotFree(snaps[i]); - VIR_FREE(snaps); - } + VIR_FREE(snaps); return rv; } @@ -4603,11 +4601,10 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU virDomainSnapshotFree(snapshot); if (dom) virDomainFree(dom); - if (snaps && nsnaps > 0) { + if (snaps && nsnaps > 0) for (i = 0; i < nsnaps; i++) virDomainSnapshotFree(snaps[i]); - VIR_FREE(snaps); - } + VIR_FREE(snaps); return rv; } @@ -4662,11 +4659,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++) virStoragePoolFree(pools[i]); - VIR_FREE(pools); - } + VIR_FREE(pools); return rv; } @@ -4725,11 +4721,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++) virStorageVolFree(vols[i]); - VIR_FREE(vols); - } + VIR_FREE(vols); if (pool) virStoragePoolFree(pool); return rv; @@ -4786,11 +4781,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++) virNetworkFree(nets[i]); - VIR_FREE(nets); - } + VIR_FREE(nets); return rv; } @@ -4845,11 +4839,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++) virInterfaceFree(ifaces[i]); - VIR_FREE(ifaces); - } + VIR_FREE(ifaces); return rv; } @@ -4904,11 +4897,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++) virNodeDeviceFree(devices[i]); - VIR_FREE(devices); - } + VIR_FREE(devices); return rv; } @@ -4963,11 +4955,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++) virNWFilterFree(filters[i]); - VIR_FREE(filters); - } + VIR_FREE(filters); return rv; } @@ -5022,11 +5013,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++) virSecretFree(secrets[i]); - VIR_FREE(secrets); - } + VIR_FREE(secrets); return rv; } @@ -6207,11 +6197,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); virNetworkFree(net); return rv; }