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

[John Ferlan]

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