[libvirt] [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. 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@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
On Fri, Dec 07, 2012 at 09:44:35AM +0100, Viktor Mihajlovski wrote:
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@linux.vnet.ibm.com> --- src/util/sysinfo.c | 160 ++++++++++++++++++++++----------------------------- 1 files changed, 69 insertions(+), 91 deletions(-)
Given the complexity the parsing it would be nice to add a test case for this. It is a shame we don't already have a test case for the sysinfo code in fact :-( I'd like to see test/virsysinfotest.c to validate this parsing. Take a 'char *str' containing representation data from /proc/sysinfo, run it through the parser & then validate the result. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/07/2012 10:44 AM, Daniel P. Berrange wrote:
1 files changed, 69 insertions(+), 91 deletions(-)
Given the complexity the parsing it would be nice to add a test case for this. It is a shame we don't already have a test case for the sysinfo code in fact :-( I'd like to see test/virsysinfotest.c to validate this parsing. Take a 'char *str' containing representation data from /proc/sysinfo, run it through the parser & then validate the result.
Daniel
Makes sense ... as this is platform-specific (ifdef'd) code: should I try to (by refactoring) make the s390 code testable on other platforms or do you think it's sufficient to have make check execute the test case on the target platform only, e.g. during RPM build. Either way: the checks for DMI-based sysinfo and PPC would have to be provided by the respective authors... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Dec 07, 2012 at 05:30:05PM +0100, Viktor Mihajlovski wrote:
On 12/07/2012 10:44 AM, Daniel P. Berrange wrote:
1 files changed, 69 insertions(+), 91 deletions(-)
Given the complexity the parsing it would be nice to add a test case for this. It is a shame we don't already have a test case for the sysinfo code in fact :-( I'd like to see test/virsysinfotest.c to validate this parsing. Take a 'char *str' containing representation data from /proc/sysinfo, run it through the parser & then validate the result.
Daniel
Makes sense ... as this is platform-specific (ifdef'd) code: should I try to (by refactoring) make the s390 code testable on other platforms or do you think it's sufficient to have make check execute the test case on the target platform only, e.g. during RPM build. Either way: the checks for DMI-based sysinfo and PPC would have to be provided by the respective authors...
I think we only need to test the code associated with the platform being built for. So just make the test case code you add be #ifdef s390 too, and x86 authors can follow up with their own impl later. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/07/2012 01:44 AM, Viktor Mihajlovski wrote:
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 seem to recall that we've hit this before. /me searches Yep - src/util/buf.c contains this gem: while (*cur != 0) { /* strchr work-around for gcc 4.3 & 4.4 bug with -Wlogical-op * http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36513 */ char needle[2] = { *cur, 0 }; if (strstr(toescape, needle)) *out++ = escape; *out++ = *cur; cur++; }
I circumvented this using index(), which is deprecated, but working.
No. Don't use index(), as it is not guaranteed to exist. What we should _really_ do is provide a change in m4/virt-compile-warnings.m4 that disables -Wlogical-op if compiling with a broken gcc, now that newer gcc doesn't force its stupidity on a sensible strchr() usage. I'll attempt that patch independently, and then your patch can go on top using strchr() from the get-go. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/07/2012 06:57 PM, Eric Blake wrote:
No. Don't use index(), as it is not guaranteed to exist.
What we should _really_ do is provide a change in m4/virt-compile-warnings.m4 that disables -Wlogical-op if compiling with a broken gcc, now that newer gcc doesn't force its stupidity on a sensible strchr() usage. I'll attempt that patch independently, and then your patch can go on top using strchr() from the get-go.
Good! I will need to add a testcase per Daniel's comment anyway, so there's time... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/07/2012 06:57 PM, Eric Blake wrote:
No. Don't use index(), as it is not guaranteed to exist.
What we should _really_ do is provide a change in m4/virt-compile-warnings.m4 that disables -Wlogical-op if compiling with a broken gcc, now that newer gcc doesn't force its stupidity on a sensible strchr() usage. I'll attempt that patch independently, and then your patch can go on top using strchr() from the get-go.
On a second thought I didn't like to disable the warning for all sources as I still would like to be informed about potential issues. I've send out a new version of the patch (in fact a new series) disabling the warning with a pragma only in the affected file. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 12/12/2012 09:10 AM, Viktor Mihajlovski wrote:
On 12/07/2012 06:57 PM, Eric Blake wrote:
No. Don't use index(), as it is not guaranteed to exist.
What we should _really_ do is provide a change in m4/virt-compile-warnings.m4 that disables -Wlogical-op if compiling with a broken gcc, now that newer gcc doesn't force its stupidity on a sensible strchr() usage. I'll attempt that patch independently, and then your patch can go on top using strchr() from the get-go.
On a second thought I didn't like to disable the warning for all sources as I still would like to be informed about potential issues.
Don't worry about it. There are enough people compiling with newer gcc where the warning is not disabled that you won't be introducing in problems.
I've send out a new version of the patch (in fact a new series) disabling the warning with a pragma only in the affected file.
But isn't the problem that the use of pragmas to silence broken warnings is specific to newer gcc, while you are trying to silence the warning on older gcc? I guess I'll have to look at what you did; and I still need to work up my patch for the warning avoidance at configure time anyway. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/12/2012 05:23 PM, Eric Blake wrote:
Don't worry about it. There are enough people compiling with newer gcc where the warning is not disabled that you won't be introducing in problems.
heard that too often ;-)
I've send out a new version of the patch (in fact a new series) disabling the warning with a pragma only in the affected file.
But isn't the problem that the use of pragmas to silence broken warnings is specific to newer gcc, while you are trying to silence the warning on older gcc? I guess I'll have to look at what you did; and I still need to work up my patch for the warning avoidance at configure time anyway.
obviously I can't vouch for it ... but according to the GCC manuals, -Wlogical-op was introduced in gcc 4.3 while #pragma GCC diagnostic showed up in gcc 4.2, so we should be safe. But then again, I said that too often as well ... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Viktor Mihajlovski