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