[libvirt] [PATCH 0/4] Extra validation for the <sysinfo> section
Not necessary for 1.0.5, but figured I'd put it out there now. 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). 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" John Ferlan (4): docs: Fix syntax in sysinfo description docs: Update description of SMBIOS fields Validate the bios_date format for <sysinfo> Need better validation of <sysinfo> uuid docs/formatdomain.html.in | 51 ++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 13 deletions(-) -- 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 f325c3c..017d2b9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -305,7 +305,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 04/30/2013 08:19 PM, John Ferlan wrote:
--- 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 f325c3c..017d2b9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -305,7 +305,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>
ACK, that was probably just an old copy-paste error. Martin
--- 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 017d2b9..2e16a64 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -330,17 +330,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 04/30/2013 08:19 PM, John Ferlan wrote:
--- docs/formatdomain.html.in | 48 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 8 deletions(-)
ACK, also looks much nicer, Martin
--- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..43273f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + 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(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_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid BIOS 'date' format: %s"), + def->sysinfo->bios_date); + goto error; + } + } } if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { -- 1.8.1.4
On 04/30/2013 08:19 PM, John Ferlan wrote:
--- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..43273f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + 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(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_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Seems like another abuse of internal error, but I don't know what to use here, properly. Maybe VIR_ERR_XML_DETAIL?
+ _("Invalid BIOS 'date' format: %s"), + def->sysinfo->bios_date);
Unnecessarily long, you can do 's/def->sysinfo->bios_//' and save one line here ;-)
+ goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
I find it a bit harder to read. Wouldn't this be more nicer if we used sscanf()? Or we could take care a bit about the date and do it even shorter with strptime(), something like this: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d55ce6b..61f385c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11588,6 +11588,20 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + char *end; + struct tm tm; + memset(&tm, 0, sizeof(struct tm)); + + end = strptime(date, "%D", &tm); + + if (!end || *end != '\0') { + virReportError(VIR_ERR_XML_DETAIL, + _("Invalid BIOS 'date' format: %s"), date); + goto error; + } + } } if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { -- Or should we allow even dates like "99/99/9999"? Martin P.S.: I don't mean to be rude with nit-picking, but a test for that would be nice ;-)
On 05/09/2013 06:59 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, John Ferlan wrote:
--- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..43273f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + 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(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_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Seems like another abuse of internal error, but I don't know what to use here, properly. Maybe VIR_ERR_XML_DETAIL?
+ _("Invalid BIOS 'date' format: %s"), + def->sysinfo->bios_date);
Unnecessarily long, you can do 's/def->sysinfo->bios_//' and save one line here ;-)
+ goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
FYI: The above is essentially a cut-n-reformat for this particular need of virDomainGraphicsAuthDefParseXML(). And while I agree it's an eye strain to read - I also tried various strptime() formats then using strftime() to format it back..
I find it a bit harder to read. Wouldn't this be more nicer if we used sscanf()? Or we could take care a bit about the date and do it even shorter with strptime(), something like this:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d55ce6b..61f385c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11588,6 +11588,20 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + char *end; + struct tm tm; + memset(&tm, 0, sizeof(struct tm)); + + end = strptime(date, "%D", &tm);
I did try using strptime() in order to validate, but it was far from perfect, although easier to read... The %D is the equivalent to %m/%d/%y which doesn't work when the date is presented as "5/9/2013" a resulting strftime() provides "05/09/20". The "best" format has been "%m/%d/%Y" and it's perfectly reasonable to use it rather than the virStrToLong_i() calls. The purpose for the tm_year validation/check comes from the spec which has requirement regarding using 'yy' vs. 'yyyy'. In particular, is 1/1/1850 a valid date? Well yes, technically according to strptime(), but not necessarily "right" according to the spec. There is an SMBIOS spec which describes the various fields and their requirements. See page 28 of the following: http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf
+ + if (!end || *end != '\0') { + virReportError(VIR_ERR_XML_DETAIL, + _("Invalid BIOS 'date' format: %s"), date); + goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { --
Or should we allow even dates like "99/99/9999"?
Which would fail using strptime(), but not the above algorithm.
Martin
P.S.: I don't mean to be rude with nit-picking, but a test for that would be nice ;-)
Nit picking is fine - wasn't quite sure where to put a test on something like this. John
On 05/09/2013 01:43 PM, John Ferlan wrote:
On 05/09/2013 06:59 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, John Ferlan wrote:
--- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..43273f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + 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(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_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Seems like another abuse of internal error, but I don't know what to use here, properly. Maybe VIR_ERR_XML_DETAIL?
+ _("Invalid BIOS 'date' format: %s"), + def->sysinfo->bios_date);
Unnecessarily long, you can do 's/def->sysinfo->bios_//' and save one line here ;-)
+ goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
FYI: The above is essentially a cut-n-reformat for this particular need of virDomainGraphicsAuthDefParseXML(). And while I agree it's an eye strain to read - I also tried various strptime() formats then using strftime() to format it back..
I haven't seen it being used somewhere else, but makes sense also due to the rest of the mail.
I find it a bit harder to read. Wouldn't this be more nicer if we used sscanf()? Or we could take care a bit about the date and do it even shorter with strptime(), something like this:
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d55ce6b..61f385c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11588,6 +11588,20 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + char *end; + struct tm tm; + memset(&tm, 0, sizeof(struct tm)); + + end = strptime(date, "%D", &tm);
I did try using strptime() in order to validate, but it was far from perfect, although easier to read...
The %D is the equivalent to %m/%d/%y which doesn't work when the date is presented as "5/9/2013" a resulting strftime() provides "05/09/20". The "best" format has been "%m/%d/%Y" and it's perfectly reasonable to use it rather than the virStrToLong_i() calls.
I was sure that %y can take both 2 and 4 digit year numbers, but after trying that one more time, you're right.
The purpose for the tm_year validation/check comes from the spec which has requirement regarding using 'yy' vs. 'yyyy'. In particular, is 1/1/1850 a valid date? Well yes, technically according to strptime(), but not necessarily "right" according to the spec.
There is an SMBIOS spec which describes the various fields and their requirements. See page 28 of the following:
http://dmtf.org/sites/default/files/standards/documents/DSP0134_2.8.0.pdf
+ + if (!end || *end != '\0') { + virReportError(VIR_ERR_XML_DETAIL, + _("Invalid BIOS 'date' format: %s"), date); + goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) { --
Or should we allow even dates like "99/99/9999"?
Which would fail using strptime(), but not the above algorithm.
Yes, that's why I asked, but I definitely don't insist on such strict checking. I haven't thought my proposal through enough and what you say makes more sense, so ACK. Feel free to squash in the test proposed below as an ACK from your side.
Martin
P.S.: I don't mean to be rude with nit-picking, but a test for that would be nice ;-)
Nit picking is fine - wasn't quite sure where to put a test on something like this.
I'd add it to qemuxml2argvtest with invalid date and DO_TEST_PARSE_ERROR. Example (feel free to use it, it's tested): 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 1286273..7d5c3d0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -814,6 +814,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); -- Martin
On 05/09/2013 09:58 AM, Martin Kletzander wrote:
On 05/09/2013 01:43 PM, John Ferlan wrote:
On 05/09/2013 06:59 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, John Ferlan wrote:
--- src/conf/domain_conf.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a8b5dfd..43273f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11591,6 +11591,30 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } } + if (def->sysinfo->bios_date != NULL) { + char *date = def->sysinfo->bios_date; + 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(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_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) { + virReportError(VIR_ERR_INTERNAL_ERROR,
Seems like another abuse of internal error, but I don't know what to use here, properly. Maybe VIR_ERR_XML_DETAIL?
+ _("Invalid BIOS 'date' format: %s"), + def->sysinfo->bios_date);
Unnecessarily long, you can do 's/def->sysinfo->bios_//' and save one line here ;-)
+ goto error; + } + } }
if ((tmp = virXPathString("string(./os/smbios/@mode)", ctxt))) {
FYI: The above is essentially a cut-n-reformat for this particular need of virDomainGraphicsAuthDefParseXML(). And while I agree it's an eye strain to read - I also tried various strptime() formats then using strftime() to format it back..
I haven't seen it being used somewhere else, but makes sense also due to the rest of the mail.
I guess I agree in principal that a month of 99 or a date of 99 would be incorrect and since we're doing some sort of validation it wouldn't hurt to do a bit more. Doing full blown is the days in the month right and handling leap year - is just outside the realm. My guess is that somewhere some code will do a similar strptime() like call anyway. So I made the change: *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, - _("Invalid BIOS 'date' format: %s"), - def->sysinfo->bios_date); + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("Invalid BIOS 'date' format"));
... And did squash/add the test provided - thanks! I also tried a couple of other dates (both good and bad) during self testing to make sure the code validated properly... Going to do something similar with uuid validation shortly... John
I'd add it to qemuxml2argvtest with invalid date and DO_TEST_PARSE_ERROR.
Example (feel free to use it, it's tested):
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 1286273..7d5c3d0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -814,6 +814,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); --
Martin
On 05/09/2013 05:43 AM, John Ferlan wrote:
On 05/09/2013 06:59 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, John Ferlan wrote: I find it a bit harder to read. Wouldn't this be more nicer if we used sscanf()? Or we could take care a bit about the date and do it even shorter with strptime(), something like this:
strptime() is not portable - mingw lacks it, and gnulib doesn't provide it (gnulib prefers to use the parse-datetime module used by coreutils, but that's GPL, so we can't use it either). We're stuck with hand-rolled parsing. -- 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 In this case the "system_uuid" field was a94b4335-6a14-8bc4-d6da-f7ea590b68-16 which passed the virUUIDParse() code because 32 hexadecimal digits were found and the extra hyphen in the last section was ignored. Add checks to not only parse the read field, but then use virUUIDFormat() to validate that what gets formatted matches what was read - if not, then fail the edit. --- src/conf/domain_conf.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43273f8..c1fd99b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11577,17 +11577,32 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->sysinfo->system_uuid != NULL) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + /* Ensure that what we convert to a uuidbuf is converted back to + * the same string when formatted as a UUID. This field may be + * used by the underlying hypervisor driver instead of the domain + * uuid field and must be properly formatted. The virUUIDParse() + * is designed to "skip" extra "-"'s in the values and only + * validate that there are 32 hexadecimal digits. virUUIDFormat() + * returns uuidstr formatted properly. + */ + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0 || + memcmp(def->sysinfo->system_uuid, + virUUIDFormat(uuidbuf, uuidstr), + VIR_UUID_STRING_BUFLEN) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); + _("malformed <sysinfo> uuid element '%s' " + "found on '%s'"), + def->sysinfo->system_uuid, def->name); 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", + virReportError(VIR_ERR_INTERNAL_ERROR, _("UUID mismatch between <uuid> and " - "<sysinfo>")); + "<sysinfo> 'uuid' <entry> on '%s'"), + def->name); goto error; } } -- 1.8.1.4
On 04/30/2013 08:19 PM, 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
In this case the "system_uuid" field was a94b4335-6a14-8bc4-d6da-f7ea590b68-16 which passed the virUUIDParse() code because 32 hexadecimal digits were found and the extra hyphen in the last section was ignored.
Add checks to not only parse the read field, but then use virUUIDFormat() to validate that what gets formatted matches what was read - if not, then fail the edit.
I feel like we could do better. Either 1) such UUID is not valid (which I think it really isn't [1]) and we should fail when when parsing it or 2) it is valid, but qemu doesn't like it, so we should fixup the UUID before passing it to qemu (and maybe request proper UUID parsing from qemu guys). What do you think? Other that that, the patch look fine. Martin [1] http://www.ietf.org/rfc/rfc4122.txt
--- src/conf/domain_conf.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43273f8..c1fd99b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11577,17 +11577,32 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->sysinfo->system_uuid != NULL) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + /* Ensure that what we convert to a uuidbuf is converted back to + * the same string when formatted as a UUID. This field may be + * used by the underlying hypervisor driver instead of the domain + * uuid field and must be properly formatted. The virUUIDParse() + * is designed to "skip" extra "-"'s in the values and only + * validate that there are 32 hexadecimal digits. virUUIDFormat() + * returns uuidstr formatted properly. + */ + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0 || + memcmp(def->sysinfo->system_uuid, + virUUIDFormat(uuidbuf, uuidstr), + VIR_UUID_STRING_BUFLEN) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); + _("malformed <sysinfo> uuid element '%s' " + "found on '%s'"), + def->sysinfo->system_uuid, def->name); 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", + virReportError(VIR_ERR_INTERNAL_ERROR,
Pre-existing, but it looks like another abuse of internal error.
_("UUID mismatch between <uuid> and " - "<sysinfo>")); + "<sysinfo> 'uuid' <entry> on '%s'"), + def->name); goto error; } }
On 05/09/2013 06:27 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, 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
In this case the "system_uuid" field was a94b4335-6a14-8bc4-d6da-f7ea590b68-16 which passed the virUUIDParse() code because 32 hexadecimal digits were found and the extra hyphen in the last section was ignored.
Add checks to not only parse the read field, but then use virUUIDFormat() to validate that what gets formatted matches what was read - if not, then fail the edit.
I feel like we could do better. Either 1) such UUID is not valid (which I think it really isn't [1]) and we should fail when when parsing it or 2) it is valid, but qemu doesn't like it, so we should fixup the UUID before passing it to qemu (and maybe request proper UUID parsing from qemu guys).
What do you think?
That seemed to be a much larger fish to fry than I wanted to take on with this particular patch. That is by avoiding the dashes ("-") in virUUIDParse() already leads to this particular validation. The numbers are all correct, it's just additional dashes that were a problem. Changing that algorithm to be less forgiving led me down a path of wondering why that code is forgiving w/r/t "-" and " " and what could fall out from changing that? I suppose for this particular case it could have been possible to take the provided 'sysinfo' uuid field in qemuBuildSmbiosSystemStr() and make the virUUIDParse() and virUUIDFormat() calls prior to the virBufferAsprintf() which adds it to the command line. If that's desired, I can take that route.
Other that that, the patch look fine.
Martin
[1] http://www.ietf.org/rfc/rfc4122.txt
--- src/conf/domain_conf.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 43273f8..c1fd99b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11577,17 +11577,32 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; if (def->sysinfo->system_uuid != NULL) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + /* Ensure that what we convert to a uuidbuf is converted back to + * the same string when formatted as a UUID. This field may be + * used by the underlying hypervisor driver instead of the domain + * uuid field and must be properly formatted. The virUUIDParse() + * is designed to "skip" extra "-"'s in the values and only + * validate that there are 32 hexadecimal digits. virUUIDFormat() + * returns uuidstr formatted properly. + */ + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0 || + memcmp(def->sysinfo->system_uuid, + virUUIDFormat(uuidbuf, uuidstr), + VIR_UUID_STRING_BUFLEN) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); + _("malformed <sysinfo> uuid element '%s' " + "found on '%s'"), + def->sysinfo->system_uuid, def->name); 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", + virReportError(VIR_ERR_INTERNAL_ERROR,
Pre-existing, but it looks like another abuse of internal error.
To be consistent w/ the bios_date change, I changed from INTERNAL_ERROR to XML_DETAIL
_("UUID mismatch between <uuid> and " - "<sysinfo>")); + "<sysinfo> 'uuid' <entry> on '%s'"), + def->name); goto error; } }
I squashed the following in - I could send a v2 if that's desired... diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-format.xml b/tests/ new file mode 100644 index 0000000..ccbce54 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-format.xml @@ -0,0 +1,24 @@ +<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">362d1fc1-df7d-193e-5c18-49a71b-d1da66</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/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml b/tests/q new file mode 100644 index 0000000..e23c95e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml @@ -0,0 +1,24 @@ +<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 1286273..35427a1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -814,6 +814,9 @@ 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-format", 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);
On 05/09/2013 06:42 PM, John Ferlan wrote:
On 05/09/2013 06:27 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, 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
In this case the "system_uuid" field was a94b4335-6a14-8bc4-d6da-f7ea590b68-16 which passed the virUUIDParse() code because 32 hexadecimal digits were found and the extra hyphen in the last section was ignored.
Add checks to not only parse the read field, but then use virUUIDFormat() to validate that what gets formatted matches what was read - if not, then fail the edit.
I feel like we could do better. Either 1) such UUID is not valid (which I think it really isn't [1]) and we should fail when when parsing it or 2) it is valid, but qemu doesn't like it, so we should fixup the UUID before passing it to qemu (and maybe request proper UUID parsing from qemu guys).
What do you think?
That seemed to be a much larger fish to fry than I wanted to take on with this particular patch. That is by avoiding the dashes ("-") in virUUIDParse() already leads to this particular validation. The numbers are all correct, it's just additional dashes that were a problem. Changing that algorithm to be less forgiving led me down a path of wondering why that code is forgiving w/r/t "-" and " " and what could fall out from changing that?
I must admit I have no idea why we allow spaces and hyphens everywhere in the UUID string. Changing it to proper UUID parser is not just fixing this issue, but also removing more lines than adding, since the parser could get halved. That's why I was thinking more about taking that route. I'd love to hear some input from someone else in case this looks like it would break something. IMNSHO that's a bad usage unless we explicitly allowed spaces and hyphens somewhere in the docs.
I suppose for this particular case it could have been possible to take the provided 'sysinfo' uuid field in qemuBuildSmbiosSystemStr() and make the virUUIDParse() and virUUIDFormat() calls prior to the virBufferAsprintf() which adds it to the command line. If that's desired, I can take that route.
If I read the code correctly, we are properly saving the UUID in the definition in our format and using virUUIDFormat() properly to build the command line. Aren't we doing the same with system_uuid? If not, that could be helpful even for the future (actually until someone files a bug about dumpxml generating invalid UUIDs ;-) ) I can't help myself, but parsing, formatting and throwing away the result just to check the validity seems weird. [...]
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + virReportError(VIR_ERR_INTERNAL_ERROR,
Pre-existing, but it looks like another abuse of internal error.
To be consistent w/ the bios_date change, I changed from INTERNAL_ERROR to XML_DETAIL
Great, I know we can never eliminate even half of them, but we can definitely try :) Martin
On 05/10/2013 05:07 AM, Martin Kletzander wrote:
I must admit I have no idea why we allow spaces and hyphens everywhere in the UUID string. Changing it to proper UUID parser is not just fixing this issue, but also removing more lines than adding, since the parser could get halved. That's why I was thinking more about taking that route. I'd love to hear some input from someone else in case this looks like it would break something. IMNSHO that's a bad usage unless we explicitly allowed spaces and hyphens somewhere in the docs.
We're lax in what we parse and strict in what we output. We can't change the lax parser, because there really is code out there that doesn't format valid uuids itself and relies on libvirt to clean up the mess, but I don't think the current lax parser is all that bad (as long as we see exactly enough hex digits after skipping all dashes and spaces, we have something we can then turn into valid output).
If I read the code correctly, we are properly saving the UUID in the definition in our format and using virUUIDFormat() properly to build the command line. Aren't we doing the same with system_uuid? If not, that could be helpful even for the future (actually until someone files a bug about dumpxml generating invalid UUIDs ;-) )
I can't help myself, but parsing, formatting and throwing away the result just to check the validity seems weird.
If we parse and then format, we should use what we formatted from then on, instead of the original user's (potentially unusual) formatting. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/10/2013 10:57 AM, Eric Blake wrote:
On 05/10/2013 05:07 AM, Martin Kletzander wrote:
I must admit I have no idea why we allow spaces and hyphens everywhere in the UUID string. Changing it to proper UUID parser is not just fixing this issue, but also removing more lines than adding, since the parser could get halved. That's why I was thinking more about taking that route. I'd love to hear some input from someone else in case this looks like it would break something. IMNSHO that's a bad usage unless we explicitly allowed spaces and hyphens somewhere in the docs.
We're lax in what we parse and strict in what we output. We can't change the lax parser, because there really is code out there that doesn't format valid uuids itself and relies on libvirt to clean up the mess, but I don't think the current lax parser is all that bad (as long as we see exactly enough hex digits after skipping all dashes and spaces, we have something we can then turn into valid output).
If I read the code correctly, we are properly saving the UUID in the definition in our format and using virUUIDFormat() properly to build the command line. Aren't we doing the same with system_uuid? If not, that could be helpful even for the future (actually until someone files a bug about dumpxml generating invalid UUIDs ;-) )
I can't help myself, but parsing, formatting and throwing away the result just to check the validity seems weird.
If we parse and then format, we should use what we formatted from then on, instead of the original user's (potentially unusual) formatting.
It would seem that you are thus advocating we change what was read to be formatted properly and don't tell the user we did that, e.g.: if (def->sysinfo->system_uuid != NULL) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; + char uuidstr[VIR_UUID_STRING_BUFLEN]; if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("malformed uuid element")); + "%s", _("malformed <sysinfo> uuid element")); goto error; } if (uuid_generated) @@ -11593,6 +11594,45 @@ virDomainDefParseXML(xmlDocPtr xml, "<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 (memcmp(def->sysinfo->system_uuid, uuidstr, + VIR_UUID_STRING_BUFLEN) != 0) { + VIR_FREE(def->sysinfo->system_uuid); + if (VIR_STRDUP(def->sysinfo->system_uuid, uudstr) < 0) { + goto error; + } + } + } Would it be better to see a v2 with everything? Or push patches 1-3 and make a v2 of just this one? John
On 05/10/2013 06:52 PM, John Ferlan wrote:
On 05/10/2013 10:57 AM, Eric Blake wrote:
On 05/10/2013 05:07 AM, Martin Kletzander wrote:
I must admit I have no idea why we allow spaces and hyphens everywhere in the UUID string. Changing it to proper UUID parser is not just fixing this issue, but also removing more lines than adding, since the parser could get halved. That's why I was thinking more about taking that route. I'd love to hear some input from someone else in case this looks like it would break something. IMNSHO that's a bad usage unless we explicitly allowed spaces and hyphens somewhere in the docs.
We're lax in what we parse and strict in what we output. We can't change the lax parser, because there really is code out there that doesn't format valid uuids itself and relies on libvirt to clean up the mess, but I don't think the current lax parser is all that bad (as long as we see exactly enough hex digits after skipping all dashes and spaces, we have something we can then turn into valid output).
If I read the code correctly, we are properly saving the UUID in the definition in our format and using virUUIDFormat() properly to build the command line. Aren't we doing the same with system_uuid? If not, that could be helpful even for the future (actually until someone files a bug about dumpxml generating invalid UUIDs ;-) )
I can't help myself, but parsing, formatting and throwing away the result just to check the validity seems weird.
If we parse and then format, we should use what we formatted from then on, instead of the original user's (potentially unusual) formatting.
It would seem that you are thus advocating we change what was read to be formatted properly and don't tell the user we did that, e.g.:
With hearing from Eric about why we do that, I would do what you described, with one difference; you can get the uuid into temporary buffer and parse it into system_uuid. You don't have to check anything and just format the string when the system_uuid is used. That should be the same way as def->uuid is used and it seems like the cleanest one. [...]
Would it be better to see a v2 with everything? Or push patches 1-3 and make a v2 of just this one?
v2 of [3/4] and [4/4] would be great, thanks. Martin
On 05/10/2013 10:52 AM, John Ferlan wrote:
If we parse and then format, we should use what we formatted from then on, instead of the original user's (potentially unusual) formatting.
It would seem that you are thus advocating we change what was read to be formatted properly and don't tell the user we did that, e.g.:
<snip> Yes, I think you're on the right track.
Would it be better to see a v2 with everything? Or push patches 1-3 and make a v2 of just this one?
It's okay to push your ack'd patches, then post a v2 of what remains; and this time around, I was okay with your uuid patches, but had questions on the date parsing that might deserve a v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/30/2013 02:19 PM, John Ferlan wrote:
Not necessary for 1.0.5, but figured I'd put it out there now.
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).
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"
John Ferlan (4): docs: Fix syntax in sysinfo description docs: Update description of SMBIOS fields Validate the bios_date format for <sysinfo> Need better validation of <sysinfo> uuid
docs/formatdomain.html.in | 51 ++++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.c | 47 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 13 deletions(-)
ping? Tks, John
participants (3)
-
Eric Blake -
John Ferlan -
Martin Kletzander