Re: [ros-dev] [ros-diffs] [gschneider] 42402: asctime/ctime: Check for too low input time, fixes one msvcrt time winetest

[Alexander Potashev]

Hi,
First of all, are you sure that this code is mature enough to care
about minor details? I would say, "@implemented" has been added by
mistake.

About this commit: I tried to call asctime from glibc-2.8 on Linux,
but tm_year=9 works fine (resulting in 1909). I know, it is not
msvcrt. But I don't see any good reason to not allow years before
1970. Furthermore, I'm sure, this function was once introduced to just
transform a date to human-readable format, and it shouldn't care about
the date. Btw, MSDN says nothing

Another tricky question is: How is the UNIX epoch connected with
Reactos (or Windows)?

About 'asctime': it might be holy, but it's "holey". It doesn't even
check the month and the day of week to fit the ranges 0..11 and 0..6
correspondingly.

So, please, fix the security problems first, and then revert this commit ;)

2009/8/4  [e-mail]:
> Author: gschneider
> Date: Wed Aug  5 04:06:25 2009
> New Revision: 42402
>
> URL: http://svn.reactos.org/svn/reactos?rev=42402&view=rev
> Log:
> asctime/ctime: Check for too low input time, fixes one msvcrt time winetest
>
> Modified:
>    trunk/reactos/lib/sdk/crt/time/ctime.c
>
> Modified: trunk/reactos/lib/sdk/crt/time/ctime.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/time/ctime.c?re...
> ==============================================================================
> --- trunk/reactos/lib/sdk/crt/time/ctime.c [iso-8859-1] (original)
> +++ trunk/reactos/lib/sdk/crt/time/ctime.c [iso-8859-1] Wed Aug  5 04:06:25 2009
> @@ -1200,14 +1200,23 @@
>     "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
>   };
>   static char result[26];
> (void) sprintf(result, "%.3s %.3s%3d %02d:%02d:%02d %d\n",
> wday_name[timeptr->tm_wday],
> mon_name[timeptr->tm_mon],
> timeptr->tm_mday, timeptr->tm_hour,
> timeptr->tm_min, timeptr->tm_sec,
> TM_YEAR_BASE + timeptr->tm_year);
> return result;
> char* res = result;
> /* Check for invalid input time */
> if (timeptr->tm_year <= 69)
> {
> res = NULL;
> }
> else
> {
> sprintf(res, "%.3s %.3s%3d %02d:%02d:%02d %d\n",
> wday_name[timeptr->tm_wday],
> mon_name[timeptr->tm_mon],
> timeptr->tm_mday, timeptr->tm_hour,
> timeptr->tm_min, timeptr->tm_sec,
> TM_YEAR_BASE + timeptr->tm_year);
> }
> return res;
>
> }
>
> /*

[Alexander Potashev]

I didn't really want to send this e-mail right now, but my eeepc has
strong opinions. ;)

2009/8/4 Alexander Potashev [e-mail]:
> Hi,
> First of all, are you sure that this code is mature enough to care
> about minor details? I would say, "@implemented" has been added by
> mistake.
>
> About this commit: I tried to call asctime from glibc-2.8 on Linux,
> but tm_year=9 works fine (resulting in 1909). I know, it is not
> msvcrt. But I don't see any good reason to not allow years before
> 1970. Furthermore, I'm sure, this function was once introduced to just
> transform a date to human-readable format, and it shouldn't care about
> the date. Btw, MSDN says nothing
>
> Another tricky question is: How is the UNIX epoch connected with
> Reactos (or Windows)?
>
> About 'asctime': it might be holy, but it's "holey". It doesn't even
> check the month and the day of week to fit the ranges 0..11 and 0..6
> correspondingly.
>
> So, please, fix the security problems first, and then revert this commit ;)
>
> 2009/8/4  [e-mail]:
> > Author: gschneider
> > Date: Wed Aug  5 04:06:25 2009
> > New Revision: 42402
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=42402&view=rev
> > Log:
> > asctime/ctime: Check for too low input time, fixes one msvcrt time winetest
> >
> > Modified:
> >    trunk/reactos/lib/sdk/crt/time/ctime.c
> >
> > Modified: trunk/reactos/lib/sdk/crt/time/ctime.c
> > URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/sdk/crt/time/ctime.c?re...
> > ==============================================================================
> > --- trunk/reactos/lib/sdk/crt/time/ctime.c [iso-8859-1] (original)
> > +++ trunk/reactos/lib/sdk/crt/time/ctime.c [iso-8859-1] Wed Aug  5 04:06:25 2009
> > @@ -1200,14 +1200,23 @@
> >     "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"
> >   };
> >   static char result[26];
> > (void) sprintf(result, "%.3s %.3s%3d %02d:%02d:%02d %d\n",
> > wday_name[timeptr->tm_wday],
> > mon_name[timeptr->tm_mon],
> > timeptr->tm_mday, timeptr->tm_hour,
> > timeptr->tm_min, timeptr->tm_sec,
> > TM_YEAR_BASE + timeptr->tm_year);
> > return result;
> > char* res = result;
> > /* Check for invalid input time */
> > if (timeptr->tm_year <= 69)
> > {
> > res = NULL;
> > }
> > else
> > {
> > sprintf(res, "%.3s %.3s%3d %02d:%02d:%02d %d\n",
> > wday_name[timeptr->tm_wday],
> > mon_name[timeptr->tm_mon],
> > timeptr->tm_mday, timeptr->tm_hour,
> > timeptr->tm_min, timeptr->tm_sec,
> > TM_YEAR_BASE + timeptr->tm_year);
> > }
> > return res;
> >
> > }
> >
> > /*

[Steven Edwards]

On Tue, Aug 4, 2009 at 10:52 PM, Alexander [e-mail] wrote:
> First of all, are you sure that this code is mature enough to care
> about minor details? I would say, "@implemented" has been added by
> mistake.

The whole @implemented/@unimplemented thing is a mistake. It's really
a worthless measure to go by. I mean lets say function foo takes 20
args but only 10 are useful most of the time and then 8 out of those
are implemented, what do you say? @sorta-buggy-implemented...

I think if we are going to have something like this then we need
autogenerated implementation details on top of the normal API
documentation that we have. My thought is to have something based on
the conformance tests results.

/*
 * ShellExecuteEx
 *
 * Foo Blah Stuff and Things Docu Blah Blah
 *
 * PARAMS
 *    Foo  gives stuff
 *
 * Returns
 *     Bar returns stuff
 *
 * Implementation Status
 *
 *  %72 implemented
 *  %10 implementation Fuzz (or failure or something, can't think of
the best term)
 *  %18 TODO
 */

