[tz] Thread Safety Notes
Paul Eggert
eggert at cs.ucla.edu
Mon Sep 22 05:48:21 UTC 2014
Andy Heninger wrote:
> If it were up to me, I would probably remove use of the double-checked
> idiom, and rely on solely lock(). Slightly slower, but simple, portable and
> safe.
Thanks very much for the review. Your suggestion makes gmtime 13%
slower on my platform (Fedora 20 x86-64) when THREAD_SAFE is defined,
due to the locking overhead, but safe is better than potentially-buggy.
Attached are three proposed patches that should address the problems you
raise. Patch 1 fixes the memory leak race in gmtcheck. Patch 2 should
port to the C11 memory model. Patch 3 should work around the resulting
gmtime slowdown in zdump by having zdump use localtime_rz instead of
gmtime to convert time stamps to UTC.
-------------- next part --------------
From 0eb01e8a0c2a3c8e20c56dd029d74f0d8a95af00 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert at cs.ucla.edu>
Date: Sat, 20 Sep 2014 21:22:06 -0500
Subject: [PROPOSED PATCH 1/3] Fix memory leak if ALL_STATE and multithreaded.
* localtime.c (gmtcheck) [ALL_STATE]: Don't leak memory if two
threads invoke this function nearly simultaneously.
Problem reported by Andy Heninger in:
http://mm.icann.org/pipermail/tz/2014-September/021599.html
* NEWS: Document this.
---
NEWS | 4 ++++
localtime.c | 10 ++++++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/NEWS b/NEWS
index 29cf395..56ae660 100644
--- a/NEWS
+++ b/NEWS
@@ -30,6 +30,10 @@ Unreleased, experimental changes
An access to uninitalized data has been fixed.
(Thanks to Jörg Richter for reporting the problem.)
+ A memory leak has been fixed if ALL_STATE and THREAD_SAFE are defined
+ and two threads race to initialize data used by gmtime-like functions.
+ (Thanks to Andy Heninger for reporting the problem.)
+
Changes affecting build procedure
'make check' now checks better for properly-sorted data.
diff --git a/localtime.c b/localtime.c
index 62743e2..ad1241b 100644
--- a/localtime.c
+++ b/localtime.c
@@ -1249,12 +1249,14 @@ gmtcheck(void)
return;
if (lock() != 0)
return;
+ if (! gmt_is_set) {
#ifdef ALL_STATE
- gmtptr = malloc(sizeof *gmtptr);
+ gmtptr = malloc(sizeof *gmtptr);
#endif
- if (gmtptr)
- gmtload(gmtptr);
- gmt_is_set = true;
+ if (gmtptr)
+ gmtload(gmtptr);
+ gmt_is_set = true;
+ }
unlock();
}
--
1.9.3
-------------- next part --------------
From 9f224b2078c41649355b34b4bbe0d88759a3776c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert at cs.ucla.edu>
Date: Sun, 21 Sep 2014 08:29:03 -0700
Subject: [PROPOSED PATCH 2/3] port to C11 memory model
We don't know of any problems with the previous code on practical
platforms, but it's safer to be portable.
Problem reported by Andy Heninger in:
http://mm.icann.org/pipermail/tz/2014-September/021599.html
* localtime.c (VOLATILE): Remove. All uses removed.
(gmtcheck): Don't access gmt_is_set until we have the lock.
This may be significantly slower, but it's safer.
(localtime_tzset): Likewise. This change isn't significantly
slower, though; it's more of a refactoring.
* NEWS: Document this.
---
NEWS | 3 ++-
localtime.c | 12 ++++--------
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/NEWS b/NEWS
index 56ae660..21e85a7 100644
--- a/NEWS
+++ b/NEWS
@@ -30,9 +30,10 @@ Unreleased, experimental changes
An access to uninitalized data has been fixed.
(Thanks to Jörg Richter for reporting the problem.)
+ When THREAD_SAFE is defined, the code ports to the C11 memory model.
A memory leak has been fixed if ALL_STATE and THREAD_SAFE are defined
and two threads race to initialize data used by gmtime-like functions.
- (Thanks to Andy Heninger for reporting the problem.)
+ (Thanks to Andy Heninger for reporting the problems.)
Changes affecting build procedure
diff --git a/localtime.c b/localtime.c
index ad1241b..30b5fbc 100644
--- a/localtime.c
+++ b/localtime.c
@@ -18,12 +18,10 @@
#if THREAD_SAFE
# include <pthread.h>
-# define VOLATILE volatile
static pthread_mutex_t locallock = PTHREAD_MUTEX_INITIALIZER;
static int lock(void) { return pthread_mutex_lock(&locallock); }
static void unlock(void) { pthread_mutex_unlock(&locallock); }
#else
-# define VOLATILE
static int lock(void) { return 0; }
static void unlock(void) { }
#endif
@@ -176,7 +174,7 @@ static struct state gmtmem;
#endif /* !defined TZ_STRLEN_MAX */
static char lcl_TZname[TZ_STRLEN_MAX + 1];
-static int VOLATILE lcl_is_set;
+static int lcl_is_set;
char * tzname[2] = {
(char *) wildabbr,
@@ -1244,9 +1242,7 @@ tzset(void)
static void
gmtcheck(void)
{
- static bool VOLATILE gmt_is_set;
- if (gmt_is_set)
- return;
+ static bool gmt_is_set;
if (lock() != 0)
return;
if (! gmt_is_set) {
@@ -1400,7 +1396,7 @@ localtime_tzset(time_t const *timep, struct tm *tmp, bool settz, bool setname)
errno = err;
return NULL;
}
- if (settz)
+ if (settz || !lcl_is_set)
tzset_unlocked();
tmp = localsub(lclptr, timep, setname, tmp);
unlock();
@@ -1416,7 +1412,7 @@ localtime(const time_t *const timep)
struct tm *
localtime_r(const time_t *const timep, struct tm *tmp)
{
- return localtime_tzset(timep, tmp, lcl_is_set == 0, false);
+ return localtime_tzset(timep, tmp, false, false);
}
/*
--
1.9.3
-------------- next part --------------
From 3d7af33d76348b90156a50adace91cca8e17823d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert at cs.ucla.edu>
Date: Sun, 21 Sep 2014 16:45:04 -0700
Subject: [PROPOSED PATCH 3/3] Prefer localtime_rz to gmtime, for speed.
localtime_rz does not need to lock, but gmtime does.
* zdump.c (gmtime_r) [!HAVE_LOCALTIME_R]: New function.
(USE_LOCALTIME_RZ): New macro. Use it to simplify ifdef usage.
(gmtz): New static var.
(gmtzinit, my_gmtime_r): New functions.
(main): Use them.
---
zdump.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/zdump.c b/zdump.c
index 0735c2a..0aa94fc 100644
--- a/zdump.c
+++ b/zdump.c
@@ -304,10 +304,36 @@ sumsize(size_t a, size_t b)
static void tzset(void) { }
#endif
+/* Assume gmtime_r works if localtime_r does.
+ A replacement localtime_r is defined below if needed. */
+#if ! HAVE_LOCALTIME_R
+
+# undef gmtime_r
+# define gmtime_r zdump_gmtime_r
+
+static struct tm *
+gmtime_r(time_t *tp, struct tm *tmp)
+{
+ struct tm *r = gmtime(tp);
+ if (r) {
+ *tmp = *r;
+ r = tmp;
+ }
+ return r;
+}
+
+#endif
+
/* Platforms with TM_ZONE don't need tzname, so they can use the
faster localtime_rz or localtime_r if available. */
-#if !defined TM_ZONE || ! HAVE_LOCALTIME_RZ
+#if defined TM_ZONE && HAVE_LOCALTIME_RZ
+# define USE_LOCALTIME_RZ true
+#else
+# define USE_LOCALTIME_RZ false
+#endif
+
+#if ! USE_LOCALTIME_RZ
# if !defined TM_ZONE || ! HAVE_LOCALTIME_R || ! HAVE_TZSET
# undef localtime_r
@@ -386,7 +412,31 @@ tzfree(timezone_t env)
environ = env + 1;
free(env[0]);
}
-#endif /* ! HAVE_LOCALTIME_RZ */
+#endif /* ! USE_LOCALTIME_RZ */
+
+/* A UTC time zone, and its initializer. */
+static timezone_t gmtz;
+static void
+gmtzinit(void)
+{
+ if (USE_LOCALTIME_RZ) {
+ static char const utc[] = "UTC0";
+ gmtz = tzalloc(utc);
+ if (!gmtz) {
+ perror(utc);
+ exit(EXIT_FAILURE);
+ }
+ }
+}
+
+/* Convert *TP to UTC, storing the broken-down time into *TMP.
+ Return TMP if successful, NULL otherwise. This is like gmtime_r(TP, TMP),
+ except typically faster if USE_LOCALTIME_RZ. */
+static struct tm *
+my_gmtime_r(time_t *tp, struct tm *tmp)
+{
+ return USE_LOCALTIME_RZ ? localtime_rz(gmtz, tp, tmp) : gmtime_r(tp, tmp);
+}
#ifndef TYPECHECK
# define my_localtime_rz localtime_rz
@@ -625,6 +675,7 @@ main(int argc, char *argv[])
}
}
}
+ gmtzinit();
now = time(NULL);
longest = 0;
for (i = optind; i < argc; i++) {
@@ -808,7 +859,7 @@ show(timezone_t tz, char *zone, time_t t, bool v)
printf("%-*s ", longest, zone);
if (v) {
- tmp = gmtime(&t);
+ tmp = my_gmtime_r(&t, &tm);
if (tmp == NULL) {
printf(tformat(), t);
} else {
@@ -895,7 +946,7 @@ dumptime(register const struct tm *timeptr)
return;
}
/*
- ** The packaged localtime_rz and gmtime never put out-of-range
+ ** The packaged localtime_rz and gmtime_r never put out-of-range
** values in tm_wday or tm_mon, but since this code might be compiled
** with other (perhaps experimental) versions, paranoia is in order.
*/
--
1.9.3
More information about the tz
mailing list