Hi Steve,
The reifiedEntry() approach probably can help the default JarVerifier
work as expected, but if I read the
code correctly I doubt you can get the expected CodSigner[] and
Certificatte[] result from a "versioned"
JarFileEntry, after having read all bytes from the input stream
(obtained via jzf.getInputStream(JarFileEntry)),
as the JarEntry spec suggests,. As we are passing the "reified" entry
into the VerifierStream alone, without
any reference to the original jar file entry. It seems impossible for
the original jar file entry can trace back to
the corresponding certificates and code signers. This might be fixed by
passing in the original entry together
into the JarVerifier, but I doubt we might have a bigger issue here. I
suspect with this approach an external
verifier will have no easy way to verify the digit signature of the jar
entry via java.security APIs. I would assume
this is doable right now with current JarFile APIs, via a JarFile
object, a Manifest and a target JarEntry. The external
can get the signature via name -> manifest->attributes->signature
(basically just consider to move the
JarVerifier and couple sun.security.util classes out and use it as user
code)... but with this implementation
the name now is the root entry, but the bytes you can read from the
stream is from the versioned one.
We might want to bring in Max to take a look if what I said is really a
supported use scenario. I might be
wrong, not a security expert :-)
Btw, for a "normal" JarEntry/ZipEntry (not a JarFileEntry), shouldn't
the getInputStream(ze) simply return
the stream for the root entry? The current implementation of
getJarEntry(ze) does not seem right, as it
returns a "versioned" JarFileEntry. I don't think you want to pass this
one into VerifierStream directly,
right? Again, I think it might be desired (at least the spec is not
updated to say anything about "version")
to simply return the input stream for the root entry.
One more "nitpick". searchForVersionedEntry() now lookups the versioned
candidate via super.getEntry()
from version to BASE_VERSION, if the version is the latest version 9,
the base is 0, we are basically doing
this search for each non-versioned-entry inside this multi-release-jar
file 9 times everytime when the entry
is asked. In worse case scenario, a multi-release-jar, with huge number
of entries with a small portion are
versioned to 9, and you are iterating it via "entries". Each lookup
might be cheap, but it might be worth
considering adding some optimization.
Best,
Sherman
On 10/20/2015 5:16 PM, Steve Drach wrote:
Hi,
Let’s try again. I considered the issues brought up in the last
review and was able to not only remove the versionedEntry altogether,
I was able to greatly simplify the entire changeset. I removed all
changes to JarEntry and JarVerifier, and added a name field and some
simple methods to JarFileEntry. This solved a whole bunch of
potential issues. I’m also creating the jar files used in the tests so
there are no binaries in the changeset. One test,
MultiReleaseJarURLConnection, has an error on windows because it can’t
delete one of the created jar files. I don’t think I can do anything
about it — JDK-4823678 is a 12 year old bug that describes the
problem. If anyone has an idea on what I can do to make this a clean
test, please let me know.
Issue: https://bugs.openjdk.java.net/browse/JDK-8132734
JEP 238: https://bugs.openjdk.java.net/browse/JDK-8047305
Webrev:
http://cr.openjdk.java.net/~psandoz/multiversion-jar/jar-webrev/
<http://cr.openjdk.java.net/%7Epsandoz/multiversion-jar/jar-webrev/>
Thanks,
Steve
On Oct 15, 2015, at 8:47 PM, Xueming Shen <xueming.s...@oracle.com
<mailto:xueming.s...@oracle.com>> wrote:
On 10/15/15 1:53 AM, Paul Sandoz wrote:
On 15 Oct 2015, at 05:00, Xueming Shen <xueming.s...@oracle.com
<mailto:xueming.s...@oracle.com>> wrote:
I'm not sure if it is a good idea, from performance perspective, to
add a "versionEntry" field into the JarEntry
to support this feature, given most of the jar files might not be
multi-release-jar aware, and the Jar input&
output streams dont work with a multi-release jar directly. Why
should they all pay a runtime price for it. If
we really have to add an extra field, the JarFileEntry might be a
better place, and it might be desired to
define a new subclass JarFileEntryMR to use when the MR is enabled,
instead of adding directly into the existing
JarFileEntry.
According to jol there is currently space available due to
alignment. If there was not it would add about 4% in direct instance
size. But the actual footprint is likely to be chunkier because of
the string character storage for the name so the % increase in size
would be smaller e.g. perhaps on average < 2% which might be ok
given that i presume such entries are unlikely to be cached.
So i am not concerned about the size. If there was a way to design
it to avoid modification of existing classes all the better, but i
dunno if it is possible. Steve will surely know more.
Paul.
Let's try it from another angle:-) Based on the webrev, no one need
to and actually does create a
JarEntry with a versionedEntry, except JarFile, and JarFile only
creates its own version of JarEntry,
the JarFileEntry.
The only non-JarFile consumer of "versioned" JarEntry is the package
private JarVerifier.getCoderSigners,
and obviously it takes a JarFile together with the source JarEntry
(again, if this jarEntry is not from A
JarFile, it should never have a "versionedEntry")
So why do you want to put this field into the super class JarEntry,
not the JarFileEntry, don't see any
benefit of doing that.
While I'm writing this email, it appears to me that we might have a
more "severe" issue with the
general/base JarEntry class to hold the link to the "versionedEntry".
The "general" JarEntry object is
not associated with a specific JarFile (the JarFileEntry does). So
there is no way for
JarFile.getInputStream(ZipFile ze) to verify that the JarEntry passed
in and its "versionedEntry" is
actually belong to "this" JarFile in the following implementation, if
the "ze" is just a JarEntry but
NOT instanceof of a JarFileEntry
759 public synchronized InputStream getInputStream(ZipEntry ze)
760 throws IOException
761 {
762 maybeInstantiateVerifier();
763
764 if (ze instanceof JarEntry) {
765 ZipEntry vze = ((JarEntry)ze).versionedEntry;
766 if (vze != null) {
767 return getInputStream(vze);
768 }
769 }
770
I think it's a bug of the implementation if we don't check, as the
"versioned entry" is really
associated to the jar file that creates it. Sure, as I said above,
there is actually no way you can
create a general JarEntry or a JarFileEntry with a "versionedEntry"
from "outside", but it appears
to be possible (have not tried, just in theory) to mess up the
current mechanism by passing a
"jar entry" from one JarFile instance to another one, if two JarFile
instances are open on the same
multi-release-jar, but with different "version setting" policy...
But again, I still believe it might be a wrong approach to add such a
"versionedEntry" into any of the
JarEntry, including the JarFileEntry. As specified by the
specification, the returned entry should be the
jar entry pointing to the versioned entity in the Jar File, not the
root one. The question I would like to
ask is why do you even need the "root entry" at all, if there is a
matched versioned one. It might be
desired to have a mechanism to return the "base/root name" for such
an entry, but it probably does
not deserve a "dedicate" entry object.
-Sherman
