8191216: SimpleTimeZone.clone() has a data race on cache fields
Reviewed-by: alanb, naoto
--- a/src/java.base/share/classes/java/util/SimpleTimeZone.java Mon Dec 11 11:45:02 2017 -0800
+++ b/src/java.base/share/classes/java/util/SimpleTimeZone.java Tue Dec 12 00:30:57 2017 +0100
@@ -548,12 +548,11 @@
computeOffset:
if (useDaylight) {
- synchronized (this) {
- if (cacheStart != 0) {
- if (date >= cacheStart && date < cacheEnd) {
- offset += dstSavings;
- break computeOffset;
- }
+ Cache cache = this.cache;
+ if (cache != null) {
+ if (date >= cache.start && date < cache.end) {
+ offset += dstSavings;
+ break computeOffset;
}
}
BaseCalendar cal = date >= GregorianCalendar.DEFAULT_GREGORIAN_CUTOVER ?
@@ -671,14 +670,13 @@
}
private int getOffset(BaseCalendar cal, BaseCalendar.Date cdate, int year, long time) {
- synchronized (this) {
- if (cacheStart != 0) {
- if (time >= cacheStart && time < cacheEnd) {
- return rawOffset + dstSavings;
- }
- if (year == cacheYear) {
- return rawOffset;
- }
+ Cache cache = this.cache;
+ if (cache != null) {
+ if (time >= cache.start && time < cache.end) {
+ return rawOffset + dstSavings;
+ }
+ if (year == cache.year) {
+ return rawOffset;
}
}
@@ -689,11 +687,7 @@
if (time >= start && time < end) {
offset += dstSavings;
}
- synchronized (this) {
- cacheYear = year;
- cacheStart = start;
- cacheEnd = end;
- }
+ this.cache = new Cache(year, start, end);
} else {
if (time < end) {
// TODO: support Gregorian cutover. The previous year
@@ -711,12 +705,7 @@
}
}
if (start <= end) {
- synchronized (this) {
- // The start and end transitions are in multiple years.
- cacheYear = (long) startYear - 1;
- cacheStart = start;
- cacheEnd = end;
- }
+ this.cache = new Cache((long) startYear - 1, start, end);
}
}
return offset;
@@ -876,7 +865,7 @@
* Generates the hash code for the SimpleDateFormat object.
* @return the hash code for this object
*/
- public synchronized int hashCode()
+ public int hashCode()
{
return startMonth ^ startDay ^ startDayOfWeek ^ startTime ^
endMonth ^ endDay ^ endDayOfWeek ^ endTime ^ rawOffset;
@@ -1201,19 +1190,27 @@
/**
* Cache values representing a single period of daylight saving
- * time. When the cache values are valid, cacheStart is the start
- * time (inclusive) of daylight saving time and cacheEnd is the
- * end time (exclusive).
+ * time. Cache.start is the start time (inclusive) of daylight
+ * saving time and Cache.end is the end time (exclusive).
*
- * cacheYear has a year value if both cacheStart and cacheEnd are
- * in the same year. cacheYear is set to startYear - 1 if
- * cacheStart and cacheEnd are in different years. cacheStart is 0
- * if the cache values are void. cacheYear is a long to support
- * Integer.MIN_VALUE - 1 (JCK requirement).
+ * Cache.year has a year value if both Cache.start and Cache.end are
+ * in the same year. Cache.year is set to startYear - 1 if
+ * Cache.start and Cache.end are in different years.
+ * Cache.year is a long to support Integer.MIN_VALUE - 1 (JCK requirement).
*/
- private transient long cacheYear;
- private transient long cacheStart;
- private transient long cacheEnd;
+ private static final class Cache {
+ final long year;
+ final long start;
+ final long end;
+
+ Cache(long year, long start, long end) {
+ this.year = year;
+ this.start = start;
+ this.end = end;
+ }
+ }
+
+ private transient volatile Cache cache;
/**
* Constants specifying values of startMode and endMode.
@@ -1282,9 +1279,8 @@
// Maximum number of rules.
private static final int MAX_RULE_NUM = 6;
- private synchronized void invalidateCache() {
- cacheYear = startYear - 1;
- cacheStart = cacheEnd = 0;
+ private void invalidateCache() {
+ cache = null;
}
//----------------------------------------------------------------------
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/util/TimeZone/SimpleTimeZoneCloneRaceTest.java Tue Dec 12 00:30:57 2017 +0100
@@ -0,0 +1,137 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.SimpleTimeZone;
+import java.util.TimeZone;
+import java.util.function.Supplier;
+
+/*
+ * @test
+ * @bug 8191216
+ * @summary test that provokes race between cloning and lazily initializing
+ * SimpleTimeZone cache fields
+ */
+public class SimpleTimeZoneCloneRaceTest {
+
+ public static void main(String[] args) throws InterruptedException {
+
+ // shared TZ user repeatedly samples sharedTZ and calculates offset
+ // using the shared instance
+ TimeZoneUser sharedTZuser = new TimeZoneUser(() -> sharedTZ);
+
+ // cloned TZ user repeatedly samples sharedTZ then clones it and
+ // calculates offset using the clone...
+ TimeZoneUser clonedTZuser = new TimeZoneUser(() -> {
+ // sample shared TZ
+ TimeZone tz = sharedTZ;
+ // do some computation that takes roughly the same time as it takes
+ // sharedTZUser to start changing cache fields in shared TZ
+ cpuHogTZ.getOffset(time);
+ // now clone the sampled TZ and return it, hoping the clone is done
+ // at about the right time....
+ return (TimeZone) tz.clone();
+ });
+
+ // start threads
+ Thread t1 = new Thread(sharedTZuser);
+ Thread t2 = new Thread(clonedTZuser);
+ t1.start();
+ t2.start();
+
+ // plant new SimpleTimeZone instances for 2 seconds
+ long t0 = System.currentTimeMillis();
+ do {
+ TimeZone tz1 = createSTZ();
+ TimeZone tz2 = createSTZ();
+ cpuHogTZ = tz1;
+ sharedTZ = tz2;
+ } while (System.currentTimeMillis() - t0 < 2000L);
+
+ sharedTZuser.stop = true;
+ clonedTZuser.stop = true;
+ t1.join();
+ t2.join();
+
+ System.out.println(
+ String.format("shared TZ: %d correct, %d incorrect calculations",
+ sharedTZuser.correctCount, sharedTZuser.incorrectCount)
+ );
+ System.out.println(
+ String.format("cloned TZ: %d correct, %d incorrect calculations",
+ clonedTZuser.correctCount, clonedTZuser.incorrectCount)
+ );
+ if (clonedTZuser.incorrectCount > 0) {
+ throw new RuntimeException(clonedTZuser.incorrectCount +
+ " fatal data races detected");
+ }
+ }
+
+ static SimpleTimeZone createSTZ() {
+ return new SimpleTimeZone(-28800000,
+ "America/Los_Angeles",
+ Calendar.APRIL, 1, -Calendar.SUNDAY,
+ 7200000,
+ Calendar.OCTOBER, -1, Calendar.SUNDAY,
+ 7200000,
+ 3600000);
+ }
+
+ static volatile TimeZone cpuHogTZ = createSTZ();
+ static volatile TimeZone sharedTZ = createSTZ();
+ static final long time;
+ static final long correctOffset;
+
+ static {
+ TimeZone tz = createSTZ();
+ Calendar cal = Calendar.getInstance(tz, Locale.ROOT);
+ cal.set(2000, Calendar.MAY, 1, 0, 0, 0);
+ time = cal.getTimeInMillis();
+ correctOffset = tz.getOffset(time);
+ }
+
+ static class TimeZoneUser implements Runnable {
+ private final Supplier<? extends TimeZone> tzSupplier;
+
+ TimeZoneUser(Supplier<? extends TimeZone> tzSupplier) {
+ this.tzSupplier = tzSupplier;
+ }
+
+ volatile boolean stop;
+ int correctCount, incorrectCount;
+
+ @Override
+ public void run() {
+ while (!stop) {
+ TimeZone tz = tzSupplier.get();
+ int offset = tz.getOffset(time);
+ if (offset == correctOffset) {
+ correctCount++;
+ } else {
+ incorrectCount++;
+ }
+ }
+ }
+ }
+}