[8] RFR of 8066985: Java Webstart downloading packed files can result in Timezone set to UTC
Kumar Srinivasan
kumar.x.srinivasan at oracle.com
Wed Mar 25 15:19:41 UTC 2015
On 3/25/2015 2:51 AM, mikhail cherkasov wrote:
> 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.
I see!, ok the changes look good, however my concern is the test,
should we throttle/adapt the test on the number of processors available
using Runtime.availableProcessors() ?
Kumar
>> 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