RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Paul Sandoz]

> On 6 Nov 2015, at 22:50, Xueming Shen <xueming.shen at oracle.com> wrote:
> 
> Hi Steve,
> 
> My apology for slow response. I was sick in bed since last Sat…
> 

Oh no, i hope you are getting better.


> I've scanned the update, here are my comments
> 
> (1) The newly added "configured" obviously will help in most use scenario, but (yes, there is
>     always a but :-)), given the related code is not synchronized, I'm pretty sure there is race
>     condition somewhere, for example betwen Ln#388 -- Ln#396, if there is a "faster" second
>     getEntry  catches up there, a base/root entry will return, but a versioned will return next
>     time if the isMultiRelease is set to true and there is a matched versioned entry for it.
> 

Steve and I were having a hard time determining the exact nature of what is actually thread safe in JarFile, it appears to be rather delicately balanced. We need to carefully rethink this aspect.

It would be so much easier if we could sort this out in the constructor, alas some cases seem to make it very tricky to do that.


> (2) Now the BASE_VERSION has been raised to 8, the logic in setVersioned(n) will actually block
>     any version num < 8, as now it is forced to be the BASE_VERSION, any lookup for versioned
>     will stop at BASE_VERSION. Is it a specified behavior?

It should be.


> adjust the BASE_VERSION to the version
>     being set 'n' might be a more reasonable approach?
> 

It does not make much sense to support versioned entries below the Java version that supports multi-release JARs, since such versioned entries will be ignored when the JarFile is processed on a corresponding JDK of that version, and when processed on a Java version that supports multi-release they effectively shadow to earlier or unversioned entries, thus it might induce unintended differences in behaviour if such entries differ in content.

Paul.


> (3) Again one more security concern you might want to confirm with vuln team (probably during their
>     review). With current implementation, in use scenario
>     a) it's multi-release jar
>     b) is multi-release jar enabled
>     c) all base/root entries are signed
>     d) the set of versioned entries are not signed.
> 
>     the implementation returns versioned entries successfully, they are unsigned. Is this a concern,
>     as arguably, the user is asking entry with the root name, and the corresponding entry for that
>     root name is signed. But we return a linked versioned-entry, and it's not signed. This might not
>     be  an issue at all. Please confirm this when doing security review.
> 
> -Sherman
> 
> On 11/6/15 9:23 AM, Steve Drach wrote:
>> Hi Sherman.
>> 
>> Would you please give this another pass?  I want make sure all your concerns are met.
>> 
>> Thanks
>> Steve
>> 
>>> On Nov 6, 2015, at 12:44 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>>> 
>>> Hi Steve,
>>> 
>>> This looks good to me (i only browsed the test code). It’s been around the block a few times :-) IMO, baring any major issues, it’s time to push and we can clean up any ancillary issues with later pushes.
>>> 
>>> Paul.
>>> 
>>>> On 5 Nov 2015, at 18:10, Steve Drach <steve.drach at oracle.com> wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> Here’s a new webrev that addresses the issues Paul brought up.  The versioned entry cache has been removed, the search space has been reduced, the documentation for setVersioned(int) and setRuntimeVersioned() has been updated to clarify when IllegalStateException is thrown, and the tests have been changed to accommodate a jar file with versions 9 and 10, rather than 8 and 9.
>>>> 
>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8132734 <https://bugs.openjdk.java.net/browse/JDK-8132734>
>>>> JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305 <https://bugs.openjdk.java.net/browse/JDK-8047305>
>>>> Webrev: http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/ <http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/>
>>>> 
>>>> Steve
>