CFTimeZone: lock the timezone-cache lookup (data race)

[DTW-Thalion]

Summary

CFTimeZoneCreate looked up the global timezone cache without holding the
cache lock:

old = (CFTimeZoneRef)CFDictionaryGetValue (_kCFTimeZoneCache, name);  /* unlocked */

while the insert path adds entries — which can rehash the dictionary — under
_kCFTimeZoneCacheLock. Concurrent CFTimeZoneCreate / CFTimeZoneCreateWithName
calls therefore race on the dictionary's internal bucket array, and the lazy
cache creation was double-checked with an unsynchronised pointer read.

Reproduction (ThreadSanitizer)

Built libgnustep-corebase with -fsanitize=thread; 8 threads call
CFTimeZoneCreateWithName with distinct zone names (abbreviation dictionary
pre-warmed to isolate this race). TSan reports:

WARNING: ThreadSanitizer: data race
  Read  ... GSHashTableFindBucket / GSHashTableGetValue
        <- CFDictionaryGetValue <- CFTimeZoneCreate (CFTimeZone.c:155)
  Write ... GSHashTableCreateMutable (mutex held)
        <- CFDictionaryCreateMutable <- CFTimeZoneCreate (CFTimeZone.c:149)

Fix

Create the cache and look up the entry under _kCFTimeZoneCacheLock. The
insert path already re-checks under the lock, so the slow path is unchanged.

After the change the CFTimeZoneCreate cache race no longer appears under the
same TSan harness.

[HendrikHuebner]

LGTM

[stefanbidi]

In this case, this optimization actually makes a little sense. The variable _kCFTimeZoneCache will only every be NULL once, when the function is first called. Locking the mutex every single time we call this function for something that only happens once seems unnecessary to me.

There is also no race condition here. Checking the variable against NULL happens twice, so there's never a chance that we will write to _kCFTimeZoneCache with different values.

If other folks disagree, I'm OK with merging this PR.

[DTW-Thalion]

To clarify the race: it isn't the one-time creation of _kCFTimeZoneCache, it's the lookup racing a concurrent insert. CFTimeZoneCreate reads the shared cache with CFDictionaryGetValue and, on a miss, inserts the new zone with CFDictionarySetValue — so two threads resolving different zone names can read and write (and rehash/reallocate) the same dictionary concurrently. The double NULL-check only orders the creation, not a GetValue running alongside a SetValue; holding the lock across the lookup is what closes that. No objection to merging as-is.

[stefanbidi]

We don't call CFDictionarySetValue outside of the locked mutex, though. Line 155 check if the value already exists and, if it does, returns that value. We then go through the process of creating the new CFTimeZone object outside of the mutex, but in line 241 we lock the cache mutex and check everything all over again. We never insert any value into the cache without first locking its mutex. Checking for an existing value should not cause a race condition.

[HendrikHuebner]

I'd rather accept this patch. Concurrent inserts or not, by the C standard this is a data race. On some architectures the store to _kCFTimeZoneCache may not be atomic, so another thread might observe a non-NULL value before locking and dereference a garbage pointer.

EDIT: Also, correction for the other PR that had the double-checked lock: that one was also a data race for the same reason.