tst-getdate: Improve testcase flexibility and add test.

The getdate testcases all expect successful results.  Add support for
negative testcases and testcases where a full date and time are not
supplied by skipping the tm checks in the test.  Add a testcase that
would catch a use-after-free that was recently found.
---
 time/tst-getdate.c | 56 ++++++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

Hi Joe,

I have some comments -- but overall, this seems to improve coverage
and flexibility. Martin Coufal was interested in adding more getdate
tests and this change should make it easy to add some more odd cases.

> The getdate testcases all expect successful results.  Add support for
> negative testcases and testcases where a full date and time are not
> supplied by skipping the tm checks in the test.  Add a testcase that
> would catch a use-after-free that was recently found.

OK.

> diff --git a/time/tst-getdate.c b/time/tst-getdate.c
> index 4c9ed28d58..0036d313d7 100644
> --- a/time/tst-getdate.c
> +++ b/time/tst-getdate.c
> @@ -32,34 +32,40 @@ static const struct
>    const char *tz;
>    struct tm tm;
>    bool time64;
> +  int err_val;
> +  bool check_tm;

OK.
err_val has the expected error value for each test case. This should
be 0 for all existing tests.
check_tm says if the test case should include checking values for tm.
This is because time-only inputs will return the current date, which
changes from run to run. This should be true for all existing tests.

