We're updating the issue view to help you get more done. 

Need to make TimeZone::AdoptDefault thread safe

Description

This is an issue we discovered from a mystical crash in chrome. https://bugs.chromium.org/p/chromium/issues/detail?id=950851

Currently, the TimeZone::AdoptDefault said the following
"This function is not thread safe. It is an error for multiple threads to concurrently attempt to set the default time zone, or for any thread to attempt to reference the default zone while another thread is setting it."

In chrome, several code call icu::TimeZone::CreateDefault . Some in the application, the other in internally library, such as v8, and other are called inside ICU itself. These are calls in different thread.
Chrome also listen to platform API for timezone change, for example, when the user take a flight to a different city, or change the timezone on Mac desktop via System Preference. While the system notify chrome (or other app) the change of the timezone, Chrome, in https://cs.chromium.org/chromium/src/services/device/time_zone_monitor/time_zone_monitor.cc

it call the icu::TimeZone::detectHostTimeZone() to find out the new timezone and call icu::TimeZone::adoptDefault() to inform the ICU the default is now change to a newly detected timezone.

But while that happen, there are no way this timezone monitor will be able to know all the other code running on different thread to add mutex to sync with them, because these calls are in various different places and running on different thread.

Therefore, it cause crash because it is not practical for the AdoptDefault caller to sync with the createDefault caller.. It make much more sense for the AdoptDefault implementation to sync with the createDefault implementation.

We need to change the AdoptDefault call to be thread safe. Currently the problem is

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 TimeZone* U_EXPORT2 TimeZone::createDefault() { umtx_initOnce(gDefaultZoneInitOnce, initDefault); return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->clone() : NULL; } void U_EXPORT2 TimeZone::adoptDefault(TimeZone* zone) { if (zone != NULL) { TimeZone *old = DEFAULT_ZONE; DEFAULT_ZONE = zone; delete old; ucln_i18n_registerCleanup(UCLN_I18N_TIMEZONE, timeZone_cleanup); } }

Notice if thread A is calling adoptDefault while thread B is calling createDefault, consider the following

 

Thread A

Thread B

adoptDefault()

createDefault()

TimeZone *old = DEFAULT_ZONE;

umtx_initOnce(gDefaultZoneInitOnce, initDefault);

DEFAULT_ZONE = zone;
delete old;

return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->

 

clone() : NULL;

If the “DEFAULT_ZONE = zone; delete old;” happen between the time of DEFAULT_ZONE-> pointer start to call clone() and before it finish the clone. The cloned copy could be a partially trashed old DEFAULT_ZONE

The return value of B will be a clone of a trashed old DEFAULT_ZONE since the clone could happen while the old got deleted or after it got deleted.

We need to add mutex to ensure the "delete old" and "DEFAULT_ZONE->clone()" won't happen concurrently.

Here is my propose change:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 diff --git a/source/i18n/timezone.cpp b/source/i18n/timezone.cpp index f129d8b6..f7b48273 100644 --- a/source/i18n/timezone.cpp +++ b/source/i18n/timezone.cpp @@ -527,6 +527,11 @@ TimeZone::detectHostTimeZone() // ------------------------------------- +static UMutex *gMutex() { + static UMutex* m = new UMutex(); + return m; +} + /** * Initialize DEFAULT_ZONE from the system default time zone. * Upon return, DEFAULT_ZONE will not be NULL, unless operator new() @@ -536,6 +541,7 @@ static void U_CALLCONV initDefault() { ucln_i18n_registerCleanup(UCLN_I18N_TIMEZONE, timeZone_cleanup); + Mutex lock(gMutex()); // If setDefault() has already been called we can skip getting the // default zone information from the system. if (DEFAULT_ZONE != NULL) { @@ -571,7 +577,11 @@ TimeZone* U_EXPORT2 TimeZone::createDefault() { umtx_initOnce(gDefaultZoneInitOnce, initDefault); - return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->clone() : NULL; + if (DEFAULT_ZONE == NULL) return NULL; + { + Mutex lock(gMutex()); + return DEFAULT_ZONE->clone(); + } } // ------------------------------------- @@ -582,8 +592,11 @@ TimeZone::adoptDefault(TimeZone* zone) if (zone != NULL) { TimeZone *old = DEFAULT_ZONE; - DEFAULT_ZONE = zone; - delete old; + { + Mutex lock(gMutex()); + DEFAULT_ZONE = zone; + delete old; + } ucln_i18n_registerCleanup(UCLN_I18N_TIMEZONE, timeZone_cleanup); } }

 

Status

Assignee

Andy Heninger

Reporter

Frank Yung-Fong Tang

Labels

Reviewer

None

Time Needed

Minutes

Start date

None

Components

Fix versions

Priority

critical