[libvirt] [PATCH] qemu: plug memory leak
https://bugzilla.redhat.com/show_bug.cgi?id=656795 * src/qemu/qemu_monitor.c (qemuMonitorFree): Also free the buffer. --- src/qemu/qemu_monitor.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 86ed82f..85d0d0f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -199,6 +199,7 @@ static void qemuMonitorFree(qemuMonitorPtr mon) if (virCondDestroy(&mon->notify) < 0) {} virMutexDestroy(&mon->lock); + VIR_FREE(mon->buffer); VIR_FREE(mon); } -- 1.7.3.2
Making this change makes it easier to spot the memory leaks that will be fixed in the next patch. * cfg.mk (sc_prohibit_xmlGetProp): New rule. * .x-sc_prohibit_xmlGetProp: New exception. * Makefile.am (EXTRA_DIST): Ship exception file. * tools/virsh.c (cmdDetachInterface, cmdDetachDisk): Adjust offenders. * src/conf/storage_conf.c (virStoragePoolDefParseSource): Likewise. * src/conf/network_conf.c (virNetworkDHCPRangeDefParseXML) (virNetworkIPParseXML): Likewise. --- valgrind picked up a memory leak in virNetworkDHCPRangeDefParseXML, but our use of non-idiomatic string management made it harder to see at first glance. This makes the few outliers consistent with all the rest of the code base. .x-sc_prohibit_xmlGetProp | 1 + Makefile.am | 1 + cfg.mk | 6 +++++ src/conf/network_conf.c | 56 ++++++++++++++++++++++---------------------- src/conf/storage_conf.c | 4 +- tools/virsh.c | 20 ++++++++-------- 6 files changed, 48 insertions(+), 40 deletions(-) create mode 100644 .x-sc_prohibit_xmlGetProp diff --git a/.x-sc_prohibit_xmlGetProp b/.x-sc_prohibit_xmlGetProp new file mode 100644 index 0000000..f6d7ee2 --- /dev/null +++ b/.x-sc_prohibit_xmlGetProp @@ -0,0 +1 @@ +^src/util/xml.c$ diff --git a/Makefile.am b/Makefile.am index d3f8876..e4d369d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -39,6 +39,7 @@ EXTRA_DIST = \ .x-sc_prohibit_strncpy \ .x-sc_prohibit_test_minus_ao \ .x-sc_prohibit_VIR_ERR_NO_MEMORY \ + .x-sc_prohibit_xmlGetProp \ .x-sc_require_config_h \ .x-sc_require_config_h_first \ .x-sc_trailing_blank \ diff --git a/cfg.mk b/cfg.mk index 0851f44..4c5afee 100644 --- a/cfg.mk +++ b/cfg.mk @@ -326,6 +326,12 @@ sc_prohibit_gethostby: halt='use getaddrinfo, not gethostby*' \ $(_sc_search_regexp) +# raw xmlGetProp requires some nasty casts +sc_prohibit_xmlGetProp: + @prohibit='\<xmlGetProp *\(' \ + halt='use virXMLPropString, not xmlGetProp' \ + $(_sc_search_regexp) + # Many of the function names below came from this filter: # git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \ # |sed 's/.*\.c- *//'|perl -pe 's/ ?\(.*//'|sort -u \ diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3f2bf1f..9868250 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -222,24 +222,24 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, virSocketAddr saddr, eaddr; int range; - if (!(start = (char *) xmlGetProp(cur, BAD_CAST "start"))) { + if (!(start = virXMLPropString(cur, "start"))) { cur = cur->next; continue; } - if (!(end = (char *) xmlGetProp(cur, BAD_CAST "end"))) { - xmlFree(start); + if (!(end = virXMLPropString(cur, "end"))) { + VIR_FREE(start); cur = cur->next; continue; } if (virSocketParseAddr(start, &saddr, AF_UNSPEC) < 0) { - xmlFree(start); - xmlFree(end); + VIR_FREE(start); + VIR_FREE(end); return -1; } if (virSocketParseAddr(end, &eaddr, AF_UNSPEC) < 0) { - xmlFree(start); - xmlFree(end); + VIR_FREE(start); + VIR_FREE(end); return -1; } @@ -248,14 +248,14 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, virNetworkReportError(VIR_ERR_XML_ERROR, _("dhcp range '%s' to '%s' invalid"), start, end); - xmlFree(start); - xmlFree(end); + VIR_FREE(start); + VIR_FREE(end); return -1; } if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) { - xmlFree(start); - xmlFree(end); + VIR_FREE(start); + VIR_FREE(end); virReportOOMError(); return -1; } @@ -264,19 +264,19 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, def->nranges++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { - xmlChar *mac, *name, *ip; + char *mac, *name, *ip; unsigned char addr[6]; virSocketAddr inaddr; - mac = xmlGetProp(cur, BAD_CAST "mac"); + mac = virXMLPropString(cur, "mac"); if ((mac != NULL) && - (virParseMacAddr((const char *) mac, &addr[0]) != 0)) { + (virParseMacAddr(mac, &addr[0]) != 0)) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse MAC address '%s'"), mac); VIR_FREE(mac); } - name = xmlGetProp(cur, BAD_CAST "name"); + name = virXMLPropString(cur, "name"); if ((name != NULL) && (!c_isalpha(name[0]))) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot use name address '%s'"), @@ -292,8 +292,8 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, cur = cur->next; continue; } - ip = xmlGetProp(cur, BAD_CAST "ip"); - if (virSocketParseAddr((const char *)ip, &inaddr, AF_UNSPEC) < 0) { + ip = virXMLPropString(cur, "ip"); + if (virSocketParseAddr(ip, &inaddr, AF_UNSPEC) < 0) { VIR_FREE(ip); VIR_FREE(mac); VIR_FREE(name); @@ -307,29 +307,29 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, virReportOOMError(); return -1; } - def->hosts[def->nhosts].mac = (char *)mac; - def->hosts[def->nhosts].name = (char *)name; + def->hosts[def->nhosts].mac = mac; + def->hosts[def->nhosts].name = name; def->hosts[def->nhosts].ip = inaddr; def->nhosts++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "bootp")) { - xmlChar *file; - xmlChar *server; + char *file; + char *server; virSocketAddr inaddr; memset(&inaddr, 0, sizeof(inaddr)); - if (!(file = xmlGetProp(cur, BAD_CAST "file"))) { + if (!(file = virXMLPropString(cur, "file"))) { cur = cur->next; continue; } - server = xmlGetProp(cur, BAD_CAST "server"); + server = virXMLPropString(cur, "server"); if (server && - virSocketParseAddr((const char *)server, &inaddr, AF_UNSPEC) < 0) + virSocketParseAddr(server, &inaddr, AF_UNSPEC) < 0) return -1; - def->bootfile = (char *)file; + def->bootfile = file; def->bootserver = inaddr; } @@ -354,14 +354,14 @@ virNetworkIPParseXML(virNetworkDefPtr def, } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "tftp")) { - xmlChar *root; + char *root; - if (!(root = xmlGetProp(cur, BAD_CAST "root"))) { + if (!(root = virXMLPropString(cur, "root"))) { cur = cur->next; continue; } - def->tftproot = (char *)root; + def->tftproot = root; } cur = cur->next; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 45285de..f7f471e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -446,14 +446,14 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, } for (i = 0 ; i < nsource ; i++) { - xmlChar *path = xmlGetProp(nodeset[i], BAD_CAST "path"); + char *path = virXMLPropString(nodeset[i], "path"); if (path == NULL) { VIR_FREE(nodeset); virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source device path")); goto cleanup; } - source->devices[i].path = (char *)path; + source->devices[i].path = path; } source->ndevice = nsource; } diff --git a/tools/virsh.c b/tools/virsh.c index 0ea1930..aec7749 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -8434,7 +8434,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) xmlXPathObjectPtr obj=NULL; xmlXPathContextPtr ctxt = NULL; xmlNodePtr cur = NULL; - xmlChar *tmp_mac = NULL; xmlBufferPtr xml_buf = NULL; char *doc, *mac =NULL, *type; char buf[64]; @@ -8485,10 +8484,11 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) for (; i < obj->nodesetval->nodeNr; i++) { cur = obj->nodesetval->nodeTab[i]->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "mac")) { - tmp_mac = xmlGetProp(cur, BAD_CAST "address"); - diff_mac = virMacAddrCompare ((char *) tmp_mac, mac); - xmlFree(tmp_mac); + if (cur->type == XML_ELEMENT_NODE && + xmlStrEqual(cur->name, BAD_CAST "mac")) { + char *tmp_mac = virXMLPropString(cur, "address"); + diff_mac = virMacAddrCompare (tmp_mac, mac); + VIR_FREE(tmp_mac); if (!diff_mac) { goto hit; } @@ -8689,7 +8689,6 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) xmlXPathObjectPtr obj=NULL; xmlXPathContextPtr ctxt = NULL; xmlNodePtr cur = NULL; - xmlChar *tmp_tgt = NULL; xmlBufferPtr xml_buf = NULL; virDomainPtr dom = NULL; char *doc, *target; @@ -8734,10 +8733,11 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) for (; i < obj->nodesetval->nodeNr; i++) { cur = obj->nodesetval->nodeTab[i]->children; while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "target")) { - tmp_tgt = xmlGetProp(cur, BAD_CAST "dev"); - diff_tgt = xmlStrEqual(tmp_tgt, BAD_CAST target); - xmlFree(tmp_tgt); + if (cur->type == XML_ELEMENT_NODE && + xmlStrEqual(cur->name, BAD_CAST "target")) { + char *tmp_tgt = virXMLPropString(cur, "dev"); + diff_tgt = STREQ(tmp_tgt, target); + VIR_FREE(tmp_tgt); if (diff_tgt) { goto hit; } -- 1.7.3.2
2010/11/24 Eric Blake <eblake@redhat.com>:
Making this change makes it easier to spot the memory leaks that will be fixed in the next patch.
* cfg.mk (sc_prohibit_xmlGetProp): New rule. * .x-sc_prohibit_xmlGetProp: New exception. * Makefile.am (EXTRA_DIST): Ship exception file. * tools/virsh.c (cmdDetachInterface, cmdDetachDisk): Adjust offenders. * src/conf/storage_conf.c (virStoragePoolDefParseSource): Likewise. * src/conf/network_conf.c (virNetworkDHCPRangeDefParseXML) (virNetworkIPParseXML): Likewise. ---
valgrind picked up a memory leak in virNetworkDHCPRangeDefParseXML, but our use of non-idiomatic string management made it harder to see at first glance. This makes the few outliers consistent with all the rest of the code base.
ACK. Matthias
* src/conf/network_conf.c (virNetworkDHCPRangeDefParseXML): Free xml strings when no longer referenced. --- Actually plug the leak. I've increased the context of this diff to make it easier to analyze. src/conf/network_conf.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9868250..b469269 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -233,123 +233,127 @@ virNetworkDHCPRangeDefParseXML(virNetworkDefPtr def, } if (virSocketParseAddr(start, &saddr, AF_UNSPEC) < 0) { VIR_FREE(start); VIR_FREE(end); return -1; } if (virSocketParseAddr(end, &eaddr, AF_UNSPEC) < 0) { VIR_FREE(start); VIR_FREE(end); return -1; } range = virSocketGetRange(&saddr, &eaddr); if (range < 0) { virNetworkReportError(VIR_ERR_XML_ERROR, _("dhcp range '%s' to '%s' invalid"), start, end); VIR_FREE(start); VIR_FREE(end); return -1; } + VIR_FREE(start); + VIR_FREE(end); if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) { - VIR_FREE(start); - VIR_FREE(end); virReportOOMError(); return -1; } def->ranges[def->nranges].start = saddr; def->ranges[def->nranges].end = eaddr; def->nranges++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { char *mac, *name, *ip; unsigned char addr[6]; virSocketAddr inaddr; mac = virXMLPropString(cur, "mac"); if ((mac != NULL) && (virParseMacAddr(mac, &addr[0]) != 0)) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot parse MAC address '%s'"), mac); VIR_FREE(mac); } name = virXMLPropString(cur, "name"); if ((name != NULL) && (!c_isalpha(name[0]))) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot use name address '%s'"), name); VIR_FREE(name); } /* * You need at least one MAC address or one host name */ if ((mac == NULL) && (name == NULL)) { VIR_FREE(mac); VIR_FREE(name); cur = cur->next; continue; } ip = virXMLPropString(cur, "ip"); if (virSocketParseAddr(ip, &inaddr, AF_UNSPEC) < 0) { VIR_FREE(ip); VIR_FREE(mac); VIR_FREE(name); cur = cur->next; continue; } + VIR_FREE(ip); if (VIR_REALLOC_N(def->hosts, def->nhosts + 1) < 0) { - VIR_FREE(ip); VIR_FREE(mac); VIR_FREE(name); virReportOOMError(); return -1; } def->hosts[def->nhosts].mac = mac; def->hosts[def->nhosts].name = name; def->hosts[def->nhosts].ip = inaddr; def->nhosts++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "bootp")) { char *file; char *server; virSocketAddr inaddr; memset(&inaddr, 0, sizeof(inaddr)); if (!(file = virXMLPropString(cur, "file"))) { cur = cur->next; continue; } server = virXMLPropString(cur, "server"); if (server && - virSocketParseAddr(server, &inaddr, AF_UNSPEC) < 0) + virSocketParseAddr(server, &inaddr, AF_UNSPEC) < 0) { + VIR_FREE(file); + VIR_FREE(server); return -1; + } def->bootfile = file; def->bootserver = inaddr; + VIR_FREE(server); } cur = cur->next; } return 0; } static int virNetworkIPParseXML(virNetworkDefPtr def, xmlNodePtr node) { xmlNodePtr cur; cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "dhcp")) { int result = virNetworkDHCPRangeDefParseXML(def, cur); if (result) return result; } else if (cur->type == XML_ELEMENT_NODE && -- 1.7.3.2
2010/11/24 Eric Blake <eblake@redhat.com>:
* src/conf/network_conf.c (virNetworkDHCPRangeDefParseXML): Free xml strings when no longer referenced. ---
Actually plug the leak. I've increased the context of this diff to make it easier to analyze.
src/conf/network_conf.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
ACK. Matthias
* src/storage/storage_backend.c (virStorageBackendUpdateVolTargetInfoFD): Avoid leak. --- Unlikely to bite in real life, but still a leak. src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a6ee564..10ea33c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1148,11 +1148,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } else { target->perms.label = strdup(filecon); + freecon(filecon); if (target->perms.label == NULL) { virReportOOMError(); return -1; } - freecon(filecon); } #else target->perms.label = NULL; -- 1.7.3.2
2010/11/24 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend.c (virStorageBackendUpdateVolTargetInfoFD): Avoid leak. ---
Unlikely to bite in real life, but still a leak.
src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a6ee564..10ea33c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1148,11 +1148,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } else { target->perms.label = strdup(filecon); + freecon(filecon); if (target->perms.label == NULL) { virReportOOMError(); return -1; } - freecon(filecon); } #else target->perms.label = NULL; -- 1.7.3.2
ACK. Matthias
On 11/24/2010 03:08 PM, Matthias Bolte wrote:
2010/11/24 Eric Blake <eblake@redhat.com>:
* src/storage/storage_backend.c (virStorageBackendUpdateVolTargetInfoFD): Avoid leak. ---
Unlikely to bite in real life, but still a leak.
src/storage/storage_backend.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a6ee564..10ea33c 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1148,11 +1148,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } else { target->perms.label = strdup(filecon); + freecon(filecon); if (target->perms.label == NULL) { virReportOOMError(); return -1; } - freecon(filecon); } #else target->perms.label = NULL; -- 1.7.3.2
ACK.
I've pushed the series through 5/n, although I may continue to extend it further as I review more valgrind logs (by the way, libnl is a beast for how frequently it leaks small strings). Meanwhile, as discussed with Matthew on IRC, I squashed 4 and 5 together before pushing, since 5 had the better commit message and those are the only two files to use security_context_t. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/11/24 Eric Blake <eblake@redhat.com>:
https://bugzilla.redhat.com/show_bug.cgi?id=656795
* src/qemu/qemu_monitor.c (qemuMonitorFree): Also free the buffer. --- src/qemu/qemu_monitor.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 86ed82f..85d0d0f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -199,6 +199,7 @@ static void qemuMonitorFree(qemuMonitorPtr mon) if (virCondDestroy(&mon->notify) < 0) {} virMutexDestroy(&mon->lock); + VIR_FREE(mon->buffer); VIR_FREE(mon); }
-- 1.7.3.2
ACK. Matthias
security_context_t happens to be a typedef for char*, and happens to begin with a string usable as a raw context string. But in reality, it is an opaque type that may or may not have additional information after the first NUL byte, where that additional information can include pointers that can only be freed via freecon(). Proof is from this valgrind run of daemon/libvirtd: ==6028== 839,169 (40 direct, 839,129 indirect) bytes in 1 blocks are definitely lost in loss record 274 of 274 ==6028== at 0x4A0515D: malloc (vg_replace_malloc.c:195) ==6028== by 0x3022E0D48C: selabel_open (label.c:165) ==6028== by 0x3022E11646: matchpathcon_init_prefix (matchpathcon.c:296) ==6028== by 0x3022E1190D: matchpathcon (matchpathcon.c:317) ==6028== by 0x4F9D842: SELinuxRestoreSecurityFileLabel (security_selinux.c:382) 800k is a lot of memory to be leaking. * src/security/security_selinux.c (SELinuxReserveSecurityLabel, SELinuxGetSecurityProcessLabel) (SELinuxRestoreSecurityFileLabel): Use correct function to free security_context_t. --- src/security/security_selinux.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 996177a..2a45172 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -239,7 +239,7 @@ SELinuxReserveSecurityLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } ctx = context_new(pctx); - VIR_FREE(pctx); + freecon(pctx); if (!ctx) goto err; @@ -298,11 +298,12 @@ SELinuxGetSecurityProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, _("security label exceeds " "maximum length: %d"), VIR_SECURITY_LABEL_BUFLEN - 1); + freecon(ctx); return -1; } strcpy(sec->label, (char *) ctx); - VIR_FREE(ctx); + freecon(ctx); sec->enforcing = security_getenforce(); if (sec->enforcing == -1) { @@ -387,7 +388,7 @@ SELinuxRestoreSecurityFileLabel(const char *path) } err: - VIR_FREE(fcon); + freecon(fcon); VIR_FREE(newpath); return rc; } -- 1.7.3.2
2010/11/24 Eric Blake <eblake@redhat.com>:
security_context_t happens to be a typedef for char*, and happens to begin with a string usable as a raw context string. But in reality, it is an opaque type that may or may not have additional information after the first NUL byte, where that additional information can include pointers that can only be freed via freecon().
Proof is from this valgrind run of daemon/libvirtd:
==6028== 839,169 (40 direct, 839,129 indirect) bytes in 1 blocks are definitely lost in loss record 274 of 274 ==6028== at 0x4A0515D: malloc (vg_replace_malloc.c:195) ==6028== by 0x3022E0D48C: selabel_open (label.c:165) ==6028== by 0x3022E11646: matchpathcon_init_prefix (matchpathcon.c:296) ==6028== by 0x3022E1190D: matchpathcon (matchpathcon.c:317) ==6028== by 0x4F9D842: SELinuxRestoreSecurityFileLabel (security_selinux.c:382)
800k is a lot of memory to be leaking.
* src/security/security_selinux.c (SELinuxReserveSecurityLabel, SELinuxGetSecurityProcessLabel) (SELinuxRestoreSecurityFileLabel): Use correct function to free security_context_t.
ACK. Matthias
On 11/24/2010 02:57 PM, Eric Blake wrote:
security_context_t happens to be a typedef for char*, and happens to begin with a string usable as a raw context string. But in reality, it is an opaque type that may or may not have additional information after the first NUL byte, where that additional information can include pointers that can only be freed via freecon().
Proof is from this valgrind run of daemon/libvirtd:
==6028== 839,169 (40 direct, 839,129 indirect) bytes in 1 blocks are definitely lost in loss record 274 of 274 ==6028== at 0x4A0515D: malloc (vg_replace_malloc.c:195) ==6028== by 0x3022E0D48C: selabel_open (label.c:165) ==6028== by 0x3022E11646: matchpathcon_init_prefix (matchpathcon.c:296) ==6028== by 0x3022E1190D: matchpathcon (matchpathcon.c:317) ==6028== by 0x4F9D842: SELinuxRestoreSecurityFileLabel (security_selinux.c:382)
800k is a lot of memory to be leaking.
* src/security/security_selinux.c (SELinuxReserveSecurityLabel, SELinuxGetSecurityProcessLabel) (SELinuxRestoreSecurityFileLabel): Use correct function to free security_context_t. --- src/security/security_selinux.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
Actually, it turns out that even with this patch, libvirtd is still suffering from the same severe 800k leak. libselinux has a bug where it uses __thread variables to store malloc'd memory, which is a no-no (since __thread has no destructor, you can't ever release the memory on pthread_exit(), in contrast to the heavier-weight pthread_key_create/pthread_getspecific interface). https://bugzilla.redhat.com/show_bug.cgi?id=658571 I'm wondering if adding a matchpathcon_fini() call would help reduce the leak [to pair up with the implicit matchpathcon_init(NULL) during our first use of matchpathcon()], although it won't eliminate it. Also, I peeked at the libselinux source; for now, security_context_t is just a NUL-terminated string grabbed from fgetxattr(XATTR_NAME_SELINUX), and freecon() is a thin wrapper around free(); but there's no guarantee that this won't change in a future version of the libselinux library. Therefore, using VIR_FREE() interchangeably with freecon() is not a bug today, but is not future-proof, so I'm still happy this patch went in. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* daemon/libvirtd.c (qemudRunLoop): Free any remaining client data. --- Since qemudCleanup calls VIR_FREE(server), it only makes sense to first free all of server's contents. Probably not the most important leak to plug (it only triggers at libvirtd exit, where the memory would be abandoned by process exit anyways, and does not affect clients that link against libvirt as a library), but plugging it makes leak analysis of the rest of libvirtd easier. daemon/libvirtd.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index caf51bf..791b3dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2403,6 +2403,10 @@ cleanup: server->workers[i].hasThread = 0; } VIR_FREE(server->workers); + for (i = 0; i < server->nclients; i++) + qemudFreeClient(server->clients[i]); + server->nclients = 0; + VIR_SHRINK_N(server->clients, server->nclients_max, server->nclients_max); virMutexUnlock(&server->lock); return NULL; -- 1.7.3.2
2010/11/30 Eric Blake <eblake@redhat.com>:
* daemon/libvirtd.c (qemudRunLoop): Free any remaining client data. ---
Since qemudCleanup calls VIR_FREE(server), it only makes sense to first free all of server's contents.
Probably not the most important leak to plug (it only triggers at libvirtd exit, where the memory would be abandoned by process exit anyways, and does not affect clients that link against libvirt as a library), but plugging it makes leak analysis of the rest of libvirtd easier.
daemon/libvirtd.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index caf51bf..791b3dc 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2403,6 +2403,10 @@ cleanup: server->workers[i].hasThread = 0; } VIR_FREE(server->workers); + for (i = 0; i < server->nclients; i++) + qemudFreeClient(server->clients[i]); + server->nclients = 0; + VIR_SHRINK_N(server->clients, server->nclients_max, server->nclients_max);
virMutexUnlock(&server->lock); return NULL; -- 1.7.3.2
ACK. Matthias
* src/qemu/qemu_driver.c (qemudShutdown): Free all strings and the ebtables structure. * src/libvirt_private.syms (ebtablesContextFree): Export missing symbol. * src/util/ebtables.c (ebtablesContextFree): Allow early exit. --- This leak triggers on every start/stop of a qemu domain, although it typically accounts for less than 1k leak per sequence. src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 13 ++++++++++--- src/util/ebtables.c | 2 ++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ef33f86..43df955 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -325,6 +325,7 @@ virDomainConfVMNWFilterTeardown; # ebtables.h ebtablesAddForwardAllowIn; ebtablesAddForwardPolicyReject; +ebtablesContextFree; ebtablesContextNew; ebtablesRemoveForwardAllowIn; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00d8a3..faab42a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2068,10 +2068,9 @@ qemudShutdown(void) { virSysinfoDefFree(qemu_driver->hostsysinfo); - VIR_FREE(qemu_driver->securityDriverName); - VIR_FREE(qemu_driver->logDir); VIR_FREE(qemu_driver->configDir); VIR_FREE(qemu_driver->autostartDir); + VIR_FREE(qemu_driver->logDir); VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->libDir); VIR_FREE(qemu_driver->cacheDir); @@ -2081,10 +2080,18 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); VIR_FREE(qemu_driver->vncSASLdir); - VIR_FREE(qemu_driver->saveImageFormat); + VIR_FREE(qemu_driver->spiceTLSx509certdir); + VIR_FREE(qemu_driver->spiceListen); + VIR_FREE(qemu_driver->spicePassword); VIR_FREE(qemu_driver->hugetlbfs_mount); VIR_FREE(qemu_driver->hugepage_path); + VIR_FREE(qemu_driver->securityDriverName); + VIR_FREE(qemu_driver->saveImageFormat); + VIR_FREE(qemu_driver->dumpImageFormat); + + ebtablesContextFree(qemu_driver->ebtables); + if (qemu_driver->cgroupDeviceACL) { for (i = 0 ; qemu_driver->cgroupDeviceACL[i] != NULL ; i++) VIR_FREE(qemu_driver->cgroupDeviceACL[i]); diff --git a/src/util/ebtables.c b/src/util/ebtables.c index f707756..e3b8da4 100644 --- a/src/util/ebtables.c +++ b/src/util/ebtables.c @@ -300,6 +300,8 @@ ebtablesContextNew(const char *driver) void ebtablesContextFree(ebtablesContext *ctx) { + if (!ctx) + return; if (ctx->input_filter) ebtRulesFree(ctx->input_filter); if (ctx->forward_filter) -- 1.7.3.2
2010/11/30 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (qemudShutdown): Free all strings and the ebtables structure. * src/libvirt_private.syms (ebtablesContextFree): Export missing symbol. * src/util/ebtables.c (ebtablesContextFree): Allow early exit. ---
This leak triggers on every start/stop of a qemu domain, although it typically accounts for less than 1k leak per sequence.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00d8a3..faab42a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2068,10 +2068,9 @@ qemudShutdown(void) {
virSysinfoDefFree(qemu_driver->hostsysinfo);
- VIR_FREE(qemu_driver->securityDriverName); - VIR_FREE(qemu_driver->logDir); VIR_FREE(qemu_driver->configDir); VIR_FREE(qemu_driver->autostartDir); + VIR_FREE(qemu_driver->logDir); VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->libDir); VIR_FREE(qemu_driver->cacheDir); @@ -2081,10 +2080,18 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); VIR_FREE(qemu_driver->vncSASLdir); - VIR_FREE(qemu_driver->saveImageFormat); + VIR_FREE(qemu_driver->spiceTLSx509certdir); + VIR_FREE(qemu_driver->spiceListen); + VIR_FREE(qemu_driver->spicePassword); VIR_FREE(qemu_driver->hugetlbfs_mount); VIR_FREE(qemu_driver->hugepage_path);
+ VIR_FREE(qemu_driver->securityDriverName);
Any specific reason for this empty line in this block of free calls?
+ VIR_FREE(qemu_driver->saveImageFormat); + VIR_FREE(qemu_driver->dumpImageFormat); + + ebtablesContextFree(qemu_driver->ebtables); + if (qemu_driver->cgroupDeviceACL) { for (i = 0 ; qemu_driver->cgroupDeviceACL[i] != NULL ; i++) VIR_FREE(qemu_driver->cgroupDeviceACL[i]);
ACK. Matthias
On 12/01/2010 09:44 AM, Matthias Bolte wrote:
2010/11/30 Eric Blake <eblake@redhat.com>:
* src/qemu/qemu_driver.c (qemudShutdown): Free all strings and the ebtables structure. * src/libvirt_private.syms (ebtablesContextFree): Export missing symbol. * src/util/ebtables.c (ebtablesContextFree): Allow early exit. ---
This leak triggers on every start/stop of a qemu domain, although it typically accounts for less than 1k leak per sequence.
VIR_FREE(qemu_driver->hugepage_path);
+ VIR_FREE(qemu_driver->securityDriverName);
Any specific reason for this empty line in this block of free calls?
Only because that was a place where the list of items being freed didn't directly correspond to the declaration order within the struct, whereas the earlier list had pretty much been done in the same order. Not much reason for either keeping or deleting it, since it's just cosmetic. So I deleted it. :)
ACK.
Thanks; I've pushed 6 and 7 now. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/security/security_selinux.c (SELinuxRestoreSecurityFileLabel): Use selabel_lookup instead of matchpathcon. Suggested by Daniel Walsh. --- Makes the huge difference that I originally thought I'd get with patch 5/n earlier in the series. Beforehand, when trying to start a single kvm guest then stopping libvirtd, valgrind reports: ==5584== LEAK SUMMARY: ==5584== definitely lost: 372 bytes in 13 blocks ==5584== indirectly lost: 0 bytes in 0 blocks ==5584== possibly lost: 349 bytes in 18 blocks after, it reports: ==7803== LEAK SUMMARY: ==7803== definitely lost: 412 bytes in 14 blocks ==7803== indirectly lost: 839,126 bytes in 11,265 blocks ==7803== possibly lost: 349 bytes in 18 blocks Obviously, I still haven't plugged everything, but this works around the fact that libselinux used __thread incorrectly for matchpathcon() caching. src/security/security_selinux.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2a45172..37539c2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -14,6 +14,7 @@ */ #include <config.h> #include <selinux/selinux.h> +#include <selinux/label.h> #include <selinux/context.h> #include <sys/types.h> #include <sys/stat.h> @@ -362,6 +363,7 @@ SELinuxRestoreSecurityFileLabel(const char *path) { struct stat buf; security_context_t fcon = NULL; + struct selabel_handle *handle = NULL; int rc = -1; char *newpath = NULL; char ebuf[1024]; @@ -380,14 +382,16 @@ SELinuxRestoreSecurityFileLabel(const char *path) goto err; } - if (matchpathcon(newpath, buf.st_mode, &fcon) == 0) { - rc = SELinuxSetFilecon(newpath, fcon); + if ((handle = selabel_open(SELABEL_CTX_FILE, NULL, 0)) == NULL || + selabel_lookup(handle, &fcon, newpath, buf.st_mode) < 0) { + VIR_WARN("cannot lookup default selinux label for %s", newpath); } else { - VIR_WARN("cannot lookup default selinux label for %s", - newpath); + rc = SELinuxSetFilecon(newpath, fcon); } err: + if (handle) + selabel_close(handle); freecon(fcon); VIR_FREE(newpath); return rc; -- 1.7.3.2
On 11/30/2010 08:54 PM, Eric Blake wrote:
* src/security/security_selinux.c (SELinuxRestoreSecurityFileLabel): Use selabel_lookup instead of matchpathcon. Suggested by Daniel Walsh.
https://bugzilla.redhat.com/show_bug.cgi?id=658657
@@ -380,14 +382,16 @@ SELinuxRestoreSecurityFileLabel(const char *path) goto err; }
- if (matchpathcon(newpath, buf.st_mode, &fcon) == 0) { - rc = SELinuxSetFilecon(newpath, fcon); + if ((handle = selabel_open(SELABEL_CTX_FILE, NULL, 0)) == NULL || + selabel_lookup(handle, &fcon, newpath, buf.st_mode) < 0) { + VIR_WARN("cannot lookup default selinux label for %s", newpath);
It may also make sense for a followup patch to do selabel_open once, as part of opening the security driver, and reusing the handle throughout the life of libvirtd, rather than repeatedly creating a new handle every time this function is called. But I ran out of time today to try that. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/12/1 Eric Blake <eblake@redhat.com>:
* src/security/security_selinux.c (SELinuxRestoreSecurityFileLabel): Use selabel_lookup instead of matchpathcon. Suggested by Daniel Walsh. ---
Makes the huge difference that I originally thought I'd get with patch 5/n earlier in the series. Beforehand, when trying to start a single kvm guest then stopping libvirtd, valgrind reports:
==5584== LEAK SUMMARY: ==5584== definitely lost: 372 bytes in 13 blocks ==5584== indirectly lost: 0 bytes in 0 blocks ==5584== possibly lost: 349 bytes in 18 blocks
after, it reports:
==7803== LEAK SUMMARY: ==7803== definitely lost: 412 bytes in 14 blocks ==7803== indirectly lost: 839,126 bytes in 11,265 blocks ==7803== possibly lost: 349 bytes in 18 blocks
Obviously, I still haven't plugged everything, but this works around the fact that libselinux used __thread incorrectly for matchpathcon() caching.
src/security/security_selinux.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
ACK. Matthias
On 12/13/2010 04:01 PM, Matthias Bolte wrote:
2010/12/1 Eric Blake <eblake@redhat.com>:
* src/security/security_selinux.c (SELinuxRestoreSecurityFileLabel): Use selabel_lookup instead of matchpathcon. Suggested by Daniel Walsh. ---
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 11/30/2010 08:54 PM, Eric Blake wrote:
* src/security/security_selinux.c (SELinuxRestoreSecurityFileLabel): Use selabel_lookup instead of matchpathcon. Suggested by Daniel Walsh. ---
#include <config.h> #include <selinux/selinux.h> +#include <selinux/label.h> #include <selinux/context.h> #include <sys/types.h> #include <sys/stat.h>
Phooey. This breaks on RHEL 5, where libselinux-devel-1.33.4 lacks <selinux/label.h>. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* configure.ac (with_selinux): Check for <selinux/label.h>. * src/security/security_selinux.c (getContext): New function. (SELinuxRestoreSecurityFileLabel): Use it to restore compilation when using older libselinux. --- Although this fixes a build-breaker on RHEL, I'd rather get it reviewed before pushing. Compilation on RHEL 5 also depends on https://www.redhat.com/archives/libvir-list/2010-December/msg00592.html along with the latest gnulib. configure.ac | 3 +++ src/security/security_selinux.c | 29 +++++++++++++++++++++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index 51e4271..a501721 100644 --- a/configure.ac +++ b/configure.ac @@ -1006,6 +1006,9 @@ fi if test "$with_selinux" = "yes"; then SELINUX_LIBS="-lselinux" AC_DEFINE_UNQUOTED([HAVE_SELINUX], 1, [whether basic SELinux functionality is available]) + dnl We prefer to use <selinux/label.h> and selabel_open, but can fall + dnl back to matchpathcon for the sake of RHEL 5's version of libselinux. + AC_CHECK_HEADERS([selinux/label.h]) fi AM_CONDITIONAL([HAVE_SELINUX], [test "$with_selinux" != "no"]) AC_SUBST([SELINUX_CFLAGS]) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 37539c2..1420a18 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -14,11 +14,13 @@ */ #include <config.h> #include <selinux/selinux.h> -#include <selinux/label.h> #include <selinux/context.h> #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#if HAVE_SELINUX_LABEL_H +# include <selinux/label.h> +#endif #include "security_driver.h" #include "security_selinux.h" @@ -355,6 +357,25 @@ SELinuxSetFilecon(const char *path, char *tcon) return 0; } +/* Set fcon to the appropriate label for path and mode, or return -1. */ +static int +getContext(const char *newpath, mode_t mode, security_context_t *fcon) +{ +#if HAVE_SELINUX_LABEL_H + struct selabel_handle *handle = selabel_open(SELABEL_CTX_FILE, NULL, 0); + int ret; + + if (handle == NULL) + return -1; + + ret = selabel_lookup(handle, fcon, newpath, mode); + selabel_close(handle); + return ret; +#else + return matchpathcon(newpath, mode, fcon); +#endif +} + /* This method shouldn't raise errors, since they'll overwrite * errors that the caller(s) are already dealing with */ @@ -363,7 +384,6 @@ SELinuxRestoreSecurityFileLabel(const char *path) { struct stat buf; security_context_t fcon = NULL; - struct selabel_handle *handle = NULL; int rc = -1; char *newpath = NULL; char ebuf[1024]; @@ -382,16 +402,13 @@ SELinuxRestoreSecurityFileLabel(const char *path) goto err; } - if ((handle = selabel_open(SELABEL_CTX_FILE, NULL, 0)) == NULL || - selabel_lookup(handle, &fcon, newpath, buf.st_mode) < 0) { + if (getContext(newpath, buf.st_mode, &fcon) < 0) { VIR_WARN("cannot lookup default selinux label for %s", newpath); } else { rc = SELinuxSetFilecon(newpath, fcon); } err: - if (handle) - selabel_close(handle); freecon(fcon); VIR_FREE(newpath); return rc; -- 1.7.3.3
participants (2)
-
Eric Blake -
Matthias Bolte