From 88f8950a495cea1544acd9ace54c5cb0ac5141c4 Mon Sep 17 00:00:00 2001
From: Milan Crha <mcrha@redhat.com>
Date: Tue, 10 Oct 2017 08:40:53 +0200
Subject: [PATCH] Add locking around icaltimezone::changes array

This prevents crash caused by two threads accessing the same icaltimezone
and both accessing the icaltimezone::changes, one reading, another replacing
existing array with new coverage range, like shown below. The lock is global,
thus it can have performance impact in multithreaded applications, though
it should not be significant.

 Thread 12 (Thread 0x7f3d8bfff700 (LWP 27954)):
 #3  0x00007f3dee4f13b0 in <signal handler called> () at /lib64/libpthread.so.0
 #4  0x00007f3dedf83de4 in icalarray_element_at (array=0x7f3d84074a60, position=58) at icalarray.c:137
 #5  0x00007f3dedf9ab3f in icaltimezone_find_nearby_change (zone=0x7f3d84033c20, change=0x7f3d8bffd660) at icaltimezone.c:1020
 #6  0x00007f3dedf9a5ab in icaltimezone_get_utc_offset (zone=0x7f3d84033c20, tt=0x7f3d8bffd720, is_daylight=0x0) at icaltimezone.c:800
 #7  0x00007f3dedf9a3fd in icaltimezone_convert_time (tt=0x7f3d8bffd720, from_zone=0x7f3d84033c20, to_zone=0x7f3dee1ca760 <utc_timezone>) at icaltimezone.c:747
 #8  0x00007f3dedf96de9 in icaltime_convert_to_zone (tt=..., zone=0x7f3dee1ca760 <utc_timezone>) at icaltime.c:989
 #9  0x00007f3dedf96658 in icaltime_compare (a_in=..., b_in=...) at icaltime.c:744

 Thread 25 (Thread 0x7f3cabfff700 (LWP 28111)):
 #19 0x00007f3dedf83caa in icalarray_free (array=0x7f3d84074a60) at icalarray.c:112
 #20 0x00007f3dedf99c17 in icaltimezone_expand_changes (zone=0x7f3d84033c20, end_year=2022) at icaltimezone.c:480
 #21 0x00007f3dedf99b67 in icaltimezone_ensure_coverage (zone=0x7f3d84033c20, end_year=2014) at icaltimezone.c:452
 #22 0x00007f3dedf9a51a in icaltimezone_get_utc_offset (zone=0x7f3d84033c20, tt=0x7f3cabffd880, is_daylight=0x0) at icaltimezone.c:784
 #23 0x00007f3dedf9a3fd in icaltimezone_convert_time (tt=0x7f3cabffd880, from_zone=0x7f3d84033c20, to_zone=0x7f3dee1ca760 <utc_timezone>) at icaltimezone.c:747
 #24 0x00007f3dedf95b26 in icaltime_as_timet_with_zone (tt=..., zone=0x7f3d84033c20) at icaltime.c:324
---
 src/libical/icaltimezone.c | 70 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/src/libical/icaltimezone.c b/src/libical/icaltimezone.c
index f5066822..dde3c02b 100644
--- a/src/libical/icaltimezone.c
+++ b/src/libical/icaltimezone.c
@@ -45,6 +45,8 @@ static pthread_mutex_t builtin_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
 #else
 static pthread_mutex_t builtin_mutex = PTHREAD_MUTEX_INITIALIZER;
 #endif
+/* To avoid use-after-free in multithreaded applications when accessing icaltimezone::changes array */
+static pthread_mutex_t changes_mutex = PTHREAD_MUTEX_INITIALIZER;
 #endif
 
 #if defined(_WIN32)
@@ -147,6 +149,20 @@ static char *icaltimezone_load_get_line_fn(char *s, size_t size, void *data);
 static void format_utc_offset(int utc_offset, char *buffer, size_t buffer_size);
 static const char *get_zone_directory(void);
 
+static void icaltimezone_changes_lock(void)
+{
+#if defined(HAVE_PTHREAD)
+    pthread_mutex_lock(&changes_mutex);
+#endif
+}
+
+static void icaltimezone_changes_unlock(void)
+{
+#if defined(HAVE_PTHREAD)
+    pthread_mutex_unlock(&changes_mutex);
+#endif
+}
+
 const char *icaltimezone_tzid_prefix(void)
 {
     return ical_tzid_prefix;
@@ -188,10 +204,15 @@ icaltimezone *icaltimezone_copy(icaltimezone *originalzone)
     if (zone->tznames != NULL) {
         zone->tznames = strdup(zone->tznames);
     }
+
+    icaltimezone_changes_lock();
+
     if (zone->changes != NULL) {
         zone->changes = icalarray_copy(zone->changes);
     }
 
+    icaltimezone_changes_unlock();
+
     /* Let the caller set the component because then they will
        know to be careful not to free this reference twice. */
     zone->component = NULL;
@@ -222,11 +243,15 @@ static void icaltimezone_reset(icaltimezone *zone)
     if (zone->component)
         icalcomponent_free(zone->component);
 
+    icaltimezone_changes_lock();
+
     if (zone->changes) {
         icalarray_free(zone->changes);
         zone->changes = NULL;
     }
 
+    icaltimezone_changes_unlock();
+
     icaltimezone_init(zone);
 }
 
