From: Radoslaw Jablonski <radoslawx.jablonski@intel.com>
Date: Fri, 30 Mar 2018 15:31:00 +0000 (+0100)
Subject: virt: Fix memory handling for domains data
X-Git-Url: https://git.verplant.org/?a=commitdiff_plain;h=0a00498519b0fcb15bb4514823003a71d23afa56;p=collectd.git

virt: Fix memory handling for domains data

Memory with data for inactive domains was wrongly
freed earlier in refresh_lists() function and crash
was generated. Memory for active domains have
had the same problems with unnecessary free.
Also added handling for freeing inactive domain in
case of error.

Change-Id: I618798d9a369840be9ee596c96f12cbc1f7b24a6
Signed-off-by: Radoslaw Jablonski <radoslawx.jablonski@intel.com>
---

diff --git a/src/virt.c b/src/virt.c
index 99483c47..01f68b24 100644
--- a/src/virt.c
+++ b/src/virt.c
@@ -2271,6 +2271,8 @@ static int refresh_lists(struct lv_read_instance *inst) {
   for (int i = 0; i < m; ++i)
     if (add_domain(state, domains_inactive[i], 0) < 0) {
       ERROR(PLUGIN_NAME " plugin: malloc failed.");
+      virDomainFree(domains_inactive[i]);
+      domains_inactive[i] = NULL;
       continue;
     }
 #endif
@@ -2298,6 +2300,20 @@ static int refresh_lists(struct lv_read_instance *inst) {
     }
 #endif
 
+    if (add_domain(state, dom, 1) < 0) {
+      /*
+       * When domain is already tracked, then there is
+       * no problem with memory handling (will be freed
+       * with the rest of domains cached data)
+       * But in case of error like this (error occurred
+       * before adding domain to track) we have to take
+       * care it ourselves and call virDomainFree
+       */
+      ERROR(PLUGIN_NAME " plugin: malloc failed.");
+      virDomainFree(dom);
+      goto cont;
+    }
+
     name = virDomainGetName(dom);
     if (name == NULL) {
       VIRT_ERROR(conn, "virDomainGetName");
@@ -2343,11 +2359,6 @@ static int refresh_lists(struct lv_read_instance *inst) {
     if (!lv_instance_include_domain(inst, name, tag))
       goto cont;
 
-    if (add_domain(state, dom, 1) < 0) {
-      ERROR(PLUGIN_NAME " plugin: malloc failed.");
-      goto cont;
-    }
-
     /* Block devices. */
     const char *bd_xmlpath = "/domain/devices/disk/target[@dev]";
     if (blockdevice_format == source)
@@ -2438,11 +2449,10 @@ static int refresh_lists(struct lv_read_instance *inst) {
   }
 
 #ifdef HAVE_LIST_ALL_DOMAINS
-  for (int i = 0; i < n; ++i)
-    virDomainFree(domains[i]);
+  /* NOTE: domains_active and domains_inactive data will be cleared during
+     refresh of all domains (inside lv_clean_read_state function) so we need
+     to free here only allocated arrays */
   sfree(domains);
-  for (int i = 0; i < m; ++i)
-    virDomainFree(domains_inactive[i]);
   sfree(domains_inactive);
 #else
   sfree(domids);