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

[Kumar Srinivasan]

Hi Mikhail,

Nit:
Utils.java:

-            // reset timezone when all the packers have terminated
+            // reset timezone when all the packer/unpacker instances have terminated

DefaultTimeZoneTest.java
What did you change between 01 and 02 revisions ? They appear to be the
same ?

Kumar


On 3/24/2015 1:14 PM, mikhail cherkasov wrote:
> Hi there,
>
> There's a new webrev: 
> http://cr.openjdk.java.net/~mcherkas/8066985/webrev.02/
>
> I reduced the a thread number in the test, because with big count of 
> threads I got OOM on JPRT.
> And extract common part form PackerImpl and UnpackerImpl to Utils.
>
> Thanks,
> Mikhail.
>
>
> On 3/20/2015 6:04 PM, mikhail cherkasov wrote:
>>
>> On 3/19/2015 7:29 PM, Peter Levart wrote:
>>> 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.
>> Agree, I'll move changeDefaultTimeZoneToUtc and 
>> restoreDefaultTimeZone to Util class, so TZ changes will be managed 
>> in one place  for both PackerImpl and UnpackerImpl classes.
>>
>> NonPack200 threads that reads/writes DTZ can collide with Pack200, 
>> but there's other bug that should fix this: 
>> https://bugs.openjdk.java.net/browse/JDK-8073187
>>
>> My fix is only for jdk8 and 7, and it focused on making 
>> packer/unpacker safe for concurrent use,  because now applets and 
>> JNLP uses packer/unpacker concurrently and this can
>> lead to DTZ changing to UTC.
>>
>>>>
>>>>
>>>> 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).
>> all solutions that you described require introducing a new API and 
>> this can not be done for jdk7 and 8, so that's why JDK-8073187 was 
>> created.
>> The main purpose of this fix is to make packed jars delivery save for 
>> applets/jnlp application.
>>
>>>
>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>