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

[mikhail cherkasov]

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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>