bugGNUstep - Bugs: bug #60952, NSTimeZone fail to deal with...

Group
 
 

bug #60952: NSTimeZone fail to deal with tzfile v2+

Submitter:  None
Submitted:  Wed 21 Jul 2021 03:43:09 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 30 Aug 2021 02:25:14 PM UTC, comment #28: 

Changes merged into trunk.

Richard Frith-Macdonald <CaS>
Group Member
Fri 27 Aug 2021 08:30:12 AM UTC, comment #27: 

I've been away for a week (while other developers have been looking at the code and making suggestions to improve it).  I expect we can merge it soon.

Richard Frith-Macdonald <CaS>
Group Member
Thu 26 Aug 2021 12:13:15 PM UTC, comment #26: 

Hello

No news for two weeks. Is there anything missing?

Emmanuel Dreyfus <manubsd>
Mon 16 Aug 2021 02:02:10 PM UTC, comment #25: 

You should remove that two now unused files:
Tests/base/NSTimeZone/Paris.tzdbv1
Tests/base/NSTimeZone/Paris.tzdbv2

We use ParisV1.tzdb  and ParisV2.tzdb in the tests.

Emmanuel Dreyfus <manubsd>
Mon 16 Aug 2021 01:09:57 PM UTC, comment #24: 

comment #23:

> Please give it another try on any platforms you have available.


It seems fine.

Emmanuel Dreyfus <manubsd>
Mon 16 Aug 2021 11:05:38 AM UTC, comment #23: 

Thanks ... I made that modification to have 2000 slots available, and updated the tests.  I also made a lot of simplifications to tzdb.h (removing unnecessary defines and includes etc) and made various portability changes (to make the code run on ms-windows).
Please give it another try on any platforms you have available.

Richard Frith-Macdonald <CaS>
Group Member
Sat 14 Aug 2021 01:36:39 AM UTC, comment #22: 

comment #21:

> With the correct file, there are two failed tests with latest code


The two tests pass with this change. tzdb.h is the only place where TZ_MAX_TIMES is used, hence it seems safe.

