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

mikhail cherkasov mikhail.cherkasov at oracle.com
Tue Mar 24 20:14:54 UTC 2015


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