From cab938c993cb720daae047dcc0bafe1f339e841c Mon Sep 17 00:00:00 2001 From: Viktor Mihajlovski Date: Fri, 14 Dec 2012 16:08:24 +0100 Subject: [PATCH] S390: Fix virSysinfoRead memory corruption 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. The restructuring made it necessary to conditionally disable -Wlogical-op for some older GCC versions, using pragma GCC diagnostic. This is a GCC specific pragma, which is acceptable, since we're using it to work around a GCC specific bug. Finally, added a function virSysinfoSetup to configure the sysinfo data source files/script during run time, to facilitate writing test programs. This function is not published in sysinfo.h and only there for testing. Signed-off-by: Viktor Mihajlovski --- src/libvirt_private.syms | 1 + src/util/sysinfo.c | 195 ++++++++++++++++++++------------------- 2 files changed, 102 insertions(+), 94 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9288ad3a94..5907d1d54e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1186,6 +1186,7 @@ virStorageFileResize; virSysinfoDefFree; virSysinfoFormat; virSysinfoRead; +virSysinfoSetup; # threadpool.h diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index bac4b234b2..6a5db802ec 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -39,13 +39,30 @@ #define VIR_FROM_THIS VIR_FROM_SYSINFO -#define SYSINFO_SMBIOS_DECODER "dmidecode" -#define SYSINFO "/proc/sysinfo" -#define CPUINFO "/proc/cpuinfo" VIR_ENUM_IMPL(virSysinfo, VIR_SYSINFO_LAST, "smbios"); +static const char *sysinfoDmidecode = "dmidecode"; +static const char *sysinfoSysinfo = "/proc/sysinfo"; +static const char *sysinfoCpuinfo = "/proc/cpuinfo"; + +#define SYSINFO_SMBIOS_DECODER sysinfoDmidecode +#define SYSINFO sysinfoSysinfo +#define CPUINFO sysinfoCpuinfo + +/* only to be used test programs, therefore not in sysinfo.h */ +extern void virSysinfoSetup(const char *dmidecode, const char *sysinfo, + const char *cpuinfo); + +void virSysinfoSetup(const char *dmidecode, const char *sysinfo, + const char *cpuinfo) +{ + sysinfoDmidecode = dmidecode; + sysinfoSysinfo = sysinfo; + sysinfoCpuinfo = cpuinfo; +} + /** * virSysinfoDefFree: * @def: a sysinfo structure @@ -240,114 +257,103 @@ no_memory: #elif defined(__s390__) || defined(__s390x__) +/* + we need to ignore warnings about strchr caused by -Wlogical-op + for some GCC versions. + Doing it via a local pragma keeps the damage smaller than + disabling it on the package level. + Unfortunately, the affected GCCs don't allow diagnostic push/pop + which would have further reduced the impact. + */ +# if BROKEN_GCC_WLOGICALOP +# pragma GCC diagnostic ignored "-Wlogical-op" +# endif + +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 = strchr(start, delim1))) { + 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 +394,7 @@ virSysinfoRead(void) { return ret; no_memory: + virSysinfoDefFree(ret); VIR_FREE(outbuf); return NULL; }