[libvirt] [PATCH] S390: Fix virSysinfoRead memory corruption

[Viktor Mihajlovski]

There was a double free issue caused by virSysinfoRead on s390,
as the same manufacturer string instance was assigned to more
than one processor record.
Cleaned up other potential memory issues and restructured the sysinfo
parsing code by moving repeating patterns into a helper function.

BTW: I hit an issue with using strchr(string,variable), as I am
still compiling with gcc 4.4.x, see
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513
I circumvented this using index(), which is deprecated, but working.

Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
---
 src/util/sysinfo.c |  160 ++++++++++++++++++++++-----------------------------
 1 files changed, 69 insertions(+), 91 deletions(-)

diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c
index bac4b23..0be5b75 100644
--- a/src/util/sysinfo.c
+++ b/src/util/sysinfo.c
@@ -240,114 +240,91 @@ no_memory:
 
 #elif defined(__s390__) || defined(__s390x__)
 
+static char *
+virSysinfoParseDelimited(const char *base, const char *name, char **value,
+                         char delim1, char delim2)
+{
+    const char *start;
+    char *end;
+
+    if (delim1 != delim2 &&
+        (start = strstr(base, name)) &&
+        (start = index(start, delim1))) { /* using index as strchr produces compile error on older gcc's - see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513 */
+        start += 1;
+        end = strchrnul(start, delim2);
+        virSkipSpaces(&start);
+        if (!((*value) = strndup(start, end - start))) {
+            virReportOOMError();
+            goto error;
+        }
+        virTrimSpaces(*value, NULL);
+        return end;
+    }
+
+error:
+    return NULL;
+}
+
+static char *
+virSysinfoParseLine(const char *base, const char *name, char **value)
+{
+    return virSysinfoParseDelimited(base, name, value, ':', '\n');
+}
+
 static int
 virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret)
 {
-    char *cur, *eol = NULL;
-    const char *property;
-
-    /* Return if Manufacturer field is not found */
-    if ((cur = strstr(base, "Manufacturer")) == NULL)
+    if (virSysinfoParseLine(base, "Manufacturer",
+                            &ret->system_manufacturer) &&
+        virSysinfoParseLine(base, "Type",
+                            &ret->system_family) &&
+        virSysinfoParseLine(base, "Sequence Code",
+                            &ret->system_serial))
         return 0;
-
-    base = cur;
-    if ((cur = strstr(base, "Manufacturer")) != NULL) {
-        cur = strchr(cur, ':') + 1;
-        eol = strchr(cur, '\n');
-        virSkipSpacesBackwards(cur, &eol);
-        if ((eol) && ((property = strndup(cur, eol - cur)) == NULL))
-            goto no_memory;
-        virSkipSpaces(&property);
-        ret->system_manufacturer = (char *) property;
-    }
-    if ((cur = strstr(base, "Type")) != NULL) {
-        cur = strchr(cur, ':') + 1;
-        eol = strchr(cur, '\n');
-        virSkipSpacesBackwards(cur, &eol);
-        if ((eol) && ((property = strndup(cur, eol - cur)) == NULL))
-            goto no_memory;
-        virSkipSpaces(&property);
-        ret->system_family = (char *) property;
-    }
-    if ((cur = strstr(base, "Sequence Code")) != NULL) {
-        cur = strchr(cur, ':') + 1;
-        eol = strchr(cur, '\n');
-        virSkipSpacesBackwards(cur, &eol);
-        if ((eol) && ((property = strndup(cur, eol - cur)) == NULL))
-            goto no_memory;
-        virSkipSpaces(&property);
-        ret->system_serial = (char *) property;
-    }
-
-    return 0;
-
-no_memory:
-    return -1;
+    else
+        return -1;
 }
 
 static int
 virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret)
 {
-    char *cur, *eol, *tmp_base;
-    char *manufacturer;
-    const char *tmp;
+    char *tmp_base;
+    char *manufacturer = NULL;
+    char *procline = NULL;
+    int result = -1;
     virSysinfoProcessorDefPtr processor;
 
-    if ((cur = strstr(base, "vendor_id")) != NULL) {
-        cur = strchr(cur, ':') + 1;
-        eol = strchr(cur, '\n');
-        virSkipSpacesBackwards(cur, &eol);
-        if ((eol) && ((tmp = strndup(cur, eol - cur)) == NULL))
-            goto no_memory;
-        virSkipSpaces(&tmp);
-        manufacturer = (char *) tmp;
-    }
-
-    /* Find processor N: line and gather the processor manufacturer, version,
-     * serial number, and family */
-    while ((tmp_base = strstr(base, "processor ")) != NULL) {
-        base = tmp_base;
-        eol = strchr(base, '\n');
-        cur = strchr(base, ':') + 1;
+    if (!(tmp_base=virSysinfoParseLine(base, "vendor_id", &manufacturer)))
+        goto cleanup;
 
+    /* Find processor N: line and gather the processor manufacturer,
+       version, serial number, and family */
+    while ((tmp_base = strstr(tmp_base, "processor "))
+           && (tmp_base = virSysinfoParseLine(tmp_base, "processor ",
+                                              &procline))) {
         if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) {
-            goto no_memory;
+            virReportOOMError();
+            goto cleanup;
         }
-
         processor = &ret->processor[ret->nprocessor - 1];
-
-        /* Set the processor manufacturer */
-        processor->processor_manufacturer = manufacturer;
-
-        if ((cur = strstr(base, "version =")) != NULL) {
-            cur += sizeof("version =");
-            eol = strchr(cur, ',');
-            if ((eol) &&
-                ((processor->processor_version = strndup(cur, eol - cur)) == NULL))
-                goto no_memory;
-        }
-        if ((cur = strstr(base, "identification =")) != NULL) {
-            cur += sizeof("identification =");
-            eol = strchr(cur, ',');
-            if ((eol) &&
-                ((processor->processor_serial_number = strndup(cur, eol - cur)) == NULL))
-                goto no_memory;
-        }
-        if ((cur = strstr(base, "machine =")) != NULL) {
-            cur += sizeof("machine =");
-            eol = strchr(cur, '\n');
-            if ((eol) &&
-                ((processor->processor_family = strndup(cur, eol - cur)) == NULL))
-                goto no_memory;
-        }
-
-        base = cur;
+        processor->processor_manufacturer = strdup(manufacturer);
+        if (!virSysinfoParseDelimited(procline, "version",
+                                      &processor->processor_version,
+                                      '=', ',') ||
+            !virSysinfoParseDelimited(procline, "identification",
+                                      &processor->processor_serial_number,
+                                      '=', ',') ||
+            !virSysinfoParseDelimited(procline, "machine",
+                                      &processor->processor_family,
+                                      '=', '\n'))
+            goto cleanup;
     }
+    result = 0;
 
-    return 0;
-
-no_memory:
-    return -1;
+cleanup:
+    VIR_FREE(manufacturer);
+    VIR_FREE(procline);
+    return result;
 }
 
 /* virSysinfoRead for s390x
@@ -388,6 +365,7 @@ virSysinfoRead(void) {
     return ret;
 
 no_memory:
+    virSysinfoDefFree(ret);
     VIR_FREE(outbuf);
     return NULL;
 }
-- 
1.7.0.4