>  } tests [] =
>  {
>    {"21:01:10 1999-1-31", "Universal", {10, 1, 21, 31, 0, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},
>    {"21:01:10    1999-1-31", "Universal", {10, 1, 21, 31, 0, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},
>    {"   21:01:10 1999-1-31", "Universal", {10, 1, 21, 31, 0, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},
>    {"21:01:10 1999-1-31   ", "Universal", {10, 1, 21, 31, 0, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},
>    {"    21:01:10 1999-1-31   ", "Universal", {10, 1, 21, 31, 0, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},
>    {"21:01:10 1999-2-28", "Universal", {10, 1, 21, 28, 1, 99, 0, 0, 0},
> -   false },
> +   false , 0, true},

OK. Existing tests all get err_val = 0 and check_tm = true.

>    {"16:30:46 2000-2-29", "Universal", {46, 30,16, 29, 1, 100, 0, 0, 0},
> -   false },
> -  {"01-08-2000 05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> -   false },
> +   false , 0, true},
> +  {"01-08-2000  05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> +   false , 0, true},

First test in this block is an old test. It gets a 0/true like others. OK.

Second test in this block looks like it got some new whitespace. Was
it intentional? I guess it's still OK because it touches upon
whitespace handling.

> +  {"01-08-2000 a 05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> +   false , 7, false},
> +  {"       12          AM     ", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> +   false , 0, false},

OK. Two new tests here:

1. has a spurious "a" in it and therefore should fail. Reason for
failure is that it doesn't match any of the date masks supplied.
2. has quite a bit of whitespace and is a time-only input. It should
succeed (err_val = 0), but we don't check the tm (check_tm = false)
because the date will be different at each execution.

>
>    /* 64 bit time_t tests.  */
>    {"21:01:10 2038-1-31", "Universal", {10, 1, 21, 31, 0, 138, 0, 0, 0},
> -   true },
> +   true , 0, true},
>    {"22:01:10 2048-5-20", "Universal", {10, 1, 22, 20, 4, 148, 0, 0, 0},
> -   true },
> +   true , 0, true},
>    {"01-08-2038 05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 138, 0, 0, 0},
> -   true },
> +   true , 0, true},
>    {"20-03-2050 21:30:08", "Europe/Berlin", {8, 30, 21, 20, 2, 150, 0, 0, 0},
> -   true }
> +   true , 0, true}
>  };
>
>  static const char *

OK. All existing 64 bit time_t tests get 0/true in line with other
existing tests.

> @@ -93,7 +99,8 @@ report_date_error (void)
>  static char *datemsk;
>  static const char datemskstr[] =
>    "%H:%M:%S %F\n"
> -  "%d-%m-%Y %T\n";
> +  "%d-%m-%Y %T\n"
> +  "%I %p\n";
>

OK. New date mask added for the time-only test.

>  static void
>  do_prepare (int argc, char **argv)
> @@ -115,13 +122,23 @@ do_test (void)
>        setenv ("TZ", tests[i].tz, 1);
>
>        tm = getdate (tests[i].str);
> -      TEST_COMPARE (getdate_err, 0);
> -      if (getdate_err != 0)
> +
> +      /* Only check getdate_err when tm is NULL as getdate doesn't set
> +         getdate_err on success. */
> +      if (tm == NULL)
> +        TEST_COMPARE (getdate_err, tests[i].err_val);

OK. Just confirmed from the man-page:

"When successful, getdate() returns a pointer to a struct tm.
Otherwise, it returns NULL and sets the global variable getdate_err to
one of the error numbers shown below."

i.e. getdate_err isn't guaranteed to be reset to 0 upon success.

> +      if (tests[i].err_val != 0)  /* Expected failure */
> +       {
> +         TEST_COMPARE_BLOB (tm, 0, NULL, 0);
> +         continue;
> +       }

I think TEST_COMPARE_BLOB (x, 0, y, 0) is a nop.

> +
> +      if (tm == NULL && getdate_err != tests[i].err_val)
>         {
>           support_record_failure ();
>           printf ("%s\n", report_date_error ());
>         }

Maybe this should go in along with the above check where we already do
TEST_COMPARE (getdate_err, tests[i].err_val)?

> -      else
> +      else if (tests[i].check_tm)
>         {
>           TEST_COMPARE (tests[i].tm.tm_mon, tm->tm_mon);
>           TEST_COMPARE (tests[i].tm.tm_year, tm->tm_year);

OK. Now we conditionally check.

> @@ -132,8 +149,9 @@ do_test (void)
>         }
>
>        struct tm tms;
> -      TEST_COMPARE (getdate_r (tests[i].str, &tms), 0);
> -      if (getdate_err == 0)
> +      int retval = getdate_r (tests[i].str, &tms);
> +      TEST_COMPARE (retval, tests[i].err_val);

OK. Now we handle expected failures here.

> +      if (retval == tests[i].err_val && tests[i].check_tm)
>         {
>           TEST_COMPARE (tests[i].tm.tm_mon, tms.tm_mon);
>           TEST_COMPARE (tests[i].tm.tm_year, tms.tm_year);

OK. We conditionally check.

Thanks!
Arjun

On Fri, Jun 09, 2023 at 07:20:38PM +0200, Arjun Shankar wrote:
> Hi Joe,
> 
> I have some comments -- but overall, this seems to improve coverage
> and flexibility. Martin Coufal was interested in adding more getdate
> tests and this change should make it easy to add some more odd cases.

Hi Arjun,

Thanks for the review.  I'll fix up the patch based on your comments and
post a v2 soon.

...

> 
> >    {"16:30:46 2000-2-29", "Universal", {46, 30,16, 29, 1, 100, 0, 0, 0},
> > -   false },
> > -  {"01-08-2000 05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> > -   false },
> > +   false , 0, true},
> > +  {"01-08-2000  05:06:07", "Europe/Berlin", {7, 6, 5, 1, 7, 100, 0, 0, 0},
> > +   false , 0, true},
> 
> First test in this block is an old test. It gets a 0/true like others. OK.
> 
> Second test in this block looks like it got some new whitespace. Was
> it intentional? I guess it's still OK because it touches upon
> whitespace handling.

That should have been a new test with more whitespace.  Fixed in v2.

...
> 
> > +      if (tests[i].err_val != 0)  /* Expected failure */
> > +       {
> > +         TEST_COMPARE_BLOB (tm, 0, NULL, 0);
> > +         continue;
> > +       }
> 
> I think TEST_COMPARE_BLOB (x, 0, y, 0) is a nop.

Fixed in v2.

> 
> > +
> > +      if (tm == NULL && getdate_err != tests[i].err_val)
> >         {
> >           support_record_failure ();
> >           printf ("%s\n", report_date_error ());
> >         }
> 
> Maybe this should go in along with the above check where we already do
> TEST_COMPARE (getdate_err, tests[i].err_val)?

Moved in v2.

Thanks,
Joe