@@ -241,7 +266,10 @@ static void icaltimezone_init(icaltimezone *zone)
     zone->component = NULL;
     zone->builtin_timezone = NULL;
     zone->end_year = 0;
+
+    icaltimezone_changes_lock();
     zone->changes = NULL;
+    icaltimezone_changes_unlock();
 }
 
 /** Gets the TZID, LOCATION/X-LIC-LOCATION and TZNAME properties of
@@ -448,10 +476,15 @@ static void icaltimezone_ensure_coverage(icaltimezone *zone, int end_year)
     if (changes_end_year > ICALTIMEZONE_MAX_YEAR)
         changes_end_year = ICALTIMEZONE_MAX_YEAR;
 
+    icaltimezone_changes_lock();
+
     if (!zone->changes || zone->end_year < end_year)
         icaltimezone_expand_changes(zone, changes_end_year);
+
+    icaltimezone_changes_unlock();
 }
 
+/* Hold the icaltimezone_changes_lock(); before calling this function */
 static void icaltimezone_expand_changes(icaltimezone *zone, int end_year)
 {
     icalarray *changes;
@@ -783,8 +816,12 @@ int icaltimezone_get_utc_offset(icaltimezone *zone, struct icaltimetype *tt, int
     /* Make sure the changes array is expanded up to the given time. */
     icaltimezone_ensure_coverage(zone, tt->year);
 
-    if (!zone->changes || zone->changes->num_elements == 0)
+    icaltimezone_changes_lock();
+
+    if (!zone->changes || zone->changes->num_elements == 0) {
+        icaltimezone_changes_unlock();
         return 0;
+    }
 
     /* Copy the time parts of the icaltimetype to an icaltimezonechange so we
        can use our comparison function on it. */
@@ -847,6 +884,9 @@ int icaltimezone_get_utc_offset(icaltimezone *zone, struct icaltimetype *tt, int
             if (is_daylight) {
                 *is_daylight = ! tmp_change.is_daylight;
             }
+
+            icaltimezone_changes_unlock();
+
             return tmp_change.prev_utc_offset;
         }
 
@@ -901,7 +941,11 @@ int icaltimezone_get_utc_offset(icaltimezone *zone, struct icaltimetype *tt, int
     if (is_daylight) {
         *is_daylight = zone_change->is_daylight;
     }
-    return zone_change->utc_offset;
+    utc_offset_change = zone_change->utc_offset;
+
+    icaltimezone_changes_unlock();
+
+    return utc_offset_change;
 }
 
 /** Calculates the UTC offset of a given UTC time in the given
@@ -914,7 +958,7 @@ int icaltimezone_get_utc_offset_of_utc_time(icaltimezone *zone,
     icaltimezonechange *zone_change, tt_change, tmp_change;
     size_t change_num, change_num_to_use;
     int found_change = 1;
-    int step;
+    int step, utc_offset;
 
     if (is_daylight)
         *is_daylight = 0;
@@ -930,8 +974,12 @@ int icaltimezone_get_utc_offset_of_utc_time(icaltimezone *zone,
     /* Make sure the changes array is expanded up to the given time. */
     icaltimezone_ensure_coverage(zone, tt->year);
 
-    if (!zone->changes || zone->changes->num_elements == 0)
+    icaltimezone_changes_lock();
+
+    if (!zone->changes || zone->changes->num_elements == 0) {
+        icaltimezone_changes_unlock();
         return 0;
+    }
 
     /* Copy the time parts of the icaltimetype to an icaltimezonechange so we
        can use our comparison function on it. */
@@ -980,6 +1028,9 @@ int icaltimezone_get_utc_offset_of_utc_time(icaltimezone *zone,
             if (is_daylight) {
                 *is_daylight = ! tmp_change.is_daylight;
             }
+
+            icaltimezone_changes_unlock();
+
             return tmp_change.prev_utc_offset;
         }
 
@@ -999,12 +1050,16 @@ int icaltimezone_get_utc_offset_of_utc_time(icaltimezone *zone,
     zone_change = icalarray_element_at(zone->changes, change_num_to_use);
     if (is_daylight)
         *is_daylight = zone_change->is_daylight;
+    utc_offset = zone_change->utc_offset;
 
-    return zone_change->utc_offset;
+    icaltimezone_changes_unlock();
+
+    return utc_offset;
 }
 
 /** Returns the index of a timezone change which is close to the time
    given in change. */
+/* Hold icaltimezone_changes_lock(); before calling this function */
 static size_t icaltimezone_find_nearby_change(icaltimezone *zone, icaltimezonechange * change)
 {
     icaltimezonechange *zone_change;
@@ -1847,6 +1902,8 @@ int icaltimezone_dump_changes(icaltimezone *zone, int max_year, FILE *fp)
     printf("Num changes: %i\n", zone->changes->num_elements);
 #endif
 
+    icaltimezone_changes_lock();
+
     for (change_num = 0; change_num < zone->changes->num_elements; change_num++) {
         zone_change = icalarray_element_at(zone->changes, change_num);
 
@@ -1864,6 +1921,9 @@ int icaltimezone_dump_changes(icaltimezone *zone, int max_year, FILE *fp)
 
         fprintf(fp, "\n");
     }
+
+    icaltimezone_changes_unlock();
+
     return 1;
 }
 
-- 
2.13.5