[PATCH] Improve thread safety of icaltimezone_load_builtin_timezone()

[Milan Crha]

On Sun, 2019-01-13 at 11:37 -0500, Allen Winter wrote:
> The clang thread sanitizer is not happy with this change.
> see attachment

	Hi,
can we reiterate on this patch, please? I face occasional crashes in
libical's icalcomponent_new_clone(), which I didn't know what they are
about, then I added a debug print [1] to the
icaltimezone_load_builtin_timezone() and it is hit once or more times
here, which led me to this old patch proposal. I see the message
printed several times when running the patched bultin_timezones test.

I'll comment on the tasn log entries:

> WARNING: ThreadSanitizer: data race (pid=40885)
>   Write of size 8 at 0x7bc400016d28 by thread T1 (mutexes: write M9):
>     #0 icaltimezone_get_vtimezone_properties .../src/libical/icaltimezone.c:320 (libical.so.3+0xb6fdf)
>     #1 icaltimezone_load_builtin_timezone .../src/libical/icaltimezone.c:1852 (libical.so.3+0xb7454)
>     #2 icaltimezone_get_component .../src/libical/icaltimezone.c:1224 (libical.so.3+0xb9df2)
>     #3 thread_func .../src/test/builtin_timezones.c:48 (builtin_timezones+0x401a71)
>     #4 <null> <null> (libtsan.so.0+0x2d33f)
> 
>   Previous read of size 8 at 0x7bc400016d28 by thread T20:
>     #0 icaltimezone_load_builtin_timezone .../src/libical/icaltimezone.c:1785 (libical.so.3+0xb7235)
>     #1 icaltimezone_get_component .../src/libical/icaltimezone.c:1224 (libical.so.3+0xb9df2)
>     #2 thread_func .../src/test/builtin_timezones.c:48 (builtin_timezones+0x401a71)
>     #3 <null> <null> (libtsan.so.0+0x2d33f)

The above is due to:

    /* Prevent blocking on mutex lock caused by recursive calls */
    if (zone->component)
        return;

near the top of the icaltimezone_load_builtin_timezone(), which is just
before the icaltimezone_builtin_lock(). That means it's an intentional
failure in the thread sanitizing in the libical's code.

Then there was a set of failures around 'pvl_elem_count' and
'pvl_list_count' global variables located in src/libical/pvl.c. Looking
on their usage they are used only to set structures' MAGIC property,
which isn't read in the pvl.c. That's a correct claim from the thread
sanitizer and it goes away when I stop using those variables.

Finally, there was a claim about 'thread_comp' global variable in the
src/test/builtin_timezones.c. That had been added by the proposed
patch. It's also a valid claim from the ThreadSanitizer, multiple
threads read/write the variable at the same time. I addressed it in the
new patch (attached) by added mutex for the variable usage.

There's nothing else in the ThreadSanitizer log.

To summarize, most of the claims are expected and the one, which could
be fixed, is fixed by the new patch. I hope this will make this easier
to reconsider the patch.

The attached ical.patch contains an attempt to mute the rest
ThreadSanitizer claims, but I've not been able to test it, because
gcc 10.2.1 doesn't have defined '__has_feature' and trying to run the
code compiled with clang 10.0.0 doesn't run due to a segmentation fault
under __tsan::MetaMap::GetBlock(unsigned long) () in the compiled code.

By the way, the patch is included in the Fedora package since the
initial proposal and I cannot reproduce the crash with it.

	Thanks and bye,
	Milan

[1] the diff of the debug print:

diff --git a/src/libical/icaltimezone.c b/src/libical/icaltimezone.c
index 9a1c641c..8e6c35ed 100644
--- a/src/libical/icaltimezone.c
+++ b/src/libical/icaltimezone.c
@@ -1787,6 +1787,10 @@ static void icaltimezone_load_builtin_timezone(icaltimezone *zone)
 
     icaltimezone_builtin_lock();
 
+    if (zone->component) {
+       printf ("%s: zone %p has already set component %p, even it did not have it set before acquaring the lock\n", __FUNCTION__, zone, zone->component);
+    }
+
     /* If the location isn't set, it isn't a builtin timezone. */
     if (!zone->location || !zone->location[0]) {
         icaltimezone_builtin_unlock();

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Improve-thread-safety-of-icaltimezone_load_builtin_t.patch
Type: text/x-patch
Size: 3217 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/libical-devel/attachments/20201021/ca64eacd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ical.patch
Type: text/x-patch
Size: 1987 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/libical-devel/attachments/20201021/ca64eacd/attachment-0001.bin>