[libvirt] [PATCH 3/4] Validate the bios_date format for <sysinfo>

[John Ferlan]

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
> 
>