[8] RFR of 8066985: Java Webstart downloading packed files can result in Timezone set to UTC

[Peter Levart]

On 03/19/2015 04:46 PM, Peter Levart wrote:
> On 03/19/2015 02:53 PM, Kumar Srinivasan wrote:
>> Mikhail,
>>
>> You can move the common utilitieschangeDefaultTimeZoneToUtc and
>> restoreDefaultTimeZoneto Utils.java.
>>
>> Also I am not sure how effective the test is ?  does it catch the 
>> issue ?
>> if you don't have the fix in PackerImpl and UnpackerImpl ?
>>
>> Otherwise it looks good, and I can sponsor this patch for you.
>>
>> Kumar
>
> Hi Mikhail,
>
> Is this code part of some utility so that it is executed exclusively 
> in it's own JVM, or can it also be executed by some public API or 
> internal JVM thread? It appears to be the later (used by 
> java.util.jar.Pack200 public API), but haven't checked.
>
> In case of the later, I think it is very strange that the code changes 
> global JVM timezone, albeit just temporarily. This could affect other 
> code that needs default TZ and executes concurrently.
>
> Even in case of the former, if the PackerImpl.pack can be executed by 
> multiple concurrent threads and if UnpackerImpl.unpack can be executed 
> by multiple concurrent threads, what guarantees that some 
> PackerImpl.pack thread is not executed concurrently with some 
> UnpackerImpl.unpack thread? You only track each method separately.
>
>
> Regards, Peter

I guess this default TZ manipulation is needed because 
JarOutputStream/JarInputStream don't provide constructors that would 
take a TZ object, but always use default TZ, right?

The cleanest way would be to add such constructors, but if this is too 
much for 8u, then perhaps some internal ThreadLocal<TZ> could be exposed 
to both PackerImpl/UnpackerImpl and Jar[Zip]Input[Output] stream 
internals. It think the use of default TZ in Jar/Zip/Streams is very 
localized. It is needed to convert local time (specified as DOS time) to 
and from epoch-based time exposed in ZipEntry. The code is located in 
package-private class java.util.zip.ZipUtils in methods dosToJavaTime 
/javaToDosTime (or equivalent extendedDosToJavaTime / 
javaToExtendedDosTime in JDK9).

Another way would be to provide a general thread-local default TZ 
override in TimeZone itself. Something like the following:

===================================================================
--- jdk/src/share/classes/java/util/TimeZone.java
+++ jdk/src/share/classes/java/util/TimeZone.java
@@ -630,8 +630,11 @@
       * method doesn't create a clone.
       */
      static TimeZone getDefaultRef() {
-        TimeZone defaultZone = defaultTimeZone;
+        TimeZone defaultZone = threadLocalTimeZone.get();
          if (defaultZone == null) {
+            defaultZone = defaultTimeZone;
+        }
+        if (defaultZone == null) {
              // Need to initialize the default time zone.
              defaultZone = setDefaultZone();
              assert defaultZone != null;
@@ -713,6 +716,20 @@
          defaultTimeZone = zone;
      }

+    public void withThreadDefaultDo(Runnable runnable) {
+        TimeZone prevZone = threadLocalTimeZone.get();
+        threadLocalTimeZone.set(this);
+        try {
+            runnable.run();
+        } finally {
+            if (prevZone == null) {
+                threadLocalTimeZone.remove();
+            } else {
+                threadLocalTimeZone.set(prevZone);
+            }
+        }
+    }
+
      /**
       * Returns true if this zone has the same rule and offset as 
another zone.
       * That is, if this zone differs only in ID, if at all.  Returns false
@@ -760,6 +777,8 @@
       */
      private String           ID;
      private static volatile TimeZone defaultTimeZone;
+    private static final ThreadLocal<TimeZone> threadLocalTimeZone =
+        new ThreadLocal<>();

      static final String         GMT_ID        = "GMT";
      private static final int    GMT_ID_LENGTH = 3;


Regards, Peter

>
>>
>>
>> On 3/18/2015 3:07 PM, Kumar Srinivasan wrote:
>>>
>>> Hello Mikhail,
>>>
>>>> Hi all,
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8066985
>>>>
>>>> The problem is that packer/unpacker changes global state( default 
>>>> time zone ) without proper synchronization:
>>>> http://hg.openjdk.java.net/jdk9/client/jdk/file/a159e5358e25/src/java.base/share/classes/com/sun/java/util/jar/pack/PackerImpl.java#l85 
>>>>
>>>>
>>>> however javadoc says that it's save to use it in concurrent way if 
>>>> each thread has it's own packer/unpacker instance:
>>>> http://hg.openjdk.java.net/jdk9/client/jdk/file/a159e5358e25/src/java.base/share/classes/java/util/jar/Pack200.java#l149 
>>>>
>>>>
>>>> The fix is:
>>>> 1. first packer/unpacker saves default time zone
>>>> 2. the last set it back.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~mcherkas/8066985/webrev.00/
>>>
>>> The above link is wrong!, it takes me to webrev for 8073187,
>>> which has only the PackerImpl changes.
>>>
>>> I am guessing the link should be:
>>> http://cr.openjdk.java.net/~mcherkas/8066985/webrev.00/
>>>
>>> Kumar
>>>
>>>>
>>>> Thanks,
>>>> Mikhail.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>