Re: mktime fix

[Jonathan Larmour <jifl at jifvik dot org>]

Gary Thomas wrote:
> The _simple_mktime() function could hang (*), given bad input data
> and no asserts.  This was found in the real world - a brand
> new I2C clock which had never been initialized passed in an
> incorrect month and the whole thing locked up :-(

Arguably if a wallclock driver knows that it's possible for the hardware to
express bogus dates, it should be sanitizing its output (for many
wallclocks, the restrictions on possible values is intrinsic of course, so
this isn't universally applicable). Hence asserts rather than runtime
check, and also hence why simple_mktime doesn't have an error return.

> --- io/wallclock/current/include/wallclock/wallclock.inl	27 Mar 2003 08:38:07 -0000	1.7
> +++ io/wallclock/current/include/wallclock/wallclock.inl	9 Sep 2008 11:30:19 -0000
> @@ -74,26 +74,26 @@ static time_t
>  _simple_mktime(cyg_uint32 year, cyg_uint32 mon,
>                 cyg_uint32 day, cyg_uint32 hour,
>                 cyg_uint32 min, cyg_uint32 sec)
>  {
>      time_t secs;
> -    cyg_uint32 y, m, days;
> +    cyg_int32 y, m, days;
>  
>      CYG_ASSERT(year <= 3124, "Year is unreasonably large");
>      CYG_ASSERT(mon <= 12, "Month is invalid");
>      CYG_ASSERT(day <= 31, "Day is invalid");
>      CYG_ASSERT(hour <= 23, "Hour is invalid");
>      CYG_ASSERT(min <= 59, "Minutes is invalid");
>      CYG_ASSERT(sec <= 61, "Seconds is invalid");
>  
>      // Number of days due to years
>      days = 0;
> -    for (y = 1970; y < year; y++)
> +    for (y = 1970; y < (cyg_int32)year; y++)
>          days += year_days(y);
>      // Due to months
> -    for (m = 0; m < mon-1; m++)
> +    for (m = 0; m < (cyg_int32)mon-1; m++)
>          days += days_per_month[is_leap(year)][m];
>      // Add days
>      days += day - 1;
>  
>      // Add hours, minutes, and seconds

If a device can return bogus values, then year==0 or mon==0 aren't the only
potential problems - for a device which would initially come up with a
random (rather than 0) value, it would be just as problematic as that case.
For example the above could easily walk off the end of the days_per_month
array.

I guess I mean the above change isn't a big deal, but doesn't really
address the main problem - wallclock drivers should validate data they read
if that's relevant for a particular device.

Jifl
-- 
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine