[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/13/2013 03:51 PM, Eric Blake wrote:
> 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.
> 

Oh right - at one time I was doing the gmtime or strptime call to attempt
to validate the data; however, I found it was less than perfect allowing
things like 2/31/2000 to be accepted.

In any case, in lieu of a v3, here's a diff:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fbe4d9a..0516413 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8442,23 +8442,22 @@ virSysinfoParseXML(const xmlNodePtr node,
         virXPathString("string(bios/entry[ name='date'])", ctxt);
     if (def->bios_date != NULL) {
         char *ptr;
-        struct tm tm;
-        memset(&tm, 0, sizeof(tm));
+        int month, day, year;
 
         /* 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 ||
+        if (virStrToLong_i(def->bios_date, &ptr, 10, &month) < 0 ||
             *ptr != '/' ||
-            virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_mday) < 0 ||
+            virStrToLong_i(ptr + 1, &ptr, 10, &day) < 0 ||
             *ptr != '/' ||
-            virStrToLong_i(ptr+1, &ptr, 10, &tm.tm_year) < 0 ||
+            virStrToLong_i(ptr + 1, &ptr, 10, &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))) {
+            (month < 1 || month > 12) ||
+            (day < 1 || day > 31) ||
+            (year < 0 || (year >= 100 && year < 1900))) {
             virReportError(VIR_ERR_XML_DETAIL, "%s",
                            _("Invalid BIOS 'date' format"));
             goto error;


or more visually appealing

    if (def->bios_date != NULL) {
        char *ptr;
        int month, day, year;

        /* 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, &month) < 0 ||
            *ptr != '/' ||
            virStrToLong_i(ptr + 1, &ptr, 10, &day) < 0 ||
            *ptr != '/' ||
            virStrToLong_i(ptr + 1, &ptr, 10, &year) < 0 ||
            *ptr != '\0' ||
            (month < 1 || month > 12) ||
            (day < 1 || day > 31) ||
            (year < 0 || (year >= 100 && year < 1900))) {
            virReportError(VIR_ERR_XML_DETAIL, "%s",
                           _("Invalid BIOS 'date' format"));
            goto error;
        }
    }


John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]