diff --git a/Source/nstzfile.h b/Source/nstzfile.h
index 265974110..72194a0d5 100644
--- a/Source/nstzfile.h
+++ b/Source/nstzfile.h
@@ -81,8 +81,9 @@ struct tzhead {
 ** year's worth of solar time (corrected daily to the nearest second) or
 ** 138 years of Pacific Presidential Election time
 ** (where there are three time zone transitions every fourth year).
+** XXX it needs to be at least 2000 to cope with TZDB v2+
 */
-#define TZ_MAX_TIMES   370
+#define TZ_MAX_TIMES   2000
 #endif /* !defined TZ_MAX_TIMES */
 
 #ifndef TZ_MAX_TYPES

Emmanuel Dreyfus <manubsd>
Thu 12 Aug 2021 01:23:01 AM UTC, comment #21: 

Hold on, I used a wrong file for v2.

With the correct file, there are two failed tests with latest code, for the two unexpected results with latest code:

post-1996 standard time offset = 7200 (3600 expected)
post-2038 standard time offset = 7200 (3600 expected)

Updated files attached in localtime2.tgz


(file #51766)

Emmanuel Dreyfus <manubsd>
Thu 12 Aug 2021 01:01:05 AM UTC, comment #20: 

I modified the test to check behavior for 4 files
- A real v1 file obtained from 13 years old NetBSD 4.0
- A real v2 file obtained from recent NetBSD 9.0
- Your buggy v1 file without magic string
- My buggy v2 file without v2 header

The modified localtime.m and the zone files are attached in localtime.tgz

For all files, I test standard and DST before and after 1996. For v2 files I also test beyond 2038.

When linking the test with latest libgnustep-base that contains the fixes, all tests pass, even with the buggy files.

When linking the test with libgnustep-base.so.1.27.0 that does not contains our fixes, the following tests are failed (I only copy the failed tests for the sake of clarity):

2021-08-12 02:53:56.687 localtime[3228:3091415124] Unable to obtain time zone `./ParisV1-noMagic.tzdb'... <NSException: 0xb82d15e4> NAME:GSTimeZoneFileException REASON:TZ_MAGIC is incorrect INFO:(null)
Failed test:     localtime.m:20 ... +timeZoneWithName data works
Failed test:     localtime.m:26 ... pre-1996 standard time offset vs UTC found for user-supplied buggy Europe/Paris TZDB v1 without magic
Failed test:     localtime.m:31 ... pre-1996 DST time offset vs UTC found for user-supplied buggy Europe/Paris TZDB v1 without magic
Failed test:     localtime.m:37 ... post-1996 standard time offset vs UTC found for user-supplied buggy Europe/Paris TZDB v1 without magic
Failed test:     localtime.m:42 ... post-1996 DST time offset vs UTC found for user-supplied buggy Europe/Paris TZDB v1 without magic
Failed test:     localtime.m:49 ... post-2038 DST time offset vs UTC found for user-supplied buggy Europe/Paris TZDB v2 without v2 header

In summary, the new code fixes post-2038 v2 as we expected, and it also copes with v1 without magic that caused exception in previous code.

(file #51765)

Emmanuel Dreyfus <manubsd>
Wed 11 Aug 2021 06:50:02 PM UTC, comment #19: 


> I noticed that Tests/base/NSTimeZone/Paris.tzdbv1 was not a
> type1 time zone file, so I replaced the test files with an
> ancient file I found


I picked the file from a older CentOS system, and I did not notice it was not RFC 8536 compliant v1 file. But despite the TZif2 magic, it is not RFC 8536 compliant v2 file either, since it misses the second header.

Even more surprising, your file is not RFC 8536 compliant v1, because it lacks the TZif magic.

It seems there are many odd files formats lying around. We should probably add tests for more sample files.

Emmanuel Dreyfus <manubsd>
Wed 11 Aug 2021 08:03:10 AM UTC, comment #18: 

I restructured the code changes, to retain the existing behavior for absolute time zones, and to fix some memory issues and remove unused code etc while getting the code to work on my primary dev system (CentOS).

I noticed that Tests/base/NSTimeZone/Paris.tzdbv1 was not a type1 time zone file, so I replaced the test files with an ancient file I found (the oldest one for Paris in the GNUstep repository) and the current system time zone file for Paris from CentOS.  Maybe these are not the best files to use for testing though?

Looking at the code for handling the TZ style string in tzdb.h, I'm not sure it actually works properly.

Please could you take a look and let me know what you think.  The changes are all in a new branch at https://github.com/gnustep/libs-base/tree/nstimezone

Richard Frith-Macdonald <CaS>
Group Member
Fri 06 Aug 2021 01:45:04 PM UTC, comment #17: 

I pushed the following changes:
- added (hopefully all) missing macros for Linux
- fixed loading zone from user-supplied NSData
- added tests for system TZDB, user-supplied v1 and v2.

I hope you now can take it over.

Emmanuel Dreyfus <manubsd>
Fri 06 Aug 2021 10:18:32 AM UTC, comment #16: 

I was looking at building (on GNU/Linux), but the code currently doesn't compile due to the use of a few undeclared macros in tzdb.c

Richard Frith-Macdonald <CaS>
Group Member
Fri 06 Aug 2021 08:48:01 AM UTC, comment #15: 

Hold on. I have written tests and there is something broken for loading from NSData.

Anonymous
Thu 05 Aug 2021 03:16:04 PM UTC, comment #14: 

comment #13:

> Certainly it looks to be in a state where I could take it and clean it up and incorporate it into the source code repository if you like, or you could carry on cleaning it up if you prefer.


You will certainly beat me at it, and I will appreciate if you could take it over.

I pushed two more commits
1) split tzloadbody so that we can use it when loading with  - (id) initWithName: (NSString*)name data: (NSData*)data. This removes the older parser and make sure we get consistent output

2) Move public domain code in tzdb.c as you requested.

And there are still 4 methods that do not initialise struct state. I am not sure of what happen if you call secondsFromGMTForDate for a zone created that way.

- (id) initWithOffset: (NSInteger)anOffset name: (NSString*)aName;
- (id) initWithTimeZone: (GSAbsTimeZone*)aZone;
- (id) initWithCoder: (NSCoder*)aDecoder
- (id) initWithTimeZone: (NSTimeZone*)aZone
             withAbbrev: (NSString*)anAbbrev
             withOffset: (NSInteger)anOffset
                withDST: (BOOL)isDST


Emmanuel Dreyfus <manubsd>
Thu 05 Aug 2021 02:40:54 PM UTC, comment #13: 

Thanks for the new changes.  For clarity, I would prefer all the public domain code to be in one file:  nothing other than NSTimeZone.m will include tzdb_private.h so it would be find to put the public domain implementation there after the headers.

Then NSTimeZone.m would use the public domain code, but not actually contain any of it.

The actual changes look reasonable, though as you point out the memory management is very leaky at the momement.  That's easily fixable though.

Certainly it looks to be in a state where I could take it and clean it up and incorporate it into the source code repository if you like, or you could carry on cleaning it up if you prefer.

Richard Frith-Macdonald <CaS>
Group Member
Wed 04 Aug 2021 03:24:53 PM UTC, comment #12: 

Hello

Could you please review this change?
https://github.com/manu0401/libs-base/commit/8037f880f729f210d1c2428b2df7c97a0093c250

It is far from being pretty, but it brings full tzfile v2+ and leap seconds support. NSTimeZone tests show no regression.

Emmanuel Dreyfus <manubsd>
Fri 30 Jul 2021 09:04:26 AM UTC, comment #11: 

Thanks for looking at this.

> it is tempting to toss the part of NSTimeZone that deals with transitions


There is no problem throwing away old code as long as the new code does the job as well or better.  However, the more that's changed, the more we ought to improve/extend the automated regression tests in the Tests/base/NSTimeZone directory.

Richard Frith-Macdonald <CaS>
Group Member
Thu 29 Jul 2021 01:06:05 PM UTC, comment #10: 

A quick update, I got success with the graft of NetBSD libc's localtime.c in NSTimeZone. I do not post a patch for now because the code is awful, but at least it produces the correct result:

TZ Europe/Paris
1981-01-16 TZ offset = 3600 (expects 3600)
1981-08-16 TZ offset = 7200 (expects 7200)
2021-01-16 TZ offset = 3600 (expects 3600)
2021-08-16 TZ offset = 7200 (expects 7200)

While there it is tempting to toss the part of NSTimeZone that deals with transitions, since it is handled by the imported code.

Emmanuel Dreyfus <manubsd>
Sun 25 Jul 2021 06:58:45 AM UTC, comment #9: 


> adding a wrapper function around chop() calls to check is when is after the last transition,
> and on in this case give the answer based on the TZstring.


I think this is the best approach since the timeZoneDetailForDate method is actually pretty much obsolete: it was removed from Apple's API many years ago.  In GNUstep we aim to retain backward compatibility for a very long time (which is why the method is still available), but this method will never have returned a meaningful result for these future dates anyway, so your proposed change does not break anything.

It seems a good idea to me.

Richard Frith-Macdonald <CaS>
Group Member
Sun 25 Jul 2021 06:48:05 AM UTC, comment #8: 


> but I note this bug:


Either the value of version is 1 (the value it's initialised to), or the version is not 1 (and is set to header->tzh_version[0])

>   if (1 == version)
> That will always be false.


No, that will be false only if it's a version 2 or later (because for a version 1 file the variable was initialised to 1).

However, you are right to point out that tzh_version[0] is an ascii character for version2+, so the assignment should really convert that to an integer.  I'll change it to subtract '0' when setting the variable.

Richard Frith-Macdonald <CaS>
Group Member
Sun 25 Jul 2021 01:21:39 AM UTC, comment #7: 

comment #5:

> I think using public domain code is fine, as long as we include comments around it to make it clear that it is public domain (and where it came from).


How do you want v2+ TZstring-based transitions to be implemented?

We can generate transitions based on the TZstring up to a given date in the future. The advantage is that there is no need to change the code that uses chop to find a transition. But it will waste memory

The other approach is adding a wrapper function around chop() calls to check is when is after the last transition, and on in this case give the answer based on the TZstring. But if we do that, the date returned by timeZoneDetailForDate() will be of little sense to a consumer unaware of v2+ TZfile.



Emmanuel Dreyfus <manubsd>
Sun 25 Jul 2021 12:45:37 AM UTC, comment #6: 

comment #1:

> The new code is now in the git repository;  please give it a try and update this issue to let me know how you got on with it.


Not yes tested, but I note this bug:

    version = header->tzh_version[0];  
Now version is either '\0' (for version 1), '2' or '3'

    if (1 == version)
That will always be false.

If you want version to be the version as an int, you could write:
     version = header->tzh_version[0] ? header->tzh_version[0] - 0x30 : 1;

Emmanuel Dreyfus <manubsd>
Sat 24 Jul 2021 03:57:40 PM UTC, comment #5: 

I think using public domain code is fine, as long as we include comments around it to make it clear that it is public domain (and where it came from).

Richard Frith-Macdonald <CaS>
Group Member
Sat 24 Jul 2021 12:52:46 AM UTC, comment #4: 


comment #3:

> Unfortunately, we don't have existing code to parse/implement the tzstring extension; 


At least on NetBSD, tzalloc() is in public domain. It does all the tzstring parsing, in tzparse(), see here:
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/timea/localtime.c?rev=1.123

It is used by zdump, also in public domain:
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/time/zdump.c?rev=1.53

Would it be acceptable to include that code?

Emmanuel Dreyfus <manubsd>
Fri 23 Jul 2021 09:23:27 AM UTC, comment #3: 

Unfortunately, we don't have existing code to parse/implement the tzstring extension;  that would need to be written from scratch.

Richard Frith-Macdonald <CaS>
Group Member
Thu 22 Jul 2021 01:56:36 PM UTC, comment #2: 

Thank you for fixing it, unfortunately I overlooked a huge part of the required support for tzfile v2+, and my fix worked by chance.

The Europe/Paris last transition is in 1996. After that date (or if there was no transition at all), RFC 8536 says date should be computed using the last TZstring of the footer.

The footer is after the v2+ data and is a list of nul-terminated strings. Here is the example for Europe/Paris
00000400  00 00 00 0d 00 00 0e 10  00 11 00 00 1c 20 01 15  |............. ..|
00000410  00 00 1c 20 01 1a 4c 4d  54 00 50 4d 54 00 57 45  |... ..LMT.PMT.WE|
00000420  53 54 00 57 45 54 00 43  45 54 00 43 45 53 54 00  |ST.WET.CET.CEST.|
00000430  57 45 4d 54 00 0a 43 45  54 2d 31 43 45 53 54 2c  |WEMT..CET-1CEST,|
00000440  4d 33 2e 35 2e 30 2c 4d  31 30 2e 35 2e 30 2f 33  |M3.5.0,M10.5.0/3|
00000450  0a                                                |.|
00000451

Last string is CET-1CEST,M3.5.0,M10.5.0/3 which is in the very versatile format described in section 8.3 of POSIX base definitions
https://pubs.opengroup.org/onlinepubs/9699919799/

I hope you already have a parser for that. Here it means CET-1 until last sunday of march (implicit hour 2:00), and CEST until last sunday of octobre 3:00, but there are more formats.

TZfile version 3 adds two extensions to this format, documented in RFC 8536 section 3.3.1. But at least it would be nice if v2 was supported, as v1 is vulnerable to Y2038 bug.

I attached v2 Europe/Paris if you want to give it a try. You just have to drop it in /usr/share/zoneinfo/Europe/Paris.

Emmanuel Dreyfus <manubsd>
Thu 22 Jul 2021 09:51:41 AM UTC, comment #1: 

Thanks very much for bringing that issue to my attention and providing a patch to fix the problem.

I couldn't use your patch directly (a few bugs and a lack of copyright assignment to the FSF), but have updated NSTimeZone on the lines you suggest. I agree with the idea of always holding transitions in 64bit form, and it won't break anything.

The significant bugs I spotted were a failure to update the variable 'when' in the chop() function to be 64bit (which would make transition lookups fail for dates that won't fit in 32 bits) and allocation of memory for the internal memory using trans_size (32bits on v1) but then unconditionally treating that memory as if it contained 64bit values.  Neither would have shown up if you only tested using v2+ zone info and didn't try looking up transitions for times after the year 2038.
 
The new code is now in the git repository;  please give it a try and update this issue to let me know how you got on with it.

Thanks again for pointing this bug out.

Richard Frith-Macdonald <CaS>
Group Member
Wed 21 Jul 2021 03:43:09 PM UTC, original submission:  

RFC 8536 defines 3 formats for tzfile. NSTimeZone only knows about version 1, and reports bad data without error when dealing with version 2+. I tracked down this patch because I discovered SOGo have been wrecking gmtoffset for years.

Attached is a patch that implements enough of version 2 so that NSTimeZone can report correct gmtoffset with a version 2+.

I had to change trans from int32_t to int64_t in the interface. I guess it could break other code. If that is better, I can refactor the code to ignore transitions that do not fit in in32_t.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #51766:  localtime2.tgz added by manubsd (3KiB - application/x-gzip)
file #51765:  localtime.tgz added by manubsd (3KiB - application/x-gzip)
file #51681:  Paris added by manubsd (1KiB - application/octet-stream - tzfile v2 to drop in /usr/share/zoneinfo/Europe/Paris for testing)
file #51678:  tzfile2.patch added by None (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by manubsd (Posted a comment)
  • -email is unavailable- added by CaS (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-30 CaS Open/ClosedIn Test Closed
    2021-08-12 manubsd Attached File- Added localtime2.tgz, #51766
    2021-08-12 manubsd Attached File- Added localtime.tgz, #51765
    2021-07-22 manubsd Attached File- Added Paris, #51681
    2021-07-22 CaS StatusNone Fixed
        Open/ClosedOpen In Test
    2021-07-21 None Attached File- Added tzfile2.patch, #51678

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code