Incorrect validation of DST in java.util.SimpleTimeZone

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Incorrect validation of DST in java.util.SimpleTimeZone

Venkateswara R Chintala
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.

--
Regards
-Venkat

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Venkateswara R Chintala
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.
>

--
Regards
-Venkat

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Seán Coffey
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.
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Venkateswara R Chintala
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.
>>>
>>
>
>

--
Regards
-Venkat

Reply | Threaded
Open this post in threaded view
|

Re: 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.
>>>>
>>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
In reply to this post by Venkateswara R Chintala
Hi Venkateswara R Chintala,

I would like to remind that TimeZone.clone() is also in the code path of
java.time.ZoneId.systemDefault() where it was relied on to be optimized
by JIT to never actually allocate the cloned TimeZone object by
employing escape analysis. It would be nice to verify if this patch
keeps that behavior. To test this, consider a JMH benchmark that simply
invokes ZoneId.systemDefault() and returns it from the test method. 1st
verify that unmodified code, when JITed, performs no allocations. Then
test with modified code by applying the patch and see if there's any
difference. Hint: use "-prof gc" command line options to run the
benchmarks.jar.

Regards, Peter


On 11/11/17 06:53, 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.
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
In reply to this post by Venkateswara R Chintala
Hi Venkat,

On 11/10/17 13:07, 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.

Unfortunately, the SimpleTimeZone.invalidateCache() is also
synchronized. So of the same cost, and it may inhibit JIT optimization.

Perhaps it would be best to synchronize cloning and then arrange the
ZoneId.systemDefault() codepath to not involve cloning. I refrained from
doing that in a patch for:

https://bugs.openjdk.java.net/browse/JDK-8074002

simply because it was easier and benchmarks showed that cloning is
optimized away. But now we should reconsider that and use
TimeZone.getDefaultRef() from the ZoneId.systemDefault() (introducing
JavaUtilAccess into SharedSecrets mechanism)...


Regards, Peter

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

David Holmes
In reply to this post by Peter Levart
Hi Peter,

On 11/11/2017 8:06 PM, Peter Levart wrote:
> Hi Venkateswara R Chintala,
>
> I would like to remind that TimeZone.clone() is also in the code path of
> java.time.ZoneId.systemDefault() where it was relied on to be optimized
> by JIT to never actually allocate the cloned TimeZone object by
> employing escape analysis.

remind? Where is this documented?

And given:

     public static TimeZone getDefault() {
         return (TimeZone) getDefaultRef().clone();
     }

how can it not allocate??

David
-----

> It would be nice to verify if this patch
> keeps that behavior. To test this, consider a JMH benchmark that simply
> invokes ZoneId.systemDefault() and returns it from the test method. 1st
> verify that unmodified code, when JITed, performs no allocations. Then
> test with modified code by applying the patch and see if there's any
> difference. Hint: use "-prof gc" command line options to run the
> benchmarks.jar.
>
> Regards, Peter
>
>
> On 11/11/17 06:53, 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.
>>>>>
>>>>
>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
Hi David,

On 11/11/17 13:37, David Holmes wrote:

> Hi Peter,
>
> On 11/11/2017 8:06 PM, Peter Levart wrote:
>> Hi Venkateswara R Chintala,
>>
>> I would like to remind that TimeZone.clone() is also in the code path
>> of java.time.ZoneId.systemDefault() where it was relied on to be
>> optimized by JIT to never actually allocate the cloned TimeZone
>> object by employing escape analysis.
>
> remind? Where is this documented?

Unfortunately it is not documented. My bad. I did test it at the time
and determined that ZoneId.systemDefault(), with my patch applied, did
not do allocations when JIT kicked-in. So we settled for an easier variant:

http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.04/

instead of a more complicated one that uses SharedSecrets to avoid cloning:

http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneId.systemDefault/webrev.01/

>
> And given:
>
>     public static TimeZone getDefault() {
>         return (TimeZone) getDefaultRef().clone();
>     }
>
> how can it not allocate??

When JIT compiles some method and inlines called methods, it also
analyses all allocations performed to see if some of them can be proved
to not escape the invocation of that compiled method. It then eliminates
allocations of such objects on heap and rather generates code that keeps
their state in registers or stack.

For example, take the following method:

String defaultTZID() {
     return TimeZone.getDefault().getID();
}

When JIT compiles it and inlines invocations to other methods within it,
it can prove that cloned TimeZone instance never escapes the call to
defaultTZID() and can therefore skip allocating the instance on heap.

