After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730332 - glib uses wrong timezone transition with zoneinfo 2014c
glib uses wrong timezone transition with zoneinfo 2014c
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: datetime
2.40.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 730333 730426 730741 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-05-18 14:21 UTC by Marien Zwart
Modified: 2018-05-24 16:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Australia/Sydney using tzcode and tzdata 2014c (2.16 KB, application/octet-stream)
2014-05-21 09:44 UTC, Marien Zwart
  Details
Fixes the overflow problem (761 bytes, patch)
2014-05-23 22:21 UTC, John Ralls
reviewed Details | Review
Gurards against reading past the end of a truncated time zone file. (2.81 KB, patch)
2014-05-23 22:24 UTC, John Ralls
reviewed Details | Review

Description Marien Zwart 2014-05-18 14:21:28 UTC
Noticed this because the clock in gnome-panel was 5 minutes off. This seems to be caused by a change in tzcode 2014c, which violates an assumption in gtimezone.c.

The change is that the zoneinfo files now contain a transition at the minimum time value (G_MININT64). gdatetime.c internally uses intervals, with interval 0 being before the first transition. The end time of interval 0 is calculated as the time of the first transition - 1, which now wraps.

I confirmed g_time_zone_find_interval returns 0 when called on the Australia/Sydney timezone with G_TIME_TYPE_UNIVERSAL and the current time. That gets me the timezone "LMT" and an offset of 36292 (hence the clock being a few minutes off). gdb confirmed the first transition is at -9223372036854775808, and a script I threw together to dump the zoneinfo file confirmed that transition is really in there.

The actual overflow occurs in interval_end (on "return (TRANSITION(interval)).time - 1"), but that may not be the only spot affected.

Note tzdata 2014c is fine, as long as it's compiled with an older zic. For example, Debian testing's tzdata-2014c-1 is unaffected, as it seems to be built using libc-bin's zic, which does not have this change yet.

