code review request: 7012160: read SF file in signed jar in streaming mode
Hi Sean
http://cr.openjdk.java.net/~weijun/7012160/webrev.00/
I've made changes to the following classes to enable streaming mode SF
file reading:
- java/util/jar/JarVerifier.java:
1. New verifyBlock method.
2. Change the constructor from JarVerifier(byte[]) to
JarVerifier(byte[], Manifest). In SignatureFileVerifier.processImpl(),
if we already confirm the *-Digest-Manifest header in the SF file
matches the whole MANIFEST.MF, there'se no need to parse the rest of the
SF file, since we can be sure that entries in the SF file are identical
to those in MANIFEST.MF. Of course, the content of the SF file still
needs to be fed into PKCS7Verifier to verify the signature.
- java/util/jar/JarFile.java:
Read DSA file in byte[] and SF file in InputStream, and call
JarVerifier.verifyBlock() to verify.
- java/util/jar/Manifest.java:
Adding update(byte[]) to read manifest in streaming mode. This is a new
public API.
- sun/security/pkcs/PKCS7.java:
New PKCS7Verifier class to verify SignedData in streaming mode. I
basically divide the SignerInfo.verify(PKCS7 block, byte[] data) method
into 3 parts and make them the 3 methods of this class.
- sun/security/util/SignatureFileVerifier.java:
Rewrite the processImpl(*) method to make use of new methods in PKCS7
and Manifest.
No new regression tests, use existing ones.
I've tried NetBeans profiler to look at the memory. The program simply
calls JarSigner.main(new String[]{-verify, x.jar}) and the signed
jar x.jar has 1 files inside.
BeforeAfter
byte[]3.6MB 2.8MB
char[]2.0MB 1.3MB
String1.1MB 650KB
So it does have some difference.
Thanks
Max
Original Message
*Change Request ID*: 7012160
*Synopsis*: read SF file in signed jar in streaming mode
=== *Description*
When a signed jar is verified, its SF file is read into a byte array and
verified against the signature. When there are many files in the jar,
the SF file can be very big. It will be better if the file can be read
in streaming mode.
*** (#1 of 1): 2011-01-13 12:23:25 GMT+00:00 weijun.w...@oracle.com
Re: review request for 7011998: diamond conversion for jgss and pkcs11
Get it! Thanks! Andrew On 1/14/2011 2:47 PM, Stuart Marks wrote: Diamond conversion doesn't do very much other than to make the code shorter, by removing redundancy. The meaning and function of the program aren't changed at all. Given a diamond, the compiler infers type arguments based on the context. In many cases where a variable is declared and initialized at the same location, the type arguments in the initializer are identical to those in the declaration. So, using diamond lets us get rid of a bit of noise. In the example you chose the benefit is fairly small. There are other cases where the benefit is greater, such as this one from src/share/classes/sun/security/pkcs11/SunPKCS11.java: -final MapDescriptor,Integer supportedAlgs = -new HashMapDescriptor,Integer(); +final MapDescriptor,Integer supportedAlgs = new HashMap(); The shorter initializer lets us fold things back onto a single line. Not a really big deal, but a little nicer I think. s'marks On 1/13/11 7:15 PM, Xuelei Fan wrote: Sorry, I did not look into this too much. I have a question about the diamond conversion. Why we want to make the change like the following code? What's the benefits? private final static MapBulkCipher,Boolean availableCache = - new HashMapBulkCipher,Boolean(8); + new HashMap(8); Thanks, Andrew On 1/14/2011 11:02 AM, Stuart Marks wrote: I did full clean builds of the JDK repo with -g:none, both with and without the diamond changes. I then compared all of the .class files in the two builds using the cmp command. The files were all identical, with the exception of two version classes which I think are auto-generated with date stamps or something. In any case all of the .class files corresponding to .java files in my changeset were byte-for-byte identical. s'marks On 1/13/11 4:41 PM, Valerie (Yu-Ching) Peng wrote: Which particular class did you compared? Just to double check... Thanks, Valerie On 01/13/11 04:15 PM, Stuart Marks wrote: Yes, the byte codes are identical. I compiled with -g:none before and after the changes and the classfiles are all identical. (Even though the bytecodes are identical, the classfiles would differ because of changed line number information, which is disabled with -g:none.) So, I assume this means that sunpkcs11.jar doesn't need to be updated, and that I can push this changeset without further changes? s'marks On 1/12/11 7:06 PM, Valerie (Yu-Ching) Peng wrote: The changes look good to me. BTW, I recall seeing in one of your earlier email that the byte code is the same w/ the usage of this diamond operator. Is this so? If not, then we need to update the sunpkcs11.jar also. Thanks, Valerie On 01/12/11 05:30 PM, Stuart Marks wrote: Hi Valerie, You're up next for diamond conversion. :-) These should be pretty straightforward. Almost all changes are variable initializations. There's one return statement, one use of diamond in a ternary operator (a ? b : c), and one whitespace fixup. Webrev is here: http://cr.openjdk.java.net/~smarks/reviews/7011998/webrev.0/ Thanks! s'marks
hg: jdk7/tl/langtools: 3 new changesets
Changeset: 2d5aff89aaa3 Author:mcimadamore Date: 2011-01-14 09:45 + URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/2d5aff89aaa3 6992698: JSR 292: remove support for transient syntax in polymorphic signature calls Summary: special syntax to denote indy return type through type parameters should be removed (and cast shall be used instead) Reviewed-by: jjg, jrose ! src/share/classes/com/sun/tools/javac/code/Symtab.java ! src/share/classes/com/sun/tools/javac/comp/Attr.java ! src/share/classes/com/sun/tools/javac/comp/Flow.java ! src/share/classes/com/sun/tools/javac/comp/Infer.java ! src/share/classes/com/sun/tools/javac/comp/Resolve.java ! src/share/classes/com/sun/tools/javac/jvm/Gen.java ! src/share/classes/com/sun/tools/javac/jvm/Items.java ! src/share/classes/com/sun/tools/javac/main/Main.java ! src/share/classes/com/sun/tools/javac/resources/compiler.properties ! src/share/classes/com/sun/tools/javac/util/Names.java - test/tools/javac/diags/examples/TypeParameterOnPolymorphicSignature.java - test/tools/javac/meth/InvokeDynTrans.out - test/tools/javac/meth/InvokeMHTrans.java - test/tools/javac/meth/InvokeMHTrans.out ! test/tools/javac/meth/TestCP.java ! test/tools/javac/meth/XlintWarn.java Changeset: c8d312dd17bc Author:mcimadamore Date: 2011-01-14 09:45 + URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/c8d312dd17bc 7007432: Test generic types well-formedness Summary: add a new kind of check (well-formedness of generic type w.r.t. declared bounds) in the type-harness Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/comp/Check.java + test/tools/javac/types/GenericTypeWellFormednessTest.java ! test/tools/javac/types/TypeHarness.java Changeset: 712be35e40b5 Author:mcimadamore Date: 2011-01-14 09:46 + URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/712be35e40b5 6949040: java.dyn package must be compiled with -target 7 or better Summary: issue error (rather than warning) when @PolymorphicSignature is found and target 7 Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/comp/MemberEnter.java ! src/share/classes/com/sun/tools/javac/resources/compiler.properties ! test/tools/javac/diags/examples.not-yet.txt
Re: code review 7011497: new CertPathValidatorException.BasicReason enum constant for constrained algorithm
Hi Sean, webrev: Would you please review the update again. I integrate the fix for 7011497 and 7012357 together. Comparing with previous webrev, the following updates are unchanged: src/share/classes/java/security/cert/CertPathValidatorException.java src/share/classes/sun/security/provider/certpath/AlgorithmChecker.java src/share/classes/sun/security/validator/SimpleValidator.java other test files. The following are new changes for CR 7012357: src/share/classes/sun/security/provider/certpath/ForwardBuilder.java src/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorEndEntity.java Thanks, Xuelei On 1/14/2011 11:10 AM, Xuelei Fan wrote: We don't checking the SKID and AKID during searching for the trust anchor. I have filled a new CR for the issue, 7012357, Improve trust anchor searching method during cert path validation. I will have this commented out block in CPValidatorEndEntity.java. I will use this test case for CR 7012357. Thanks, Xuelei On 1/14/2011 12:44 AM, Xuelei Fan wrote: I just realized, if subject KID and issuer KID works, the cert path validation should be able to find the proper trust anchor. I will look into the issue tomorrow. Xuelei On 1/14/2011 12:27 AM, Xuelei Fan wrote: On 1/14/2011 12:05 AM, Sean Mullan wrote: On 1/13/11 6:38 AM, Xuelei Fan wrote: Hi Sean, Would you please review the fix for CR 7011497? http://cr.openjdk.java.net/~xuelei/7011497/webrev/ Thanks, Xuelei CPValidatorEndEntity.java: 307 /* coment out useless trust anchor 308 is = new ByteArrayInputStream(trustAnchor_SHA1withRSA_512.getBytes()); 309 cert = cf.generateCertificate(is); 310 anchor = new TrustAnchor((X509Certificate)cert, null); 311 anchors.add(anchor); 312 */ Why do you leave this code in with this comment? If I have this block. The cert path validation cannot find the proper trust anchor. As there are two trusted certificates, they are almost the same except the key size (one key size is 1024, another one is 512). In cert path validation, once a trust anchor found, if the signature is not valid, I think no more effort to test more trust anchors. I was wondering whether it is worthy to try more trust anchors. It's expensive! Thanks for the review. Xuelei Otherwise, looks good. --Sean
Re: code review 7011497: new CertPathValidatorException.BasicReason enum constant for constrained algorithm
On 1/15/2011 1:30 AM, Xuelei Fan wrote: Hi Sean, webrev: http://cr.openjdk.java.net/~xuelei/7011497/webrev/ Would you please review the update again. I integrate the fix for 7011497 and 7012357 together. Comparing with previous webrev, the following updates are unchanged: src/share/classes/java/security/cert/CertPathValidatorException.java src/share/classes/sun/security/provider/certpath/AlgorithmChecker.java src/share/classes/sun/security/validator/SimpleValidator.java other test files. The following are new changes for CR 7012357: src/share/classes/sun/security/provider/certpath/ForwardBuilder.java src/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorEndEntity.java Thanks, Xuelei On 1/14/2011 11:10 AM, Xuelei Fan wrote: We don't checking the SKID and AKID during searching for the trust anchor. I have filled a new CR for the issue, 7012357, Improve trust anchor searching method during cert path validation. I will have this commented out block in CPValidatorEndEntity.java. I will use this test case for CR 7012357. Thanks, Xuelei On 1/14/2011 12:44 AM, Xuelei Fan wrote: I just realized, if subject KID and issuer KID works, the cert path validation should be able to find the proper trust anchor. I will look into the issue tomorrow. Xuelei On 1/14/2011 12:27 AM, Xuelei Fan wrote: On 1/14/2011 12:05 AM, Sean Mullan wrote: On 1/13/11 6:38 AM, Xuelei Fan wrote: Hi Sean, Would you please review the fix for CR 7011497? http://cr.openjdk.java.net/~xuelei/7011497/webrev/ Thanks, Xuelei CPValidatorEndEntity.java: 307 /* coment out useless trust anchor 308 is = new ByteArrayInputStream(trustAnchor_SHA1withRSA_512.getBytes()); 309 cert = cf.generateCertificate(is); 310 anchor = new TrustAnchor((X509Certificate)cert, null); 311 anchors.add(anchor); 312 */ Why do you leave this code in with this comment? If I have this block. The cert path validation cannot find the proper trust anchor. As there are two trusted certificates, they are almost the same except the key size (one key size is 1024, another one is 512). In cert path validation, once a trust anchor found, if the signature is not valid, I think no more effort to test more trust anchors. I was wondering whether it is worthy to try more trust anchors. It's expensive! Thanks for the review. Xuelei Otherwise, looks good. --Sean
hg: jdk7/tl/langtools: 6419926: JSR 199: FileObject.toUri() generates URI without schema (Solaris)
Changeset: 7c7c1787fbbe Author:jjg Date: 2011-01-14 11:45 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/7c7c1787fbbe 6419926: JSR 199: FileObject.toUri() generates URI without schema (Solaris) Reviewed-by: mcimadamore + test/tools/javac/api/T6419926.java
hg: jdk7/tl/langtools: 6571165: Minor doc bugs in JavaCompiler.java
Changeset: 0a509c765657 Author:jjg Date: 2011-01-14 11:55 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/0a509c765657 6571165: Minor doc bugs in JavaCompiler.java Reviewed-by: mcimadamore ! src/share/classes/javax/tools/JavaCompiler.java
hg: jdk7/tl/langtools: 7011272: langtools build.xml should provide a patch target
Changeset: 19f9b6548c70 Author:ksrini Date: 2011-01-14 13:59 -0800 URL: http://hg.openjdk.java.net/jdk7/tl/langtools/rev/19f9b6548c70 7011272: langtools build.xml should provide a patch target Reviewed-by: jonathan, jjh ! make/build.xml
