From zyx@no.where Wed Nov 26 15:07:04 2014 Message-ID: <1417010824.9143.20.camel@no.where> Subject: [sf#86] Avoid putenv() in libical From: zyx X-Evolution-Identity: 1342640441.11122.3@zyxPad X-Evolution-Fcc: folder://remote-imap-account/INBOX/sent-mail X-Evolution-Transport: 1342640441.11122.4@zyxPad To: libical-devel@lists.infradead.org X-Evolution-Format: text/plain X-Evolution-Composer-Mode: text/plain Content-Type: multipart/mixed; boundary="=-R5vXJ4rIjFlMFra7xOz5" X-Mailer: Evolution 3.13.9 Date: Wed, 26 Nov 2014 15:07:04 +0100 Mime-Version: 1.0 X-Evolution-Source: local --=-R5vXJ4rIjFlMFra7xOz5 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

Hello,

you've filled:

libical messes with timezone, affecting other threads [sf#86] #86

https://github.com/libical/libical/issues/86

but I do not have an account there, and I'm not going to have one in the near future, thus I'm writing rather here (I'm also not subscribed on the list, thus please keep me in CC when/if replying).

There seems to be an increasing issue with this, in connection with evolution-data-server, which uses libical and is highly multi-threaded. One recent valgrind log shows this:

  Invalid read of size 8

     at 0xAEACBA4: getenv (getenv.c:76)

     by 0xAEA3141: guess_category_value (dcigettext.c:1356)

     by 0xAEA3141: __dcigettext (dcigettext.c:561)

     by 0x31986744: ??? (in /usr/lib64/gio/modules/libgiognutls.so)

     by 0xC5D8771: g_input_stream_read (ginputstream.c:195)

     by 0xC5D8771: g_input_stream_read (ginputstream.c:195)

     by 0xC2F804E: soup_body_input_stream_read_raw (soup-body-input-stream.c:131)

     by 0xC2F804E: soup_body_input_stream_read_chunked (soup-body-input-stream.c:183)

     by 0xC2F804E: read_internal (soup-body-input-stream.c:250)

     by 0xC5D8771: g_input_stream_read (ginputstream.c:195)

     by 0xC3113E0: io_read (soup-message-io.c:646)

     by 0xC311828: io_run_until (soup-message-io.c:864)

     by 0xC312254: io_run (soup-message-io.c:936)

     by 0xC30F044: soup_message_send_request (soup-message-client-io.c:161)

     by 0xC320E5B: soup_session_process_queue_item (soup-session.c:1964)

   Address 0x33700a68 is 296 bytes inside a block of size 408 free'd

     at 0x4C2BB1C: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)

     by 0xAEACDAB: __add_to_environ (setenv.c:141)

     by 0xAEACC50: putenv (putenv.c:78)

     by 0x4C31027: putenv (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)

     by 0xB47DE1E: set_tz (icaltime.c:342)

     by 0xB47EB09: icaltime_as_timet_with_zone (icaltime.c:435)

     by 0x50A55B7: time_from_isodate (e-cal-time-util.c:656)

     by 0x4E4EFC1: e_cal_backend_sexp_func_make_time (e-cal-backend-sexp.c:1392)

     by 0x557C410: e_sexp_term_eval (e-sexp.c:783)

     by 0x557C3E3: e_sexp_term_eval (e-sexp.c:779)

     by 0x557CC6C: term_eval_and (e-sexp.c:287)

     by 0x557C452: e_sexp_term_eval (e-sexp.c:771)

(taken from https://bugzilla.gnome.org/show_bug.cgi?id=701138#c16 )

I understand the above valgrind claim as a thread interleave of libical's putenv() call and libgiognutls' (indirect) getenv() call. There are few libraries involved in this, as you can see. It also shows that any locking on the libical side is useless when the final application uses more than just libical. (That's only to mention, not a complain.)

I tried to address this with as less pain as possible and I feel I found a solution. Let me show you what I've done:

a) I applied the attached test.patch to libical 1.0. It extends libical's make_time() function whether it should be strict (take care of integer overflow) or not. The 64bit systems (like Linux) define time_t as 8 byte sized, thus there the limits do not apply, thus I added the check to it too. Then I used the modified make_time() in icaltime_as_timet_with_zone(), but only to compare whether the time_t returned from this function matches the one from mktime(), that's the assert() at the end.

b) to verify the test.patch I created a simple ical.c test program (attached). It just calls icaltime_as_timet_with_zone(), which does the actual test whether the values match, for various zones and times during the year. If they do not match, then the application aborts. It doesn't abort on my 64bit Linux system during the whole run.

c) the above led me to the final proposed.patch (attached), which removed the putenv() call completely. Actually, there are two places where it's left, in:

libical-1.0/src/test/regression.c:3791

libical-1.0/src/test/timezones.c:61

but those are just tests, no "production" code. The "make test" finished properly with this change too.

Please, consider using this proposed.patch in the next libical release (when is the 1.1 planned, by the way?).

Bye,

Milan

--=-R5vXJ4rIjFlMFra7xOz5 Content-Disposition: attachment; filename="test.patch" Content-Type: text/x-patch; name="test.patch"; charset="ISO-8859-9" Content-Transfer-Encoding: 8bit diff -up libical-1.0/src/libical/icaltime.c.old libical-1.0/src/libical/icaltime.c --- libical-1.0/src/libical/icaltime.c.old 2014-11-26 13:29:50.809112801 +0100 +++ libical-1.0/src/libical/icaltime.c 2014-11-26 14:14:42.309998754 +0100 @@ -77,7 +77,7 @@ * local daylight savings time applied to the result. * This function expects well-formed input. */ -static time_t make_time(struct tm *tm, int tzm) +static time_t make_time(struct tm *tm, int tzm, int be_strict) { time_t tim; @@ -85,7 +85,7 @@ static time_t make_time(struct tm *tm, i /* check that year specification within range */ - if (tm->tm_year < 70 || tm->tm_year > 138) + if (be_strict && sizeof (time_t) == 4 && (tm->tm_year < 70 || tm->tm_year > 138)) return((time_t) -1); /* check that month specification within range */ @@ -96,7 +96,7 @@ static time_t make_time(struct tm *tm, i /* check for upper bound of Jan 17, 2038 (to avoid possibility of 32-bit arithmetic overflow) */ - if (tm->tm_year == 138) { + if (be_strict && sizeof (time_t) == 4 && tm->tm_year == 138) { if (tm->tm_mon > 0) return((time_t) -1); else if (tm->tm_mday > 17) @@ -289,7 +289,7 @@ time_t icaltime_as_timet(const struct ic stm.tm_year = tt.year-1900; stm.tm_isdst = -1; - t = make_time(&stm, 0); + t = make_time(&stm, 0, 1); return t; @@ -395,7 +395,7 @@ time_t icaltime_as_timet_with_zone(const { icaltimezone *utc_zone; struct tm stm; - time_t t; + time_t t, maketimeval; char *old_tz; struct icaltimetype local_tt; @@ -427,6 +427,7 @@ time_t icaltime_as_timet_with_zone(const /* The functions putenv and mktime are not thread safe, inserting a lock to prevent any crashes */ + maketimeval = make_time (&stm, 0, 0); #ifdef HAVE_PTHREAD pthread_mutex_lock (&tzid_mutex); #endif @@ -442,6 +443,13 @@ to prevent any crashes */ #ifdef HAVE_PTHREAD pthread_mutex_unlock (&tzid_mutex); #endif + + if (t != maketimeval) { + fprintf (stderr, "Time %d doesn't match expected %d for %d-%d-%dT%02d%02d%02d\n", (int) maketimeval, (int) t, stm.tm_year + 1900, stm.tm_mon + 1, stm.tm_mday, stm.tm_hour, stm.tm_min, stm.tm_sec); + fflush (stderr); + } + assert (t == maketimeval); + return t; } --=-R5vXJ4rIjFlMFra7xOz5 Content-Disposition: attachment; filename="ical.c" Content-Type: text/x-csrc; name="ical.c"; charset="ISO-8859-9" Content-Transfer-Encoding: 8bit /* gcc `pkg-config --cflags --libs libical` ical.c -g -O0 -o ical && ./ical */ #include #include #include #include int main (int argc, char *argv[]) { icalarray *builtin_timezones; icaltimetype tt; int dd, hh, zz, zz2 = -1, tried = 0; tt = icaltime_current_time_with_zone (icaltimezone_get_builtin_timezone ("America/New_York")); tt.year = 2038; icaltime_as_timet_with_zone (tt, icaltimezone_get_builtin_timezone ("PST")); tried++; tt.year = 2050; icaltime_as_timet_with_zone (tt, icaltimezone_get_builtin_timezone ("PST")); tried++; tt.year = 1958; icaltime_as_timet_with_zone (tt, icaltimezone_get_builtin_timezone ("PST")); tried++; builtin_timezones = icaltimezone_get_builtin_timezones (); printf ("got %d zones\n", builtin_timezones->num_elements); for (zz = -1; zz < (int) builtin_timezones->num_elements; zz++) { icaltimezone *zone; if (zz < 0) zone = icaltimezone_get_utc_timezone (); else zone = icalarray_element_at (builtin_timezones, zz); tt = icaltime_current_time_with_zone (zone); for (dd = 0; dd < 370; dd += 17) { for (hh = 0; hh < 60 * 60 * 24; hh += 567) { int zz2cnt; icaltime_adjust (&tt, 0, 0, 0, 1); for (zz2cnt = 0; zz2cnt < 15; zz2cnt++) { icaltimezone *zone2; if (zz2 < 0) zone2 = icaltimezone_get_utc_timezone (); else zone2 = icalarray_element_at (builtin_timezones, zz2); icaltime_as_timet_with_zone (tt, zone2); tried++; zz2++; if (zz2 >= builtin_timezones->num_elements) zz2 = -1; } } } printf ("\r%d %% done", (zz >= 0 ? zz : 0) * 100 / builtin_timezones->num_elements); fflush (stdout); } printf ("\ntried %d times\n", tried); return 0; } --=-R5vXJ4rIjFlMFra7xOz5 Content-Disposition: attachment; filename="proposed.patch" Content-Type: text/x-patch; name="proposed.patch"; charset="ISO-8859-9" Content-Transfer-Encoding: 8bit diff -up libical-1.0/src/libical/icaltime.c.old libical-1.0/src/libical/icaltime.c --- libical-1.0/src/libical/icaltime.c.old 2014-11-26 13:29:50.809112801 +0100 +++ libical-1.0/src/libical/icaltime.c 2014-11-26 14:56:05.740893525 +0100 @@ -64,11 +64,6 @@ #define gmtime_r(tp,tmp) (gmtime(tp)?(*(tmp)=*gmtime(tp),(tmp)):0) #endif -#ifdef HAVE_PTHREAD - #include - static pthread_mutex_t tzid_mutex = PTHREAD_MUTEX_INITIALIZER; -#endif - /* * Function to convert a struct tm time specification * to an ANSI time_t using the specified time zone. @@ -77,7 +72,7 @@ * local daylight savings time applied to the result. * This function expects well-formed input. */ -static time_t make_time(struct tm *tm, int tzm) +static time_t make_time(struct tm *tm, int tzm, int be_strict) { time_t tim; @@ -85,7 +80,7 @@ static time_t make_time(struct tm *tm, i /* check that year specification within range */ - if (tm->tm_year < 70 || tm->tm_year > 138) + if (be_strict && sizeof (time_t) == 4 && (tm->tm_year < 70 || tm->tm_year > 138)) return((time_t) -1); /* check that month specification within range */ @@ -96,7 +91,7 @@ static time_t make_time(struct tm *tm, i /* check for upper bound of Jan 17, 2038 (to avoid possibility of 32-bit arithmetic overflow) */ - if (tm->tm_year == 138) { + if (be_strict && sizeof (time_t) == 4 && tm->tm_year == 138) { if (tm->tm_mon > 0) return((time_t) -1); else if (tm->tm_mday > 17) @@ -289,99 +284,12 @@ time_t icaltime_as_timet(const struct ic stm.tm_year = tt.year-1900; stm.tm_isdst = -1; - t = make_time(&stm, 0); + t = make_time(&stm, 0, 1); return t; } - -/* Structure used by set_tz to hold an old value of TZ, and the new - value, which is in memory we will have to free in unset_tz */ -/* This will hold the last "TZ=XXX" string we used with putenv(). After we - call putenv() again to set a new TZ string, we can free the previous one. - As far as I know, no libc implementations actually free the memory used in - the environment variables (how could they know if it is a static string or - a malloc'ed string?), so we have to free it ourselves. */ -static char* saved_tz = NULL; - -/* If you use set_tz(), you must call unset_tz() some time later to restore the - original TZ. Pass unset_tz() the string that set_tz() returns. Call both the functions - locking the tzid mutex as in icaltime_as_timet_with_zone */ -char* set_tz(const char* tzid) -{ - char *old_tz, *old_tz_copy = NULL, *new_tz; - - /* Get the old TZ setting and save a copy of it to return. */ - old_tz = getenv("TZ"); - if(old_tz){ - old_tz_copy = (char*)malloc(strlen (old_tz) + 4); - - if(old_tz_copy == 0){ - icalerror_set_errno(ICAL_NEWFAILED_ERROR); - return 0; - } - - strcpy (old_tz_copy, "TZ="); - strcpy (old_tz_copy + 3, old_tz); - } - - /* Create the new TZ string. */ - new_tz = (char*)malloc(strlen (tzid) + 4); - - if(new_tz == 0){ - icalerror_set_errno(ICAL_NEWFAILED_ERROR); - free(old_tz_copy); - return 0; - } - - strcpy (new_tz, "TZ="); - strcpy (new_tz + 3, tzid); - - /* Add the new TZ to the environment. */ - putenv(new_tz); - - /* Free any previous TZ environment string we have used in a synchronized manner. */ - - free (saved_tz); - - /* Save a pointer to the TZ string we just set, so we can free it later. */ - saved_tz = new_tz; - - return old_tz_copy; /* This will be zero if the TZ env var was not set */ -} - -void unset_tz(char *tzstr) -{ - /* restore the original environment */ - - if(tzstr!=0){ - putenv(tzstr); - } else { - /* Delete from environment. We prefer unsetenv(3) over putenv(3) - because the former is POSIX and behaves consistently. The later - does not unset the variable in some systems (like NetBSD), leaving - it with an empty value. This causes problems later because further - calls to time related functions in libc will treat times in UTC. */ -#ifdef HAVE_UNSETENV - unsetenv("TZ"); -#else -#ifdef _MSC_VER - putenv("TZ="); // The equals is required to remove with MS Visual C++ -#else - putenv("TZ"); -#endif -#endif - } - - /* Free any previous TZ environment string we have used in a synchronized manner */ - free (saved_tz); - - /* Save a pointer to the TZ string we just set, so we can free it later. - (This can possibly be NULL if there was no TZ to restore.) */ - saved_tz = tzstr; -} - /** Return the time as seconds past the UNIX epoch, using the * given timezone. * @@ -395,8 +303,6 @@ time_t icaltime_as_timet_with_zone(const { icaltimezone *utc_zone; struct tm stm; - time_t t; - char *old_tz; struct icaltimetype local_tt; utc_zone = icaltimezone_get_utc_timezone (); @@ -424,25 +330,8 @@ time_t icaltime_as_timet_with_zone(const stm.tm_mon = local_tt.month-1; stm.tm_year = local_tt.year-1900; stm.tm_isdst = -1; -/* The functions putenv and mktime are not thread safe, inserting a lock -to prevent any crashes */ -#ifdef HAVE_PTHREAD - pthread_mutex_lock (&tzid_mutex); -#endif - - /* Set TZ to UTC and use mktime to convert to a time_t. */ - old_tz = set_tz ("UTC"); - tzset (); - - t = mktime (&stm); - unset_tz (old_tz); - tzset (); - -#ifdef HAVE_PTHREAD - pthread_mutex_unlock (&tzid_mutex); -#endif - return t; + return make_time (&stm, 0, 0); } const char* icaltime_as_ical_string(const struct icaltimetype tt) --=-R5vXJ4rIjFlMFra7xOz5--