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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list