1:19 p.m.
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
1:19 p.m.
New subject: [libvirt] [PATCH 1/4] docs: Fix syntax in sysinfo description
---
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
4:54 a.m.
New subject: [libvirt] [PATCH 1/4] docs: Fix syntax in sysinfo description
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
1:19 p.m.
New subject: [libvirt] [PATCH 2/4] docs: Update description of SMBIOS fields
---
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
4:54 a.m.
New subject: [libvirt] [PATCH 2/4] docs: Update description of SMBIOS fields
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
1:19 p.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
---
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
5:59 a.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
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 ;-)
6:43 a.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
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
8:58 a.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
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
11:11 a.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
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(+)
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"));
>>
>>> 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.
>>
...
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
11:30 a.m.
New subject: [libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>
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
1:19 p.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
5:27 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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;
}
}
11:42 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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);
6:07 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
9:57 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
11:52 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
8:26 a.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
3:01 p.m.
New subject: [libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid
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
5:52 a.m.
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
4210
days inactive
4223
days old
19 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Martin Kletzander