GNOME Bugzilla – Bug 730332
glib uses wrong timezone transition with zoneinfo 2014c
Last modified: 2018-05-24 16:33:36 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.
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.
*** Bug 730426 has been marked as a duplicate of this bug. ***
An example of a (binary) file that causes the break would help a lot.
Created attachment 276922 [details] Australia/Sydney using tzcode and tzdata 2014c
*** Bug 730333 has been marked as a duplicate of this bug. ***
Created attachment 277089 [details] [review] Fixes the overflow problem
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.
I confirm these patches fix the issue in gnome-shell (Mageia Cauldron). Thanks!
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.)
*** Bug 730741 has been marked as a duplicate of this bug. ***
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 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.)
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.
Anybody planning to update the patches in comment 6 and comment 7?
Anybody planning to update John Ralls' patches in comment 6 and comment 7?
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.
(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.
-- 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.