[libvirt] [PATCH v2 0/4] Extra validation for the <sysinfo> section
https://bugzilla.redhat.com/show_bug.cgi?id=890494 The <sysinfo> section needs an extra uuid validation check. The current check compares the numerical sysinfo/system_uuid with the domain uuid; however, it's possible that someone added extra hyphens into the sysinfo uuid which results in the inability to start the domain (at least qemu). Rather than fail due to improper format, adjust/save to the expected format. The bug report indicates the 'date' field should at least be syntax checked based on what's desribed in the SMBIOS spec. From the spec: "String number of the BIOS release date. The date string, if supplied, is in either mm/dd/yy or mm/dd/yyyy format. If the year portion of the string is two digits, the year is assumed to be 19yy. NOTE: The mm/dd/yyyy format is required for SMBIOS version 2.3 and later" v2 -> v1 differences: - Moved Sysinfo field validation checks from virDomainDefParseXML() into virSysinfoParseXML() - As long as the virUUIDParse() value matches the domain's UUID field save the 'correct' format in the system_uuid field - that is don't error out just because of extraneous space or dash in provided UUID. - Added tests for date field and uuid comparison parsing errors. NOTE: 01 and 02 are unchanged John Ferlan (4): docs: Fix syntax in sysinfo description docs: Update description of SMBIOS fields Validate the bios_date format for <sysinfo> Adjust improperly formatted <sysinfo> uuid docs/formatdomain.html.in | 51 +++++++++++--- src/conf/domain_conf.c | 78 ++++++++++++++++------ .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml | 23 +++++++ .../qemuxml2argv-smbios-uuid-match.xml | 23 +++++++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 148 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml -- 1.8.1.4
--- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9ade507..825053c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -308,7 +308,8 @@ </bios> <system> <entry name='manufacturer'>Fedora</entry> - <entry name='vendor'>Virt-Manager</entry> + <entry name='product'>Virt-Manager</entry> + <entry name='version'>0.9.4</entry> </system> </sysinfo> ...</pre> -- 1.8.1.4
On 05/13/2013 11:01 AM, John Ferlan wrote:
--- docs/formatdomain.html.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9ade507..825053c 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -308,7 +308,8 @@ </bios> <system> <entry name='manufacturer'>Fedora</entry> - <entry name='vendor'>Virt-Manager</entry> + <entry name='product'>Virt-Manager</entry> + <entry name='version'>0.9.4</entry> </system> </sysinfo> ...</pre>
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- docs/formatdomain.html.in | 48 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 825053c..abf14fa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -333,17 +333,49 @@ <dl> <dt><code>bios</code></dt> <dd> - This is block 0 of SMBIOS, with entry names drawn from - "vendor", "version", "date", and "release". + This is block 0 of SMBIOS, with entry names drawn from: + <dl> + <dt><code>vendor</code></dt> + <dd>BIOS Vendors Name</dd> + <dt><code>version</code></dt> + <dd>BIOS Version</dd> + <dt><code>date</code></dt> + <dd>BIOS release date. If supplied, is in either mm/dd/yy or + mm/dd/yyyy format. If the year portion of the string is + two digits, the year is assumed to be 19yy.</dd> + <dt><code>release</code></dt> + <dd>System BIOS Major and Minor release number values + concatenated together as one string separated by + a period, for example, 10.22.</dd> + </dl> </dd> <dt><code>system</code></dt> <dd> - This is block 1 of SMBIOS, with entry names drawn from - "manufacturer", "product", "version", "serial", "uuid", - "sku", and "family". If a "uuid" entry is provided - alongside a - top-level <a href="#elementsMetadata"><code>uuid</code> - element</a>, the two values must match. + This is block 1 of SMBIOS, with entry names drawn from: + <dl> + <dt><code>manufacturer</code></dt> + <dd>Manufacturer of BIOS</dd> + <dt><code>product</code></dt> + <dd>Product Name</dd> + <dt><code>version</code></dt> + <dd>Version of the product</dd> + <dt><code>serial</code></dt> + <dd>Serial number</dd> + <dt><code>uuid</code></dt> + <dd>Universal Unique ID number. If this entry is provided + alongside a top-level + <a href="#elementsMetadata"><code>uuid</code></a> element, + then the two values must match.</dd> + <dt><code>sku</code></dt> + <dd>SKU number to identify a particular configuration.</dd> + <dt><code>family</code></dt> + <dd>Identify the family a particular computer belongs to.</dd> + </dl> + NB: Incorrectly supplied entries in either the <code>bios</code> + or <code>system</code> blocks will be ignored without error. + Other than <code>uuid</code> validation and <code>date</code> + format checking, all values are passed as strings to the + hypervisor driver. </dd> </dl> </dd> -- 1.8.1.4
On 05/13/2013 11:01 AM, John Ferlan wrote:
--- docs/formatdomain.html.in | 48 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-)
Could probably squash with 1/4; but I'm fine either way. ACK with nits fixed.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 825053c..abf14fa 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -333,17 +333,49 @@ <dl> <dt><code>bios</code></dt> <dd> - This is block 0 of SMBIOS, with entry names drawn from - "vendor", "version", "date", and "release". + This is block 0 of SMBIOS, with entry names drawn from: + <dl> + <dt><code>vendor</code></dt> + <dd>BIOS Vendors Name</dd>
s/Vendors/Vendor's/ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add incorrectly formatted bios_date validation test --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml | 23 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 862b997..d352055 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8437,6 +8437,30 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(bios/entry[@name='version'])", ctxt); def->bios_date = virXPathString("string(bios/entry[@name='date'])", ctxt); + if (def->bios_date != NULL) { + char *ptr; + struct tm tm; + memset(&tm, 0, sizeof(tm)); + + /* Validate just the format of the date + * Expect mm/dd/yyyy or mm/dd/yy, + * where yy must be 00->99 and would be assumed to be 19xx + * a yyyy date should be 1900 and beyond + */ + if (virStrToLong_i(def->bios_date, &ptr, 10, &tm.tm_mon) < 0 || + *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 || + *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 || + *ptr != '\0' || + (tm.tm_mon < 0 || tm.tm_mon > 12) || + (tm.tm_mday < 0 || tm.tm_mday > 31) || + (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Invalid BIOS 'date' format")); + goto error; + } + } def->bios_release = virXPathString("string(bios/entry[@name='release'])", ctxt); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml new file mode 100644 index 0000000..7b2f33a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>smbios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <smbios mode="sysinfo"/> + </os> + <sysinfo type="smbios"> + <bios> + <entry name="date">999/999/123</entry> + </bios> + </sysinfo> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 98ceb83..0100937 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -815,6 +815,7 @@ mymain(void) DO_TEST("smbios", QEMU_CAPS_SMBIOS_TYPE); + DO_TEST_PARSE_ERROR("smbios-date", QEMU_CAPS_SMBIOS_TYPE); DO_TEST("watchdog", NONE); DO_TEST("watchdog-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); -- 1.8.1.4
On 05/13/2013 11:01 AM, John Ferlan wrote:
Add incorrectly formatted bios_date validation test --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml | 23 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 862b997..d352055 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8437,6 +8437,30 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(bios/entry[@name='version'])", ctxt); def->bios_date = virXPathString("string(bios/entry[@name='date'])", ctxt); + if (def->bios_date != NULL) { + char *ptr; + struct tm tm; + memset(&tm, 0, sizeof(tm)); + + /* Validate just the format of the date + * Expect mm/dd/yyyy or mm/dd/yy, + * where yy must be 00->99 and would be assumed to be 19xx + * a yyyy date should be 1900 and beyond + */ + if (virStrToLong_i(def->bios_date, &ptr, 10, &tm.tm_mon) < 0 || + *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 ||
Spaces around +
+ *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||
and again
+ *ptr != '\0' || + (tm.tm_mon < 0 || tm.tm_mon > 12) ||
According to 'man gmtime', tm_mon should be in the range 0-11 (you're allowing an off-by-one on the high end). Worse, January is 0, not 1; so you need to take the number parsed by the user and subtract one before passing it to struct tm.
+ (tm.tm_mday < 0 || tm.tm_mday > 31) ||
tm_mday should be in the range 1-31 (you're allowing an off-by-one on the low end)
+ (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) {
Oh, you aren't even using struct tm through a libc time-based function such as gmtime. In that case, my advice is to completely avoid 'struct tm'. It is such a non-intuitive struct (months start from 0, days from 1, years from 1900) that it should NOT be used for anything except the standardized functions that adhere to those conventions while converting between seconds since Epoch. Just declare 'int mon, day, year;' and parse into those variables, instead of trying to populate a struct tm that you then throw away.
+ virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Invalid BIOS 'date' format")); + goto error; + } + }
I like the approach of the patch, and especially that you added a test case, but I think it's still worth a v3 that avoids 'struct tm' and any chance of confusion it provides. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/13/2013 03:51 PM, Eric Blake wrote:
On 05/13/2013 11:01 AM, John Ferlan wrote:
Add incorrectly formatted bios_date validation test --- src/conf/domain_conf.c | 24 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml | 23 +++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 48 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 862b997..d352055 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8437,6 +8437,30 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(bios/entry[@name='version'])", ctxt); def->bios_date = virXPathString("string(bios/entry[@name='date'])", ctxt); + if (def->bios_date != NULL) { + char *ptr; + struct tm tm; + memset(&tm, 0, sizeof(tm)); + + /* Validate just the format of the date + * Expect mm/dd/yyyy or mm/dd/yy, + * where yy must be 00->99 and would be assumed to be 19xx + * a yyyy date should be 1900 and beyond + */ + if (virStrToLong_i(def->bios_date, &ptr, 10, &tm.tm_mon) < 0 || + *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 ||
Spaces around +
+ *ptr != '/' || + virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||
and again
+ *ptr != '\0' || + (tm.tm_mon < 0 || tm.tm_mon > 12) ||
According to 'man gmtime', tm_mon should be in the range 0-11 (you're allowing an off-by-one on the high end). Worse, January is 0, not 1; so you need to take the number parsed by the user and subtract one before passing it to struct tm.
+ (tm.tm_mday < 0 || tm.tm_mday > 31) ||
tm_mday should be in the range 1-31 (you're allowing an off-by-one on the low end)
+ (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) {
Oh, you aren't even using struct tm through a libc time-based function such as gmtime. In that case, my advice is to completely avoid 'struct tm'. It is such a non-intuitive struct (months start from 0, days from 1, years from 1900) that it should NOT be used for anything except the standardized functions that adhere to those conventions while converting between seconds since Epoch. Just declare 'int mon, day, year;' and parse into those variables, instead of trying to populate a struct tm that you then throw away.
+ virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Invalid BIOS 'date' format")); + goto error; + } + }
I like the approach of the patch, and especially that you added a test case, but I think it's still worth a v3 that avoids 'struct tm' and any chance of confusion it provides.
Oh right - at one time I was doing the gmtime or strptime call to attempt to validate the data; however, I found it was less than perfect allowing things like 2/31/2000 to be accepted. In any case, in lieu of a v3, here's a diff: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fbe4d9a..0516413 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8442,23 +8442,22 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(bios/entry[@name='date'])", ctxt); if (def->bios_date != NULL) { char *ptr; - struct tm tm; - memset(&tm, 0, sizeof(tm)); + int month, day, year; /* Validate just the format of the date * Expect mm/dd/yyyy or mm/dd/yy, * where yy must be 00->99 and would be assumed to be 19xx * a yyyy date should be 1900 and beyond */ - if (virStrToLong_i(def->bios_date, &ptr, 10, &tm.tm_mon) < 0 || + if (virStrToLong_i(def->bios_date, &ptr, 10, &month) < 0 || *ptr != '/' || - virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 || + virStrToLong_i(ptr + 1, &ptr, 10, &day) < 0 || *ptr != '/' || - virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 || + virStrToLong_i(ptr + 1, &ptr, 10, &year) < 0 || *ptr != '\0' || - (tm.tm_mon < 0 || tm.tm_mon > 12) || - (tm.tm_mday < 0 || tm.tm_mday > 31) || - (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + (month < 1 || month > 12) || + (day < 1 || day > 31) || + (year < 0 || (year >= 100 && year < 1900))) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("Invalid BIOS 'date' format")); goto error; or more visually appealing if (def->bios_date != NULL) { char *ptr; int month, day, year; /* Validate just the format of the date * Expect mm/dd/yyyy or mm/dd/yy, * where yy must be 00->99 and would be assumed to be 19xx * a yyyy date should be 1900 and beyond */ if (virStrToLong_i(def->bios_date, &ptr, 10, &month) < 0 || *ptr != '/' || virStrToLong_i(ptr + 1, &ptr, 10, &day) < 0 || *ptr != '/' || virStrToLong_i(ptr + 1, &ptr, 10, &year) < 0 || *ptr != '\0' || (month < 1 || month > 12) || (day < 1 || day > 31) || (year < 0 || (year >= 100 && year < 1900))) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("Invalid BIOS 'date' format")); goto error; } } John
On 05/14/2013 04:22 AM, John Ferlan wrote:
On 05/13/2013 03:51 PM, Eric Blake wrote:
On 05/13/2013 11:01 AM, John Ferlan wrote:
Add incorrectly formatted bios_date validation test ---
In any case, in lieu of a v3, here's a diff:
Thanks; that works.
or more visually appealing
if (def->bios_date != NULL) { char *ptr; int month, day, year;
/* Validate just the format of the date * Expect mm/dd/yyyy or mm/dd/yy, * where yy must be 00->99 and would be assumed to be 19xx * a yyyy date should be 1900 and beyond */ if (virStrToLong_i(def->bios_date, &ptr, 10, &month) < 0 || *ptr != '/' || virStrToLong_i(ptr + 1, &ptr, 10, &day) < 0 || *ptr != '/' || virStrToLong_i(ptr + 1, &ptr, 10, &year) < 0 || *ptr != '\0' || (month < 1 || month > 12) || (day < 1 || day > 31) || (year < 0 || (year >= 100 && year < 1900))) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("Invalid BIOS 'date' format")); goto error; } }
Indeed, that removes any confusion I had. ACK with this squashed in to the rest of your v3 patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
If the <sysinfo> system table 'uuid' field is improperly formatted, then qemu will fail to start the guest with the error: virsh start dom error: Failed to start domain dom error: internal error process exited while connecting to monitor: Invalid SMBIOS UUID string This was because the parsing rules were lax with respect to allowing extraneous spaces and dashes in the provided UUID. As long as there were 32 hexavalues that matched the UUID for the domain the string was accepted. However startup failed because the string format wasn't correct. This patch will adjust the string format so that when it's presented to the driver it's in the expected format. Addded a test for uuid comparison within sysinfo. --- src/conf/domain_conf.c | 54 ++++++++++++++-------- .../qemuxml2argv-smbios-uuid-match.xml | 23 +++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d352055..fbe4d9a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8401,10 +8401,13 @@ error: static virSysinfoDefPtr virSysinfoParseXML(const xmlNodePtr node, - xmlXPathContextPtr ctxt) + xmlXPathContextPtr ctxt, + unsigned char *domUUID, + bool uuid_generated) { virSysinfoDefPtr def; char *type; + char *tmpUUID = NULL; if (!xmlStrEqual(node->name, BAD_CAST "sysinfo")) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -8473,8 +8476,33 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(system/entry[@name='version'])", ctxt); def->system_serial = virXPathString("string(system/entry[@name='serial'])", ctxt); - def->system_uuid = - virXPathString("string(system/entry[@name='uuid'])", ctxt); + tmpUUID = virXPathString("string(system/entry[@name='uuid'])", ctxt); + if (tmpUUID) { + unsigned char uuidbuf[VIR_UUID_BUFLEN]; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + if (virUUIDParse(tmpUUID, uuidbuf) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + "%s", _("malformed <sysinfo> uuid element")); + goto error; + } + if (uuid_generated) + memcpy(domUUID, uuidbuf, VIR_UUID_BUFLEN); + else if (memcmp(domUUID, uuidbuf, VIR_UUID_BUFLEN) != 0) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("UUID mismatch between <uuid> and " + "<sysinfo>")); + goto error; + } + /* Although we've validated the UUID as good, virUUIDParse() is + * lax with respect to allowing extraneous "-" and " ", but the + * underlying hypervisor may be less forgiving. Use virUUIDFormat() + * to validate format in xml is right. If not, then format it + * properly so that it's used correctly later. + */ + virUUIDFormat(uuidbuf, uuidstr); + if (VIR_STRDUP(def->system_uuid, uuidstr) < 0) + goto error; + } def->system_sku = virXPathString("string(system/entry[@name='sku'])", ctxt); def->system_family = @@ -8482,6 +8510,7 @@ virSysinfoParseXML(const xmlNodePtr node, cleanup: VIR_FREE(type); + VIR_FREE(tmpUUID); return def; error: @@ -11737,27 +11766,12 @@ virDomainDefParseXML(xmlDocPtr xml, if ((node = virXPathNode("./sysinfo[1]", ctxt)) != NULL) { xmlNodePtr oldnode = ctxt->node; ctxt->node = node; - def->sysinfo = virSysinfoParseXML(node, ctxt); + def->sysinfo = virSysinfoParseXML(node, ctxt, + def->uuid, uuid_generated); ctxt->node = oldnode; if (def->sysinfo == NULL) goto error; - if (def->sysinfo->system_uuid != NULL) { - unsigned char uuidbuf[VIR_UUID_BUFLEN]; - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); - goto error; - } - if (uuid_generated) - memcpy(def->uuid, uuidbuf, VIR_UUID_BUFLEN); - else if (memcmp(def->uuid, uuidbuf, VIR_UUID_BUFLEN) != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("UUID mismatch between <uuid> and " - "<sysinfo>")); - goto error; - } - } } if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml new file mode 100644 index 0000000..322fb3e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml @@ -0,0 +1,23 @@ +<domain type='qemu'> + <name>smbios</name> + <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <smbios mode="sysinfo"/> + </os> + <sysinfo type="smbios"> + <system> + <entry name="uuid">a94b4335-6a14-8bc4-d6da-f7ea590b6816</entry> + </system> + </sysinfo> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0100937..92c72e0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -816,6 +816,7 @@ mymain(void) DO_TEST("smbios", QEMU_CAPS_SMBIOS_TYPE); DO_TEST_PARSE_ERROR("smbios-date", QEMU_CAPS_SMBIOS_TYPE); + DO_TEST_PARSE_ERROR("smbios-uuid-match", QEMU_CAPS_SMBIOS_TYPE); DO_TEST("watchdog", NONE); DO_TEST("watchdog-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); -- 1.8.1.4
On 05/13/2013 11:01 AM, John Ferlan wrote:
If the <sysinfo> system table 'uuid' field is improperly formatted, then qemu will fail to start the guest with the error:
virsh start dom error: Failed to start domain dom error: internal error process exited while connecting to monitor: Invalid SMBIOS UUID string
This was because the parsing rules were lax with respect to allowing extraneous spaces and dashes in the provided UUID. As long as there were 32 hexavalues that matched the UUID for the domain the string was accepted. However startup failed because the string format wasn't correct. This patch will adjust the string format so that when it's presented to the driver it's in the expected format.
Addded a test for uuid comparison within sysinfo.
s/Addded/Added/
--- src/conf/domain_conf.c | 54 ++++++++++++++-------- .../qemuxml2argv-smbios-uuid-match.xml | 23 +++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/13/2013 01:01 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=890494
The <sysinfo> section needs an extra uuid validation check. The current check compares the numerical sysinfo/system_uuid with the domain uuid; however, it's possible that someone added extra hyphens into the sysinfo uuid which results in the inability to start the domain (at least qemu). Rather than fail due to improper format, adjust/save to the expected format.
The bug report indicates the 'date' field should at least be syntax checked based on what's desribed in the SMBIOS spec. From the spec:
"String number of the BIOS release date. The date string, if supplied, is in either mm/dd/yy or mm/dd/yyyy format. If the year portion of the string is two digits, the year is assumed to be 19yy.
NOTE: The mm/dd/yyyy format is required for SMBIOS version 2.3 and later"
v2 -> v1 differences:
- Moved Sysinfo field validation checks from virDomainDefParseXML() into virSysinfoParseXML()
- As long as the virUUIDParse() value matches the domain's UUID field save the 'correct' format in the system_uuid field - that is don't error out just because of extraneous space or dash in provided UUID.
- Added tests for date field and uuid comparison parsing errors.
NOTE: 01 and 02 are unchanged
John Ferlan (4): docs: Fix syntax in sysinfo description docs: Update description of SMBIOS fields Validate the bios_date format for <sysinfo> Adjust improperly formatted <sysinfo> uuid
docs/formatdomain.html.in | 51 +++++++++++--- src/conf/domain_conf.c | 78 ++++++++++++++++------ .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml | 23 +++++++ .../qemuxml2argv-smbios-uuid-match.xml | 23 +++++++ tests/qemuxml2argvtest.c | 2 + 5 files changed, 148 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml
Thanks for the reviews - this has been pushed. John
participants (2)
-
Eric Blake -
John Ferlan