Re: [PATCH] add tests for tzset(3)

[jdoubleu <hi@jdoubleu.de>]

Hi,

> I believe it is the ('>' != tzenv[n]) check that fails, because the
> maximum parsed string limit of 10 has been reached and n=10.

Okay, so the newlib implementation actually fails, when the name is too 
long.

The POSIX standard leaves it up to the implementation how to handle 
names[1]:
> The interpretation of these fields is unspecified if either field is less than three bytes (except for the case when dst is missing), more than {TZNAME_MAX} bytes, or if they contain characters other than those specified.

I'm not sure where to go with this. In both cases, the current 
implementation needs a fix. Looping in Brian Inglis, the writer of the 
current implementation and Jeff Johnston, a maintainer.

As I see it, there are two solutions:
1. Treat long names as an error case. The failed condition[2] needs to 
apply UTC as current time, like Dimitar Dimitrov previously suggested. 
The test needs to be changed to check that it actually fails.
2. Mimic glibc's behavior and ignore the remaining characters from the 
name. In that case, the condition[2] and following code needs to be 
updated as well.

I am happy to provide a patch, when we agreed on one solution.

> I assume you no longer need assembly output from compiler?

No, thanks. We already found the issue. It would only be helpful, if 
different configurations generate different code (through macros, 
optimization, etc.).

[1]: 
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
[2]: 
https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/time/tzset_r.c;h=9cb30b188f989f65ec9eb6417f5d74020f8c72e9;hb=HEAD#l57

Cheers
---
🙎🏻‍♂️ jdoubleu
On 4/11/2022 7:27 PM, Dimitar Dimitrov wrote:
> On Mon, Apr 11, 2022 at 01:17:16PM +0200, jdoubleu wrote:
>> Hi,
>>
>> looks like I'm running the testsuite against glibc and not newlib (for
>> target x86_64-pc-linux-gnu). I'm not even sure whether there's a backend for
>> linux.
>>
>> I'm currently trying to run only the tzset code against the test vectors
>> (like Brian Inglis did[1]).
>>
>> At least it show, that the newlib implementation differs from glibc. Maybe
>> the test case is flawed and it should indeed fail.
>>
>>> 6:20:12 is the timezone string of the previous test case, whose tzset
>>> call was successful. Looking at the current code, this is expected
>>> behaviour.
>>
>> Okay. Looks like the condition[2] fails. The question is, which part of it
>> does?
> 
> I believe it is the ('>' != tzenv[n]) check that fails, because the
> maximum parsed string limit of 10 has been reached and n=10.
> 
>>
>> I've appended a patch, which prints all variables when the condition fails.
>> Could you please apply the patch and then recompile and re-run the tests
>> again?
> 
> Here is the result for arm-none-eabi:
> parsing name: tzenv="+0123456789ABCDEF>3:33:33", res=1, n=10, tzenv[n] = 9
> Assertion failed! Expected 1647906533 to equal 1647916532. winter time, timezone = "<+0123456789ABCDEF>3:33:33"
> 
> I assume you no longer need assembly output from compiler?
>>
>> I've previously noticed something with the sscanf format[3].
>>
>>> Perhaps TZ should be reset to UTC before the bail out?
>>
>> I don't think the implementation should fall back to UTC whenever parsing
>> failed. It apparently doesn't in glibc. I'm not sure if the behavior is
>> specified somewhere.
> 
> The tzset manual page says that UTC is used if TZ cannot be parsed:
> https://man7.org/linux/man-pages/man3/tzset.3.html
> 
>>
>> Maybe resetting it before each test case is a good idea, though. That makes
>> it clearer, why the test case failed.
>>
>>> With that chunk removed, as shown below:
>>>      {"3:33:33",               IN_SECONDS(3, 33, 33),   NO_TIME},                 // truncates the name (17 + 1)
>>> I still get:
>>>    Assertion failed! Expected 1647906533 to equal 1647916532. winter time, timezone = "3:33:33"
>>
>> My bad; "3:33:33" isn't a valid timezone string. It has to be prefixed by a
>> name e.g. "MESZ" or "<+00>", as you tried. That explains why it is also
>> failing.
>>
>>
>> Thanks for your effort so far!
>>
>>
>> [1]: https://sourceware.org/pipermail/newlib/2022/019529.html
>> [2]: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/time/tzset_r.c;h=9cb30b188f989f65ec9eb6417f5d74020f8c72e9;hb=HEAD#l57
>> [3]: https://sourceware.org/pipermail/newlib/2022/019535.html
>>
>>
>> Cheers
>> ---
>> 🙎🏻‍♂️ jdoubleu
> 
>>  From 8c698dc63f765d4a5b3a49a25850c1738279d68d Mon Sep 17 00:00:00 2001
>> From: jdoubleu <hi@jdoubleu.de>
>> Date: Mon, 11 Apr 2022 13:10:38 +0200
>> Subject: [PATCH] debug print condition values of tz string name parsing
>>
>> ---
>>   newlib/libc/time/tzset_r.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/newlib/libc/time/tzset_r.c b/newlib/libc/time/tzset_r.c
>> index 9cb30b188..e20a32a62 100644
>> --- a/newlib/libc/time/tzset_r.c
>> +++ b/newlib/libc/time/tzset_r.c
>> @@ -54,9 +54,11 @@ _tzset_unlocked_r (struct _reent *reent_ptr)
>>         ++tzenv;
>>   
>>         /* quit if no items, too few or too many chars, or no close quote '>' */
>> -      if (sscanf (tzenv, "%10[-+0-9A-Za-z]%n", __tzname_std, &n) <= 0
>> -		|| n < TZNAME_MIN || TZNAME_MAX < n || '>' != tzenv[n])
>> +      int res = sscanf (tzenv, "%10[-+0-9A-Za-z]%n", __tzname_std, &n);
>> +      if (res <= 0 || n < TZNAME_MIN || TZNAME_MAX < n || '>' != tzenv[n]) {
>> +        printf("parsing name: tzenv=\"%s\", res=%d, n=%d, tzenv[n] = %c\n", tzenv, res, n, tzenv[n]);
>>           return;
>> +      }
>>   
>>         ++tzenv;	/* bump for close quote '>' */
>>       }
>> -- 
>> 2.35.1
>>
>