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

[Eric Blake]

On 05/13/2013 11:01 AM, John Ferlan wrote:
> Add incorrectly formatted bios_date validation test
> ---
>  src/conf/domain_conf.c                             | 24 ++++++++++++++++++++++
>  .../qemuxml2argvdata/qemuxml2argv-smbios-date.xml  | 23 +++++++++++++++++++++
>  tests/qemuxml2argvtest.c                           |  1 +
>  3 files changed, 48 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-smbios-date.xml
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 862b997..d352055 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8437,6 +8437,30 @@ virSysinfoParseXML(const xmlNodePtr node,
>          virXPathString("string(bios/entry[@name='version'])", ctxt);
>      def->bios_date =
>          virXPathString("string(bios/entry[@name='date'])", ctxt);
> +    if (def->bios_date != NULL) {
> +        char *ptr;
> +        struct tm tm;
> +        memset(&tm, 0, sizeof(tm));
> +
> +        /* Validate just the format of the date
> +         * Expect mm/dd/yyyy or mm/dd/yy,
> +         * where yy must be 00->99 and would be assumed to be 19xx
> +         * a yyyy date should be 1900 and beyond
> +         */
> +        if (virStrToLong_i(def->bios_date, &ptr, 10, &tm.tm_mon) < 0 ||
> +            *ptr != '/' ||
> +            virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 ||

Spaces around +

> +            *ptr != '/' ||
> +            virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||

and again

> +            *ptr != '\0' ||
> +            (tm.tm_mon < 0 || tm.tm_mon > 12) ||

According to 'man gmtime', tm_mon should be in the range 0-11 (you're
allowing an off-by-one on the high end).  Worse, January is 0, not 1; so
you need to take the number parsed by the user and subtract one before
passing it to struct tm.

> +            (tm.tm_mday < 0 || tm.tm_mday > 31) ||

tm_mday should be in the range 1-31 (you're allowing an off-by-one on
the low end)

> +            (tm.tm_year < 0 || (tm.tm_year >= 100 && tm.tm_year < 1900))) {

Oh, you aren't even using struct tm through a libc time-based function
such as gmtime.  In that case, my advice is to completely avoid 'struct
tm'.  It is such a non-intuitive struct (months start from 0, days from
1, years from 1900) that it should NOT be used for anything except the
standardized functions that adhere to those conventions while converting
between seconds since Epoch.  Just declare 'int mon, day, year;' and
parse into those variables, instead of trying to populate a struct tm
that you then throw away.

> +            virReportError(VIR_ERR_XML_DETAIL, "%s",
> +                           _("Invalid BIOS 'date' format"));
> +            goto error;
> +        }
> +    }

I like the approach of the patch, and especially that you added a test
case, but I think it's still worth a v3 that avoids 'struct tm' and any
chance of confusion it provides.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130513/166e430a/attachment-0001.sig>