But this is fragile. If code in invoked methods changes, they may not
get inlined or EA may not be able to prove that the cloned instance
can't escape and allocation may be introduced. ZoneId.systemDefault() is
a hot method and it would be nice if we manage to keep it allocation free.

Regards, Peter

>
> David
> -----
>
>> It would be nice to verify if this patch keeps that behavior. To test
>> this, consider a JMH benchmark that simply invokes
>> ZoneId.systemDefault() and returns it from the test method. 1st
>> verify that unmodified code, when JITed, performs no allocations.
>> Then test with modified code by applying the patch and see if there's
>> any difference. Hint: use "-prof gc" command line options to run the
>> benchmarks.jar.
>>
>> Regards, Peter
>>
>>
>> On 11/11/17 06:53, 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.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
Hi David, Venkat,

On 11/11/17 21:15, Peter Levart wrote:

> For example, take the following method:
>
> String defaultTZID() {
>     return TimeZone.getDefault().getID();
> }
>
> When JIT compiles it and inlines invocations to other methods within
> it, it can prove that cloned TimeZone instance never escapes the call
> to defaultTZID() and can therefore skip allocating the instance on heap.
>
> But this is fragile. If code in invoked methods changes, they may not
> get inlined or EA may not be able to prove that the cloned instance
> can't escape and allocation may be introduced. ZoneId.systemDefault()
> is a hot method and it would be nice if we manage to keep it
> allocation free.

Well, I tried the following variant of SimpleTimeZone.clone() patch:

     public Object clone()
     {
         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
         // like tz.invalidateCache() but without holding a lock on clone
         tz.cacheYear = tz.startYear - 1;
         tz.cacheStart = tz.cacheEnd = 0;
         return tz;
     }


...and the JMH benchmark with gc profiling shows that
ZoneId.systemDefault() still manages to get JIT-compiled without
introducing allocation.

