Note to readers, I sent an earlier incomplete version of this email due to fat finger syndrome. Hopefully this response will be complete.
Hi Sherman, Thanks for looking at this. > (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. I guess JarFile was thread safe since getEntry was idempotent before I changed it. It seems to me we could synchronize/lock the code after line 392 with little loss in performance, although the recursive nature of getManifest might cause a problem. As Paul said, we need to rethink this. > > (2) Now the BASE_VERSION has been raised to 8, the logic in setVersioned(n) > will actually bl > 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? adjust the > BASE_VERSION to the version > being set 'n' might be a more reasonable approach? Paul’s explanation is correct. The ‘set’ methods establish an upper bound whereas the BASE_VERSION is a lower bound. > > (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. I know I’ve discussed this with Max several times. Whenever the contents of a signed jar file as entries added to it, it should be resigned. So, if folks do the right thing, your scenario shouldn’t occur, although I don’t think there is a way to guarantee that. I will point this out to the vulnerability team. Again, thanks for your comments. Steve > > -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.san...@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.dr...@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 >