The change to zic is announced on tz-announce (http://mm.icann.org/pipermail/tz-announce/2014-May/000020.html): "zic now generates transitions for minimum time values, eliminating guesswork when handling low-valued time stamps" (that's probably https://github.com/eggert/tz/commit/7fb077a9ff67dab22b9a23f64f65f85d59cf593e ). Unfortunately it looks like it actually broke glib's "guesswork".

On an unrelated note, init_zone_from_iana_info looks like it should be validating "size" after parsing the header (and again after parsing the second header). It will currently read beyond the end of the buffer returned by g_bytes_get_data if fed a truncated zoneinfo file, unless I'm overlooking validation elsewhere.
Comment 1 Olav Vitters 2014-05-20 08:12:29 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=730426 for the bug reported against gnome-shell. Many people at Mageia are seeing the same.
Comment 2 Jasper St. Pierre (not reading bugmail) 2014-05-20 17:41:51 UTC
*** Bug 730426 has been marked as a duplicate of this bug. ***
Comment 3 Allison Karlitskaya (desrt) 2014-05-20 21:14:03 UTC
An example of a (binary) file that causes the break would help a lot.
Comment 4 Marien Zwart 2014-05-21 09:44:50 UTC
Created attachment 276922 [details]
Australia/Sydney using tzcode and tzdata 2014c
Comment 5 John Ralls 2014-05-22 22:40:03 UTC
*** Bug 730333 has been marked as a duplicate of this bug. ***
Comment 6 John Ralls 2014-05-23 22:21:44 UTC
Created attachment 277089 [details] [review]
Fixes the overflow problem
Comment 7 John Ralls 2014-05-23 22:24:51 UTC
Created attachment 277090 [details] [review]
Gurards against reading past the end of a truncated time zone file.

The second patch will process the 32-bit data if the file is long enough to contain all of the 32-bit data but not all of the 64-bit data. One could argue that if the file isn't long enough to contain all of the data it should that it's defective and shouldn't be used at all.
Comment 8 Olivier Blin 2014-05-23 23:41:01 UTC
I confirm these patches fix the issue in gnome-shell (Mageia Cauldron).
Thanks!
Comment 9 Marien Zwart 2014-05-24 05:08:54 UTC
Confirmed glib 2.38.2 with those two patches applied behaves correctly on the zoneinfo file attached to this bug: for G_MININT64 g_time_zone_find_interval finds interval 0, for G_MININT64 + 1 interval 1, and for the current time interval 96, which has the correct offset of 10 hours. Thanks!

Had a quick look at the code to see if empty intervals (interval 0 and 1 now both start at G_MININT64) work correctly, and didn't see any obvious problems. Didn't look very hard, though.

The "datasize64" calculation in that second patch assumes the 32-bit header and 64-bit header match. That is not necessarily true. For example, the Australia/Sydney file I attached to this bug has 216 transitions in the 32-bit section and 217 in the 64-bit section. It should check if the second header fits (size >= (2 * sizeof (struct tzhead) + datasize32), skip ahead, calculate datasize64 from the new header, and then check if the data fits.

(as long as the 64-bit section has more entries than the 32-bit section this makes the new check not as strict as it should be. If there are crazy zoneinfo files with more transitions in the 32-bit section it'd incorrectly ignore the 64-bit section now.)
Comment 10 Kalev Lember 2014-05-26 08:15:13 UTC
*** Bug 730741 has been marked as a duplicate of this bug. ***
Comment 11 Dan Winship 2014-06-07 12:40:55 UTC
Comment on attachment 277089 [details] [review]
Fixes the overflow problem

So this means that with new-style zone files, there will be an empty interval 0 (from G_MININT64 to G_MININT64), and then interval 1 will be the first real interval. While inelegant, I don't think this actually breaks anything.

Other than that, it seems right.
Comment 12 Dan Winship 2014-06-07 12:56:46 UTC
Comment on attachment 277090 [details] [review]
Gurards against reading past the end of a truncated time zone file.

>+  datasize32 = guint32_from_be(header->tzh_ttisgmtcnt) +
>+    guint32_from_be(header->tzh_ttisstdcnt) +
>+    sizeof (gint32) * 2 * guint32_from_be(header->tzh_leapcnt) +
>+    (sizeof (gint32) + sizeof (gint8)) * guint32_from_be(header->tzh_timecnt) +
>+    sizeof(struct ttinfo) * guint32_from_be(header->tzh_typecnt) +
>+    guint32_from_be(header->tzh_charcnt);
>+
>+  datasize64 = guint32_from_be(header->tzh_ttisgmtcnt) +
>+    guint32_from_be(header->tzh_ttisstdcnt) +
>+    sizeof (gint32) * 2 * guint32_from_be(header->tzh_leapcnt) +
>+    (sizeof (gint64) + sizeof (gint8)) * guint32_from_be(header->tzh_timecnt) +
>+    sizeof(struct ttinfo) * guint32_from_be(header->tzh_typecnt) +
>+    guint32_from_be(header->tzh_charcnt);

This would be nicer with a macro:

  #define TZ_DATA_SIZE(header, element_size) (...)

  datasize32 = TZ_DATA_SIZE (header, 4);
  datasize64 = TZ_DATA_SIZE (header, 8);

>+  g_return_if_fail (size >= (sizeof (struct tzhead) + datasize32) &&
>                     memcmp (header, "TZif", 4) == 0);
> 
>-  if (header->tzh_version == '2')
>+  if (header->tzh_version == '2' &&
>+      size >= (2 * sizeof (struct tzhead) + datasize32 + datasize64))

I think I'd keep the original size>=sizeof(tzhead) check, and then check tzh_version, and then have separate return_if_fails for the version 0 and version 2 cases. (Where version 2 is required to have the 64-bit data.)
Comment 13 Marien Zwart 2014-06-07 13:34:54 UTC
Note tzcode now works around this in their version 2014d, see http://mm.icann.org/pipermail/tz-announce/2014-May/000021.html. Might still be worth fixing, of course.
Comment 14 André Klapper 2014-06-18 14:53:37 UTC
Anybody planning to update the patches in comment 6 and comment 7?
Comment 15 André Klapper 2014-08-19 20:34:33 UTC
Anybody planning to update John Ralls' patches in comment 6 and comment 7?
Comment 16 Dan Winship 2014-08-19 21:36:26 UTC
I think people mostly stopped caring, since it tzdata-2014d readjusted things to work correctly again. Should still get in at some point, but it's not urgent.
Comment 17 Paul Eggert 2016-05-01 08:33:54 UTC
(In reply to Dan Winship from comment #16)
> I think people mostly stopped caring, since it tzdata-2014d readjusted
> things to work correctly again. Should still get in at some point, but it's
> not urgent.

It's been nearly two years now. Could you please boost the priority of fixing this? I am installing into tzcode a patch to make it easier to disable tzcode's workaround for the GNOME bug, with the idea that the GNOME bug will be fixed. Thanks.
Comment 18 GNOME Infrastructure Team 2018-05-24 16:33:36 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/878.