Even the following (original Venkat's) patch:

     public Object clone()
     {
         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
         tz.invalidateCache();
         return tz;
     }

...does the same and the locking in invalidateCache() is elided too.
Allocation and lock-elision go hand-in-hand. When object doesn't escape,
allocation on heap may be eliminated and locks on that instance elided.

So this is better than synchronizing on the original instance during
.clone() execution as it has potential to avoid locking overhead.

So Venkat, go ahead. My fear was unjustified.

Regards, Peter

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Venkateswara R Chintala
Thanks David, Peter for your review and comments. As I am new to the
community, can you please help me to open a bug and integrate the
changes into code base?

-Venkat

On 12/11/17 2:19 AM, Peter Levart wrote:

> Hi David, Venkat,
>
> On 11/11/17 21:15, Peter Levart wrote:
>> For example, take the following method:
>>
>> String defaultTZID() {
>>     return TimeZone.getDefault().getID();
>> }
>>
>> When JIT compiles it and inlines invocations to other methods within
>> it, it can prove that cloned TimeZone instance never escapes the call
>> to defaultTZID() and can therefore skip allocating the instance on heap.
>>
>> But this is fragile. If code in invoked methods changes, they may not
>> get inlined or EA may not be able to prove that the cloned instance
>> can't escape and allocation may be introduced. ZoneId.systemDefault()
>> is a hot method and it would be nice if we manage to keep it
>> allocation free.
>
> Well, I tried the following variant of SimpleTimeZone.clone() patch:
>
>     public Object clone()
>     {
>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>         // like tz.invalidateCache() but without holding a lock on clone
>         tz.cacheYear = tz.startYear - 1;
>         tz.cacheStart = tz.cacheEnd = 0;
>         return tz;
>     }
>
>
> ...and the JMH benchmark with gc profiling shows that
> ZoneId.systemDefault() still manages to get JIT-compiled without
> introducing allocation.
>
> Even the following (original Venkat's) patch:
>
>     public Object clone()
>     {
>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>         tz.invalidateCache();
>         return tz;
>     }
>
> ...does the same and the locking in invalidateCache() is elided too.
> Allocation and lock-elision go hand-in-hand. When object doesn't
> escape, allocation on heap may be eliminated and locks on that
> instance elided.
>
> So this is better than synchronizing on the original instance during
> .clone() execution as it has potential to avoid locking overhead.
>
> So Venkat, go ahead. My fear was unjustified.
>
> Regards, Peter
>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
In reply to this post by David Holmes
Hi David,

On 11/11/2017 07:51 AM, David Holmes wrote:
> 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.

The instance of SimpleTimeZone that is shared among threads (internally
in JDK) is the defaultTimeZone instance (obtained through
package-private TimeZone.getDefaultRef() method). I checked the usages
and they seem to be "read-only" - not modifying the instance, just
obtaining information from it. The cache OTOH, as you say, is synchronized.

TimeZone and subclasses seem to be designed to be modified by single
thread only, but can be used from multiple threads to read the
information from them, including lazily computed and cached information.
Usage withing JDK seems to comply with that.

Venkat's patch therefore correctly fixes the remaining issue that is
observed when the shared SimpleTimeZone instance is being cloned while
also being accessed from multiple threads in read-only mode.
Invalidating cache of the cloned instance just before returning it from
clone() method means that instance obtained from TimeZone.getDefault()
will never get cached state from original instance and will always have
to re-compute it, but I think this is still better than synchronizing on
the original instance which may never be optimized away (i.e. elided) by
JIT.

Regards, Peter

>
> 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.
>>>>>
>>>>
>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
In reply to this post by Venkateswara R Chintala
Hi Venkat,

I created the following issue:

https://bugs.openjdk.java.net/browse/JDK-8191216

I can also sponsor this patch and push it for JDK 10.

The patch that you are proposing looks good to me. Does anybody have
anything else to say?

Regards, Peter


On 11/13/2017 11:28 AM, Venkateswara R Chintala wrote:

> Thanks David, Peter for your review and comments. As I am new to the
> community, can you please help me to open a bug and integrate the
> changes into code base?
>
> -Venkat
>
> On 12/11/17 2:19 AM, Peter Levart wrote:
>> Hi David, Venkat,
>>
>> On 11/11/17 21:15, Peter Levart wrote:
>>> For example, take the following method:
>>>
>>> String defaultTZID() {
>>>     return TimeZone.getDefault().getID();
>>> }
>>>
>>> When JIT compiles it and inlines invocations to other methods within
>>> it, it can prove that cloned TimeZone instance never escapes the
>>> call to defaultTZID() and can therefore skip allocating the instance
>>> on heap.
>>>
>>> But this is fragile. If code in invoked methods changes, they may
>>> not get inlined or EA may not be able to prove that the cloned
>>> instance can't escape and allocation may be introduced.
>>> ZoneId.systemDefault() is a hot method and it would be nice if we
>>> manage to keep it allocation free.
>>
>> Well, I tried the following variant of SimpleTimeZone.clone() patch:
>>
>>     public Object clone()
>>     {
>>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>>         // like tz.invalidateCache() but without holding a lock on clone
>>         tz.cacheYear = tz.startYear - 1;
>>         tz.cacheStart = tz.cacheEnd = 0;
>>         return tz;
>>     }
>>
>>
>> ...and the JMH benchmark with gc profiling shows that
>> ZoneId.systemDefault() still manages to get JIT-compiled without
>> introducing allocation.
>>
>> Even the following (original Venkat's) patch:
>>
>>     public Object clone()
>>     {
>>         SimpleTimeZone tz = (SimpleTimeZone) super.clone();
>>         tz.invalidateCache();
>>         return tz;
>>     }
>>
>> ...does the same and the locking in invalidateCache() is elided too.
>> Allocation and lock-elision go hand-in-hand. When object doesn't
>> escape, allocation on heap may be eliminated and locks on that
>> instance elided.
>>
>> So this is better than synchronizing on the original instance during
>> .clone() execution as it has potential to avoid locking overhead.
>>
>> So Venkat, go ahead. My fear was unjustified.
>>
>> Regards, Peter
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

David Holmes
In reply to this post by Peter Levart
Hi Peter,

On 15/11/2017 1:02 AM, Peter Levart wrote:

> Hi David,
>
> On 11/11/2017 07:51 AM, David Holmes wrote:
>> 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.
>
> The instance of SimpleTimeZone that is shared among threads (internally
> in JDK) is the defaultTimeZone instance (obtained through
> package-private TimeZone.getDefaultRef() method). I checked the usages
> and they seem to be "read-only" - not modifying the instance, just
> obtaining information from it. The cache OTOH, as you say, is synchronized.

The initial problem statement was:

"When a thread is cloning a default timezone object (SimpleTimeZone) and
at the same time if a different thread modifies the time/year values, ..."

so that doesn't seem to be read-only. Though perhaps it is a very
specific race.

> TimeZone and subclasses seem to be designed to be modified by single
> thread only, but can be used from multiple threads to read the
> information from them, including lazily computed and cached information.
> Usage withing JDK seems to comply with that.

There's certainly no documentation of any such intent, or design. Seems
more like the synchronization has been added (or not) based on how it is
used within JDK rather than considering the actual API of the public types.

>
> Venkat's patch therefore correctly fixes the remaining issue that is

Okay. As I said it certainly makes things better.

Cheers,
David

> observed when the shared SimpleTimeZone instance is being cloned while
> also being accessed from multiple threads in read-only mode.
> Invalidating cache of the cloned instance just before returning it from
> clone() method means that instance obtained from TimeZone.getDefault()
> will never get cached state from original instance and will always have
> to re-compute it, but I think this is still better than synchronizing on
> the original instance which may never be optimized away (i.e. elided) by
> JIT.
>
> Regards, Peter
>
>>
>> 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.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: Incorrect validation of DST in java.util.SimpleTimeZone

Peter Levart
Hi David,

On 11/14/2017 10:28 PM, David Holmes wrote:

> Hi Peter,
>
> On 15/11/2017 1:02 AM, Peter Levart wrote:
>> Hi David,
>>
>> On 11/11/2017 07:51 AM, David Holmes wrote:
>>> 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.
>>
>> The instance of SimpleTimeZone that is shared among threads
>> (internally in JDK) is the defaultTimeZone instance (obtained through
>> package-private TimeZone.getDefaultRef() method). I checked the
>> usages and they seem to be "read-only" - not modifying the instance,
>> just obtaining information from it. The cache OTOH, as you say, is
>> synchronized.
>
> The initial problem statement was:
>
> "When a thread is cloning a default timezone object (SimpleTimeZone)
> and at the same time if a different thread modifies the time/year
> values, ..."
>
> so that doesn't seem to be read-only. Though perhaps it is a very
> specific race.

User code may do that with its own instances, but that would be invalid
usage. There is no evidence (at least I haven't spotted it yet) that JDK
code does the same too. As far as I have checked, internal JDK code is
aware of the fact that defaultTimeZone instance is a shared instance.
For example, take a protected java.util.Calendar no-arg constructor. It
initializes the Calendar instance with the result of
TimeZone.getDefaultRef() which returns a shared instance. But it also
sets Calendar's 'sharedZone' flag, marking that the TimeZone instance it
references is a shared instance. Methods that would expose such instance
to user code are careful not to do that. For example:

     public TimeZone getTimeZone()
     {
         // If the TimeZone object is shared by other Calendar
instances, then
         // create a clone.
         if (sharedZone) {
             zone = (TimeZone) zone.clone();
             sharedZone = false;
         }
         return zone;
     }

(BTW, this method may expose shared instance to user code if it is
invoked concurrently from multiple threads on the same Calendar instance
- there's no attempt to prevent writes to zone and sharedZone fields to
be observed in non-program order by some concurrent thread)

So I believe the situation is not so critical as it seemed at first.
There may be other concurrency bugs like in above getTimeZone() method
lurking, but the real problem here is cache* fields that may be modified
while using "read-only" part of API. All such accesses are synchronized,
except in the Object.clone() which reads them without holding the lock.

>
>> TimeZone and subclasses seem to be designed to be modified by single
>> thread only, but can be used from multiple threads to read the
>> information from them, including lazily computed and cached
>> information. Usage withing JDK seems to comply with that.
>
> There's certainly no documentation of any such intent, or design.
> Seems more like the synchronization has been added (or not) based on
> how it is used within JDK rather than considering the actual API of
> the public types.

Mercurial history does not go that far in the past to be able to see if
synchronization for cache* fields was added at some point and why. My
conclusions are based solely on the state of current code.

Regards, Peter

>
>>
>> Venkat's patch therefore correctly fixes the remaining issue that is
>
> Okay. As I said it certainly makes things better.
>
> Cheers,
> David
>
>> observed when the shared SimpleTimeZone instance is being cloned
>> while also being accessed from multiple threads in read-only mode.
>> Invalidating cache of the cloned instance just before returning it
>> from clone() method means that instance obtained from
>> TimeZone.getDefault() will never get cached state from original
>> instance and will always have to re-compute it, but I think this is
>> still better than synchronizing on the original instance which may
>> never be optimized away (i.e. elided) by JIT.
>>
>> Regards, Peter
>>
>>>
>>> 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.
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>