Some one want to write a bot that does this during release time? I'd
love to be able to hack on it but like all my ideas, I lack the free
time.
-- 
Steven Edwards

"There is one thing stronger than all the armies in the world, and
that is an idea whose time has come." - Victor Hugo

[Alexander Potashev]

Nevermind. I just wanted to say that this function is very broken and
it's a bit strange that you have made another, arguable, change.

So, which test your commit fixes and why is that test correct?

2009/8/5 Steven Edwards [e-mail]:
> On Tue, Aug 4, 2009 at 10:52 PM, Alexander [e-mail] wrote:
> > First of all, are you sure that this code is mature enough to care
> > about minor details? I would say, "@implemented" has been added by
> > mistake.
>
> The whole @implemented/@unimplemented thing is a mistake. It's really
> a worthless measure to go by. I mean lets say function foo takes 20
> args but only 10 are useful most of the time and then 8 out of those
> are implemented, what do you say? @sorta-buggy-implemented...
>
> I think if we are going to have something like this then we need
> autogenerated implementation details on top of the normal API
> documentation that we have. My thought is to have something based on
> the conformance tests results.
>
> /*
>  * ShellExecuteEx
>  *
>  * Foo Blah Stuff and Things Docu Blah Blah
>  *
>  * PARAMS
>  *    Foo  gives stuff
>  *
>  * Returns
>  *     Bar returns stuff
>  *
>  * Implementation Status
>  *
>  *  %72 implemented
>  *  %10 implementation Fuzz (or failure or something, can't think of
> the best term)
>  *  %18 TODO
>  */
>
> Some one want to write a bot that does this during release time? I'd
> love to be able to hack on it but like all my ideas, I lack the free
> time.
>
> --
> Steven Edwards
>
> "There is one thing stronger than all the armies in the world, and
> that is an idea whose time has come." - Victor Hugo
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Alexander Potashev]

2009/8/5 Alexander Potashev [e-mail]:
> Nevermind. I just wanted to say that this function is very broken and
> it's a bit strange that you have made another, arguable, change.

I meant gschneider
> So, which test your commit fixes and why is that test correct?

And here too...
> 2009/8/5 Steven Edwards [e-mail]:
> > On Tue, Aug 4, 2009 at 10:52 PM, Alexander [e-mail] wrote:
> > > First of all, are you sure that this code is mature enough to care
> > > about minor details? I would say, "@implemented" has been added by
> > > mistake.
> >
> > The whole @implemented/@unimplemented thing is a mistake. It's really
> > a worthless measure to go by. I mean lets say function foo takes 20
> > args but only 10 are useful most of the time and then 8 out of those
> > are implemented, what do you say? @sorta-buggy-implemented...
> >
> > I think if we are going to have something like this then we need
> > autogenerated implementation details on top of the normal API
> > documentation that we have. My thought is to have something based on
> > the conformance tests results.
> >
> > /*
> >  * ShellExecuteEx
> >  *
> >  * Foo Blah Stuff and Things Docu Blah Blah
> >  *
> >  * PARAMS
> >  *    Foo  gives stuff
> >  *
> >  * Returns
> >  *     Bar returns stuff
> >  *
> >  * Implementation Status
> >  *
> >  *  %72 implemented
> >  *  %10 implementation Fuzz (or failure or something, can't think of
> > the best term)
> >  *  %18 TODO
> >  */
> >
> > Some one want to write a bot that does this during release time? I'd
> > love to be able to hack on it but like all my ideas, I lack the free
> > time.
> >
> > --
> > Steven Edwards
> >
> > "There is one thing stronger than all the armies in the world, and
> > that is an idea whose time has come." - Victor Hugo
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev