From ce0559a035979db6fcc75a816197a65b140757ce Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 14 Oct 2020 14:42:56 -0500 Subject: [PATCH] Fix timezone-related bugs in many applications caused by new glib timezone cache --- glib2.spec | 8 +- timezone-madness.patch | 599 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 606 insertions(+), 1 deletion(-) create mode 100644 timezone-madness.patch diff --git a/glib2.spec b/glib2.spec index fc7c92c..baea6bc 100644 --- a/glib2.spec +++ b/glib2.spec @@ -1,6 +1,6 @@ Name: glib2 Version: 2.66.1 -Release: 1%{?dist} +Release: 2%{?dist} Summary: A library of handy utility functions License: LGPLv2+ @@ -10,6 +10,9 @@ Source0: http://download.gnome.org/sources/glib/2.66/glib-%{version}.tar.xz # Avoid requiring a too new gtk-doc version for building glib Patch0: gtk-doc-1-32.patch +# https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1661 +Patch1: timezone-madness.patch + BuildRequires: chrpath BuildRequires: gcc BuildRequires: gcc-c++ @@ -217,6 +220,9 @@ glib-compile-schemas %{_datadir}/glib-2.0/schemas &> /dev/null || : %{_datadir}/installed-tests %changelog +* Wed Oct 14 2020 Michael Catanzaro - 2.66.1-2 +- Fix timezone-related bugs in many applications caused by new glib timezone cache + * Thu Oct 1 2020 Kalev Lember - 2.66.1-1 - Update to 2.66.1 diff --git a/timezone-madness.patch b/timezone-madness.patch new file mode 100644 index 0000000..62b945a --- /dev/null +++ b/timezone-madness.patch @@ -0,0 +1,599 @@ +From b4138bd4acc04ad572dfa042a384aaaa6e4621ad Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= +Date: Wed, 23 Sep 2020 18:13:47 +0100 +Subject: [PATCH 1/4] gtimezone: Split out fallback timezone identification for + unix + +When the TZ environment variable is not set, we get the local timezone +identifier by reading specific files. + +We are going to need these identifiers earlier, so split this logic into +its own function, in preparation for the next commit. + +Based on idea proposed by Sebastian Keller . +--- + glib/gtimezone.c | 127 +++++++++++++++++++++++++++-------------------- + 1 file changed, 73 insertions(+), 54 deletions(-) + +diff --git a/glib/gtimezone.c b/glib/gtimezone.c +index ef67ec50b..d90a9bb73 100644 +--- a/glib/gtimezone.c ++++ b/glib/gtimezone.c +@@ -438,11 +438,80 @@ zone_for_constant_offset (GTimeZone *gtz, const gchar *name) + } + + #ifdef G_OS_UNIX ++static gchar * ++zone_identifier_unix (void) ++{ ++ gchar *resolved_identifier = NULL; ++ gsize prefix_len = 0; ++ gchar *canonical_path = NULL; ++ GError *read_link_err = NULL; ++ const gchar *tzdir; ++ ++ /* Resolve the actual timezone pointed to by /etc/localtime. */ ++ resolved_identifier = g_file_read_link ("/etc/localtime", &read_link_err); ++ if (resolved_identifier == NULL) ++ { ++ gboolean not_a_symlink = g_error_matches (read_link_err, ++ G_FILE_ERROR, ++ G_FILE_ERROR_INVAL); ++ g_clear_error (&read_link_err); ++ ++ /* Fallback to the content of /var/db/zoneinfo or /etc/timezone ++ * if /etc/localtime is not a symlink. /var/db/zoneinfo is ++ * where 'tzsetup' program on FreeBSD and DragonflyBSD stores ++ * the timezone chosen by the user. /etc/timezone is where user ++ * choice is expressed on Gentoo OpenRC and others. */ ++ if (not_a_symlink && (g_file_get_contents ("/var/db/zoneinfo", ++ &resolved_identifier, ++ NULL, NULL) || ++ g_file_get_contents ("/etc/timezone", ++ &resolved_identifier, ++ NULL, NULL))) ++ g_strchomp (resolved_identifier); ++ else ++ { ++ /* Error */ ++ g_assert (resolved_identifier == NULL); ++ goto out; ++ } ++ } ++ else ++ { ++ /* Resolve relative path */ ++ canonical_path = g_canonicalize_filename (resolved_identifier, "/etc"); ++ g_free (resolved_identifier); ++ resolved_identifier = g_steal_pointer (&canonical_path); ++ } ++ ++ tzdir = g_getenv ("TZDIR"); ++ if (tzdir == NULL) ++ tzdir = "/usr/share/zoneinfo"; ++ ++ /* Strip the prefix and slashes if possible. */ ++ if (g_str_has_prefix (resolved_identifier, tzdir)) ++ { ++ prefix_len = strlen (tzdir); ++ while (*(resolved_identifier + prefix_len) == '/') ++ prefix_len++; ++ } ++ ++ if (prefix_len > 0) ++ memmove (resolved_identifier, resolved_identifier + prefix_len, ++ strlen (resolved_identifier) - prefix_len + 1 /* nul terminator */); ++ ++ g_assert (resolved_identifier != NULL); ++ ++out: ++ g_free (canonical_path); ++ ++ return resolved_identifier; ++} ++ + static GBytes* + zone_info_unix (const gchar *identifier, + gchar **out_identifier) + { +- gchar *filename; ++ gchar *filename = NULL; + GMappedFile *file = NULL; + GBytes *zoneinfo = NULL; + gchar *resolved_identifier = NULL; +@@ -470,61 +539,11 @@ zone_info_unix (const gchar *identifier, + } + else + { +- gsize prefix_len = 0; +- gchar *canonical_path = NULL; +- GError *read_link_err = NULL; +- +- filename = g_strdup ("/etc/localtime"); +- +- /* Resolve the actual timezone pointed to by /etc/localtime. */ +- resolved_identifier = g_file_read_link (filename, &read_link_err); ++ resolved_identifier = zone_identifier_unix (); + if (resolved_identifier == NULL) +- { +- gboolean not_a_symlink = g_error_matches (read_link_err, +- G_FILE_ERROR, +- G_FILE_ERROR_INVAL); +- g_clear_error (&read_link_err); +- +- /* Fallback to the content of /var/db/zoneinfo or /etc/timezone +- * if /etc/localtime is not a symlink. /var/db/zoneinfo is +- * where 'tzsetup' program on FreeBSD and DragonflyBSD stores +- * the timezone chosen by the user. /etc/timezone is where user +- * choice is expressed on Gentoo OpenRC and others. */ +- if (not_a_symlink && (g_file_get_contents ("/var/db/zoneinfo", +- &resolved_identifier, +- NULL, NULL) || +- g_file_get_contents ("/etc/timezone", +- &resolved_identifier, +- NULL, NULL))) +- g_strchomp (resolved_identifier); +- else +- { +- /* Error */ +- g_assert (resolved_identifier == NULL); +- goto out; +- } +- } +- else +- { +- /* Resolve relative path */ +- canonical_path = g_canonicalize_filename (resolved_identifier, "/etc"); +- g_free (resolved_identifier); +- resolved_identifier = g_steal_pointer (&canonical_path); +- } ++ goto out; + +- /* Strip the prefix and slashes if possible. */ +- if (g_str_has_prefix (resolved_identifier, tzdir)) +- { +- prefix_len = strlen (tzdir); +- while (*(resolved_identifier + prefix_len) == '/') +- prefix_len++; +- } +- +- if (prefix_len > 0) +- memmove (resolved_identifier, resolved_identifier + prefix_len, +- strlen (resolved_identifier) - prefix_len + 1 /* nul terminator */); +- +- g_free (canonical_path); ++ filename = g_strdup ("/etc/localtime"); + } + + file = g_mapped_file_new (filename, FALSE, NULL); +-- +GitLab + + +From 7e59a4c0d5ab8e08fe2cf596fb9512708e183de8 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= +Date: Wed, 23 Sep 2020 18:28:40 +0100 +Subject: [PATCH 2/4] gtimezone: Set resolved_identifier earlier + +We have been passing a &resolved_identifier address around for multiple +functions to set it. Each function may either: + + 1. leaving it for the next function to set, if returning early; + 2. set it to a duplicate of the passed identifier, if not NULL; + 3. get a fallback value and set it, otherwise. + +This can be simplified by setting it early to either: + + 1. a duplicate of the passed identifier, if not NULL; + 2. a fallback value, otherwise. + +This way we can avoid some unnecessary string duplication and freeing. +Also, on Windows, we avoid calling windows_default_tzname() twice. + +But the main motivation for this change is enabling the performance +optimization in the next commit. +--- + glib/gtimezone.c | 76 ++++++++++++++++-------------------------------- + 1 file changed, 25 insertions(+), 51 deletions(-) + +diff --git a/glib/gtimezone.c b/glib/gtimezone.c +index d90a9bb73..40064c9ab 100644 +--- a/glib/gtimezone.c ++++ b/glib/gtimezone.c +@@ -508,13 +508,12 @@ out: + } + + static GBytes* +-zone_info_unix (const gchar *identifier, +- gchar **out_identifier) ++zone_info_unix (const gchar *identifier, ++ const gchar *resolved_identifier) + { + gchar *filename = NULL; + GMappedFile *file = NULL; + GBytes *zoneinfo = NULL; +- gchar *resolved_identifier = NULL; + const gchar *tzdir; + + tzdir = g_getenv ("TZDIR"); +@@ -527,8 +526,6 @@ zone_info_unix (const gchar *identifier, + glibc allows both syntaxes, so we should too */ + if (identifier != NULL) + { +- resolved_identifier = g_strdup (identifier); +- + if (*identifier == ':') + identifier ++; + +@@ -539,7 +536,6 @@ zone_info_unix (const gchar *identifier, + } + else + { +- resolved_identifier = zone_identifier_unix (); + if (resolved_identifier == NULL) + goto out; + +@@ -559,10 +555,6 @@ zone_info_unix (const gchar *identifier, + g_assert (resolved_identifier != NULL); + + out: +- if (out_identifier != NULL) +- *out_identifier = g_steal_pointer (&resolved_identifier); +- +- g_free (resolved_identifier); + g_free (filename); + + return zoneinfo; +@@ -834,14 +826,13 @@ register_tzi_to_tzi (RegTZI *reg, TIME_ZONE_INFORMATION *tzi) + + static guint + rules_from_windows_time_zone (const gchar *identifier, +- gchar **out_identifier, +- TimeZoneRule **rules, +- gboolean copy_identifier) ++ const gchar *resolved_identifier, ++ TimeZoneRule **rules) + { + HKEY key; + gchar *subkey = NULL; + gchar *subkey_dynamic = NULL; +- gchar *key_name = NULL; ++ const gchar *key_name; + const gchar *reg_key = + "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion\\Time Zones\\"; + TIME_ZONE_INFORMATION tzi; +@@ -856,19 +847,15 @@ rules_from_windows_time_zone (const gchar *identifier, + if (GetSystemDirectoryW (winsyspath, MAX_PATH) == 0) + return 0; + +- g_assert (copy_identifier == FALSE || out_identifier != NULL); + g_assert (rules != NULL); + +- if (copy_identifier) +- *out_identifier = NULL; +- + *rules = NULL; + key_name = NULL; + + if (!identifier) +- key_name = windows_default_tzname (); ++ key_name = resolved_identifier; + else +- key_name = g_strdup (identifier); ++ key_name = identifier; + + if (!key_name) + return 0; +@@ -1011,16 +998,9 @@ utf16_conv_failed: + else + (*rules)[rules_num - 1].start_year = (*rules)[rules_num - 2].start_year + 1; + +- if (copy_identifier) +- *out_identifier = g_steal_pointer (&key_name); +- else +- g_free (key_name); +- + return rules_num; + } + +- g_free (key_name); +- + return 0; + } + +@@ -1521,16 +1501,13 @@ parse_identifier_boundaries (gchar **pos, TimeZoneRule *tzr) + */ + static guint + rules_from_identifier (const gchar *identifier, +- gchar **out_identifier, + TimeZoneRule **rules) + { + gchar *pos; + TimeZoneRule tzr; + +- g_assert (out_identifier != NULL); + g_assert (rules != NULL); + +- *out_identifier = NULL; + *rules = NULL; + + if (!identifier) +@@ -1545,7 +1522,6 @@ rules_from_identifier (const gchar *identifier, + + if (*pos == 0) + { +- *out_identifier = g_strdup (identifier); + return create_ruleset_from_rule (rules, &tzr); + } + +@@ -1566,14 +1542,8 @@ rules_from_identifier (const gchar *identifier, + /* Use US rules, Windows' default is Pacific Standard Time */ + if ((rules_num = rules_from_windows_time_zone ("Pacific Standard Time", + NULL, +- rules, +- FALSE))) ++ rules))) + { +- /* We don't want to hardcode our identifier here as +- * "Pacific Standard Time", use what was passed in +- */ +- *out_identifier = g_strdup (identifier); +- + for (i = 0; i < rules_num - 1; i++) + { + (*rules)[i].std_offset = - tzr.std_offset; +@@ -1594,7 +1564,6 @@ rules_from_identifier (const gchar *identifier, + if (!parse_identifier_boundaries (&pos, &tzr)) + return 0; + +- *out_identifier = g_strdup (identifier); + return create_ruleset_from_rule (rules, &tzr); + } + +@@ -1605,17 +1574,13 @@ parse_footertz (const gchar *footer, size_t footerlen) + gchar *tzstring = g_strndup (footer + 1, footerlen - 2); + GTimeZone *footertz = NULL; + +- /* FIXME: it might make sense to modify rules_from_identifier to +- allow NULL to be passed instead of &ident, saving the strdup/free +- pair. The allocation for tzstring could also be avoided by ++ /* FIXME: The allocation for tzstring could be avoided by + passing a gsize identifier_len argument to rules_from_identifier + and changing the code in that function to stop assuming that + identifier is nul-terminated. */ +- gchar *ident; + TimeZoneRule *rules; +- guint rules_num = rules_from_identifier (tzstring, &ident, &rules); ++ guint rules_num = rules_from_identifier (tzstring, &rules); + +- g_free (ident); + g_free (tzstring); + if (rules_num > 1) + { +@@ -1723,6 +1688,16 @@ g_time_zone_new (const gchar *identifier) + G_UNLOCK (time_zones); + return tz; + } ++ else ++ resolved_identifier = g_strdup (identifier); ++ } ++ else ++ { ++#ifdef G_OS_UNIX ++ resolved_identifier = zone_identifier_unix (); ++#elif defined (G_OS_WIN32) ++ resolved_identifier = windows_default_tzname (); ++#endif + } + + tz = g_slice_new0 (GTimeZone); +@@ -1731,7 +1706,7 @@ g_time_zone_new (const gchar *identifier) + zone_for_constant_offset (tz, identifier); + + if (tz->t_info == NULL && +- (rules_num = rules_from_identifier (identifier, &resolved_identifier, &rules))) ++ (rules_num = rules_from_identifier (identifier, &rules))) + { + init_zone_from_rules (tz, rules, rules_num, g_steal_pointer (&resolved_identifier)); + g_free (rules); +@@ -1740,7 +1715,7 @@ g_time_zone_new (const gchar *identifier) + if (tz->t_info == NULL) + { + #ifdef G_OS_UNIX +- GBytes *zoneinfo = zone_info_unix (identifier, &resolved_identifier); ++ GBytes *zoneinfo = zone_info_unix (identifier, resolved_identifier); + if (zoneinfo != NULL) + { + init_zone_from_iana_info (tz, zoneinfo, g_steal_pointer (&resolved_identifier)); +@@ -1748,9 +1723,8 @@ g_time_zone_new (const gchar *identifier) + } + #elif defined (G_OS_WIN32) + if ((rules_num = rules_from_windows_time_zone (identifier, +- &resolved_identifier, +- &rules, +- TRUE))) ++ resolved_identifier, ++ &rules))) + { + init_zone_from_rules (tz, rules, rules_num, g_steal_pointer (&resolved_identifier)); + g_free (rules); +@@ -1777,7 +1751,7 @@ g_time_zone_new (const gchar *identifier) + rules[0].start_year = MIN_TZYEAR; + rules[1].start_year = MAX_TZYEAR; + +- init_zone_from_rules (tz, rules, 2, windows_default_tzname ()); ++ init_zone_from_rules (tz, rules, 2, g_steal_pointer (&resolved_identifier)); + } + + g_free (rules); +-- +GitLab + + +From 5237b4984306847dff05db54b5c12c83662f2f1d Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= +Date: Thu, 1 Oct 2020 21:11:44 +0100 +Subject: [PATCH 3/4] gtimezone: Cache default timezone indefinitely + +We cache GTimeZone instances to avoid expensive construction when the +same id is requested again. + +However, if the NULL id is passed to g_time_zone_new(), we always +construct a new instance for the default/fallback timezone. + +With the recent introduction of some heavy calculations[1], repeated +instance construction in such cases has visible performance impact in +nautilus list view and other such GtkTreeView consumers. + +To avoid this, cache reference to a constructed default timezone and +use it the next time g_time_zone_new() is called with NULL argument, +as long as the default identifier doesn't change. We already did the +same for the local timezone[2]. + +Fixes: https://gitlab.gnome.org/GNOME/glib/-/issues/2204 + +Based on idea proposed by Sebastian Keller . + +[1] 25d950b61f92f25cc9ab20d683aa4d6969f93098 +[2] 551e83662de9815d161a82c760cfa77995905740 +--- + glib/gtimezone.c | 37 ++++++++++++++++++++++++++++++++----- + 1 file changed, 32 insertions(+), 5 deletions(-) + +diff --git a/glib/gtimezone.c b/glib/gtimezone.c +index 40064c9ab..035da2f45 100644 +--- a/glib/gtimezone.c ++++ b/glib/gtimezone.c +@@ -195,6 +195,8 @@ struct _GTimeZone + + G_LOCK_DEFINE_STATIC (time_zones); + static GHashTable/**/ *time_zones; ++G_LOCK_DEFINE_STATIC (tz_default); ++static GTimeZone *tz_default = NULL; + G_LOCK_DEFINE_STATIC (tz_local); + static gchar *tzenv_cached = NULL; + static GTimeZone *tz_local = NULL; +@@ -1675,12 +1677,12 @@ g_time_zone_new (const gchar *identifier) + gint rules_num; + gchar *resolved_identifier = NULL; + +- G_LOCK (time_zones); +- if (time_zones == NULL) +- time_zones = g_hash_table_new (g_str_hash, g_str_equal); +- + if (identifier) + { ++ G_LOCK (time_zones); ++ if (time_zones == NULL) ++ time_zones = g_hash_table_new (g_str_hash, g_str_equal); ++ + tz = g_hash_table_lookup (time_zones, identifier); + if (tz) + { +@@ -1693,11 +1695,26 @@ g_time_zone_new (const gchar *identifier) + } + else + { ++ G_LOCK (tz_default); + #ifdef G_OS_UNIX + resolved_identifier = zone_identifier_unix (); + #elif defined (G_OS_WIN32) + resolved_identifier = windows_default_tzname (); + #endif ++ if (tz_default) ++ { ++ /* Flush default if changed */ ++ if (g_strcmp0 (tz_default->name, resolved_identifier)) ++ g_clear_pointer (&tz_default, g_time_zone_unref); ++ else ++ { ++ tz = g_time_zone_ref (tz_default); ++ G_UNLOCK (tz_default); ++ ++ g_free (resolved_identifier); ++ return tz; ++ } ++ } + } + + tz = g_slice_new0 (GTimeZone); +@@ -1773,9 +1790,19 @@ g_time_zone_new (const gchar *identifier) + { + if (identifier) + g_hash_table_insert (time_zones, tz->name, tz); ++ else if (tz->name) ++ { ++ /* Caching reference */ ++ g_atomic_int_inc (&tz->ref_count); ++ tz_default = tz; ++ } + } + g_atomic_int_inc (&tz->ref_count); +- G_UNLOCK (time_zones); ++ ++ if (identifier) ++ G_UNLOCK (time_zones); ++ else ++ G_UNLOCK (tz_default); + + return tz; + } +-- +GitLab + + +From 02753644b33660a175d1a9b1ea9e8717c314ef23 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Ant=C3=B3nio=20Fernandes?= +Date: Wed, 23 Sep 2020 19:36:49 +0100 +Subject: [PATCH 4/4] Revert "gtimezone: Cache timezones based on the + identifier they were created by" + +This reverts commit 851241f19a3fd9ec693b3dd8f37a84c7f970984a. + +That commit avoids a performance regression but introduces a behavior regression: +changes to /etc/localtime have no effect for the remaining of the application's +runtime. + +With the optimization introduced by the previous commit, we can pass NULL to +g_time_zone_new() repeatedly with no performance drawback, so we no longer have +to workaround this case. +--- + glib/gtimezone.c | 13 +++---------- + 1 file changed, 3 insertions(+), 10 deletions(-) + +diff --git a/glib/gtimezone.c b/glib/gtimezone.c +index 035da2f45..8e38bb549 100644 +--- a/glib/gtimezone.c ++++ b/glib/gtimezone.c +@@ -198,7 +198,6 @@ static GHashTable/**/ *time_zones; + G_LOCK_DEFINE_STATIC (tz_default); + static GTimeZone *tz_default = NULL; + G_LOCK_DEFINE_STATIC (tz_local); +-static gchar *tzenv_cached = NULL; + static GTimeZone *tz_local = NULL; + + #define MIN_TZYEAR 1916 /* Daylight Savings started in WWI */ +@@ -1863,17 +1862,11 @@ g_time_zone_new_local (void) + G_LOCK (tz_local); + + /* Is time zone changed and must be flushed? */ +- if (tz_local && g_strcmp0 (tzenv, tzenv_cached) != 0) +- { +- g_clear_pointer (&tz_local, g_time_zone_unref); +- g_clear_pointer (&tzenv_cached, g_free); +- } ++ if (tz_local && g_strcmp0 (g_time_zone_get_identifier (tz_local), tzenv)) ++ g_clear_pointer (&tz_local, g_time_zone_unref); + + if (tz_local == NULL) +- { +- tz_local = g_time_zone_new (tzenv); +- tzenv_cached = g_strdup (tzenv); +- } ++ tz_local = g_time_zone_new (tzenv); + + tz = g_time_zone_ref (tz_local); + +-- +GitLab +