Incorrect validation of DST in java.util.SimpleTimeZone

[David Holmes]

AFAICS SimpleTimeZone is simply not thread-safe. It has state that can 
be modified concurrently without synchronization and with fields not 
even declared volatile. Only the "cache" makes an attempt to use 
synchronization. So clone() is never guaranteed to actually produce a 
copy with valid/consistent field values.

The suggested patch certainly improves the situation by at least 
resetting the cache of the cloned instance before returning it.

David

On 11/11/2017 3:53 PM, Venkateswara R Chintala wrote:
> Thanks Sean. I am pasting the patch here:
> 
> --- old/src/java.base/share/classes/java/util/SimpleTimeZone.java 
> 2017-11-11 11:17:38.643867420 +0530
> +++ new/src/java.base/share/classes/java/util/SimpleTimeZone.java 
> 2017-11-11 11:17:38.375870421 +0530
> @@ -868,7 +868,11 @@
>        */
>       public Object clone()
>       {
> -        return super.clone();
> +        // Invalidate the time zone cache while cloning as it
> +        // can be inconsistent due to race condition.
> +        SimpleTimeZone tz = (SimpleTimeZone) super.clone();
> +        tz.invalidateCache();
> +        return tz;
>       }
> 
>       /**
> --- /dev/null    2017-11-02 17:09:59.155627814 +0530
> +++ new/test/java/util/TimeZone/SimpleTimeZoneTest.java 2017-11-11 
> 11:17:38.867864912 +0530
> @@ -0,0 +1,55 @@
> +/*
> + * @test
> + * @summary Tests the race condition between 
> java.util.TimeZone.getDefault() and java.util.GregorianCalendar()
> + * @run main SimpleTimeZoneTest
> +*/
> +
> +import java.util.Calendar;
> +import java.util.GregorianCalendar;
> +import java.util.SimpleTimeZone;
> +import java.util.TimeZone;
> +
> +public class SimpleTimeZoneTest extends Thread {
> +    Calendar cal;
> +
> +    public SimpleTimeZoneTest (Calendar cal) {
> +        this.cal = cal;
> +    }
> +
> +    public static void main (String[] args) {
> +        TimeZone stz = new SimpleTimeZone(7200000, "Asia/Jerusalem", 
> Calendar.MARCH, 27, 0, 3600000, Calendar.SEPTEMBER, 16, 0, 3600000);
> +        TimeZone.setDefault(stz);
> +
> +        SimpleTimeZoneTest stt = new SimpleTimeZoneTest(new 
> GregorianCalendar());
> +        stt.setDaemon(true);
> +        stt.start();
> +
> +        for (int i = 0; i < 50000; i++) {
> +            Calendar cal = new GregorianCalendar();
> +            cal.clear();
> +            cal.getTimeInMillis();
> +            cal.set(2014, 2, 2);
> +            cal.clear();
> +            cal.getTimeInMillis();
> +            cal.set(1970, 2, 2);
> +        }
> +
> +    }
> +
> +    public void run() {
> +        while (true) {
> +            cal.setTimeZone(TimeZone.getDefault());
> +            cal.clear();
> +            cal.set(2008, 9, 9);
> +            Calendar calInst = java.util.Calendar.getInstance();
> +            calInst.setTimeInMillis(cal.getTimeInMillis());
> +
> +            if (calInst.get(java.util.Calendar.HOUR_OF_DAY) != 
> cal.get(java.util.Calendar.HOUR_OF_DAY) ||
> +                calInst.get(java.util.Calendar.MINUTE) != 
> cal.get(java.util.Calendar.MINUTE) ||
> +                calInst.get(java.util.Calendar.SECOND) != 
> cal.get(java.util.Calendar.SECOND) ||
> +                calInst.get(java.util.Calendar.MILLISECOND) != 
> cal.get(java.util.Calendar.MILLISECOND)) {
> +                    throw new RuntimeException("Test failed");
> +            }
> +        }
> +    }
> +}
> 
> 
> On 10/11/17 9:29 PM, Seán Coffey wrote:
>> I think the OpenJDK mailing lists accept attachments if in patch 
>> format. If it's a simple short patch, it's acceptable to paste it into 
>> email body.
>>
>> Easiest solution is to use webrev[1]. If you can't upload this to 
>> cr.openjdk.java.net - then one of your colleagues may be able to help.
>>
>> [1] http://openjdk.java.net/guide/webrevHelp.html
>>
>> Regards,
>> Sean.
>>
>> On 10/11/17 12:18, Venkateswara R Chintala wrote:
>>> Looks like the patch attached earlier is not visible. As this is my 
>>> first contribution, please let me know how I can send the patch for 
>>> review.
>>>
>>>
>>> On 10/11/17 5:37 PM, Venkateswara R Chintala wrote:
>>>> Hi,
>>>>
>>>> In a multi-threaded environment, when java.util.SimpleTimeZone 
>>>> object is used to create a default timezone, there can be a race 
>>>> condition between the methods java.util.Timezone.getDefault() and 
>>>> java.util.Timezone.getDefaultRef() which can result in inconsistency 
>>>> of cache that is used to validate a particular time/date in DST.
>>>>
>>>> When a thread is cloning a default timezone object (SimpleTimeZone) 
>>>> and at the same time if a different thread modifies the time/year 
>>>> values, then the cache values (cacheYear, cacheStart, cacheEnd) can 
>>>> become inconsistent which leads to incorrect DST determination.
>>>>
>>>> We considered two approaches to fix the issue.
>>>>
>>>> 1)Synchronize access to cloning default timezone object when cache 
>>>> is being modified.
>>>>
>>>> 2)Invalidate the cache while returning the clone.
>>>>
>>>> We preferred the second option as synchronization is more expensive.
>>>>
>>>> We have attached the patch and jtreg testcase. Please review.
>>>>
>>>
>>
>>
>