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

mikhail cherkasov mikhail.cherkasov at oracle.com
Wed Mar 25 09:51:11 UTC 2015


On 3/25/2015 1:48 AM, Kumar Srinivasan wrote:
> Hi Mikhail,
>
> Nit:
> Utils.java:
> -            // reset timezone when all the packers have terminated
> +            // reset timezone when all the packer/unpacker instances have terminated
fixed: http://cr.openjdk.java.net/~mcherkas/8066985/webrev.03/
>
>
> DefaultTimeZoneTest.java
> What did you change between 01 and 02 revisions ? They appear to be the
> same ?
Please, see revisions 00 and 02. 01 has DefaultTimeZoneTest.java.rej, 02 
is the same but without this extra file.
>
> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>




More information about the core-libs-dev mailing list