hg: jdk8/tl/jdk: 8000687: Correct javadoc typo for getLogWriter and setLogWriter
Changeset: 3c4be36de073 Author:lancea Date: 2012-10-10 11:15 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3c4be36de073 8000687: Correct javadoc typo for getLogWriter and setLogWriter Reviewed-by: alanb ! src/share/classes/java/sql/DriverManager.java
hg: jdk8/tl/jdk: 7192274: Deprecate LogManager addPropertyChangeListener and removePropertyChangeLister methods
Changeset: 6455182d2797 Author:alanb Date: 2012-10-10 20:47 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6455182d2797 7192274: Deprecate LogManager addPropertyChangeListener and removePropertyChangeLister methods Reviewed-by: mchung, lancea, chegar ! src/share/classes/java/util/logging/LogManager.java
hg: jdk8/tl/jdk: 8000712: Remove unused fields in SyncFactory
Changeset: 734ca9f4719c Author:lancea Date: 2012-10-10 17:34 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/734ca9f4719c 8000712: Remove unused fields in SyncFactory Reviewed-by: mchung ! src/share/classes/javax/sql/rowset/spi/SyncFactory.java
hg: jdk8/tl/langtools: 8000310: Clean up use of StringBuffer in langtools
Changeset: c46e0c9940d6 Author:jjg Date: 2012-10-10 18:44 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c46e0c9940d6 8000310: Clean up use of StringBuffer in langtools Reviewed-by: bpatel ! src/share/classes/com/sun/tools/classfile/Descriptor.java ! src/share/classes/com/sun/tools/doclets/formats/html/AbstractExecutableMemberWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/AbstractMemberWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/HtmlDocletWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/LinkFactoryImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/LinkOutputImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/TagletOutputImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/TagletWriterImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlDocWriter.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/Configuration.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/builders/AnnotationTypeBuilder.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/builders/ClassBuilder.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/taglets/LiteralTaglet.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/taglets/TagletManager.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/DirectoryManager.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/Extern.java ! src/share/classes/com/sun/tools/javac/code/Printer.java ! src/share/classes/com/sun/tools/javac/comp/Lower.java ! src/share/classes/com/sun/tools/javac/parser/JavacParser.java ! src/share/classes/com/sun/tools/javac/util/Convert.java ! src/share/classes/com/sun/tools/javac/util/List.java ! src/share/classes/com/sun/tools/javah/Gen.java ! src/share/classes/com/sun/tools/javah/LLNI.java ! src/share/classes/com/sun/tools/javah/Mangle.java
Re: Code review request: 7198507: [TEST_BUG] sun/security/tools/keytool/console.sh should be rewritten
Ping again. Thanks Max On 09/18/2012 05:26 PM, Weijun Wang wrote: First, I'm very glad to see that we are running manual tests. Then, please take a look at http://cr.openjdk.java.net/~weijun/7198507/webrev.00 Nothing is changed in the real keytool command calls, only some small changes: 1. Better prompt 2. Keystore file now /tmp/kkk.$$, clean and no conflict 3. Exit wherever a keytool command fails 4. Remove export. It causes an error on Solaris 5. Add a k after read. Only this can stop the process Thanks Max Original Message 7198507: [TEST_BUG] sun/security/tools/keytool/console.sh should be rewritten http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7198507 === *Description* test have 2 problems: 1. It failed on solaris platforms due incorrect shell syntax 2. It should be interactive but one always shows passed status on linux and windows while really additional variables should be set by user before running for successful execution and user should decide passed test or failed. There are no any messages for user when test running under jtreg. solaris log: --System.out:(0/0)-- --System.err:(1/244)*-- /net/stt-13.ru.oracle.com/export2/stt/newroot/regression/workspaces/1.8.0/1.8.0_fcsb55/j2se/test/sun/security/tools/keytool/console.sh: PASS=\\303\\244\\303\\266\\303\\244\\303\\266\\303\\244\\303\\266\\303\\244\\303\\266: is not an identifier result: Failed. Execution failed: exit code 1 linux log: --System.err:(22/3266)-- rm: cannot remove `kkk': No such file or directory /net/stt-13.ru.oracle.com/export2/stt/newroot/regression/workspaces/1.8.0/1.8.0_fcsb55/j2se/test/sun/security/tools/keytool/console.sh: line 66: /bin/keytool: No such file or directory .
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
No new webrev. I need a review of how to use SNIMatcher. See bellow
inline comments.
On 10/11/2012 7:38 AM, Brad Wetmore wrote:
On 10/10/2012 5:47 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
You got the others, but missed this one.
I think we discussed the style sometimes ago. If the sentence does not
start with a capital letter, then it does not need a period at the end
of the sentence. I can see both style in other specs.
Updated to add the period.
231: Might suggest some reason text for the
UnsupportedOperationException.
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
No problem.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
addToSNIServerNameList since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
Good point!
Or do it the way you did. This just adds a little extra source overhead.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
See the reply in SSLSocketImpl.java
Ok.
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames(example.com);
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
sun/security/ssl/HelloExtensions.java
=
423: This behavior is currently underspecified.
I think the behavior is specified in RFC 6066:
... If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake.
As means that the server will try to recognize every type of server
name.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about how to
use 1+ server names in server side.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the
Re: Code review request: 7198507: [TEST_BUG] sun/security/tools/keytool/console.sh should be rewritten
Looks fine to me. Xuelei On 10/11/2012 9:55 AM, Weijun Wang wrote: Ping again. Thanks Max On 09/18/2012 05:26 PM, Weijun Wang wrote: First, I'm very glad to see that we are running manual tests. Then, please take a look at http://cr.openjdk.java.net/~weijun/7198507/webrev.00 Nothing is changed in the real keytool command calls, only some small changes: 1. Better prompt 2. Keystore file now /tmp/kkk.$$, clean and no conflict 3. Exit wherever a keytool command fails 4. Remove export. It causes an error on Solaris 5. Add a k after read. Only this can stop the process Thanks Max Original Message 7198507: [TEST_BUG] sun/security/tools/keytool/console.sh should be rewritten http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7198507 === *Description* test have 2 problems: 1. It failed on solaris platforms due incorrect shell syntax 2. It should be interactive but one always shows passed status on linux and windows while really additional variables should be set by user before running for successful execution and user should decide passed test or failed. There are no any messages for user when test running under jtreg. solaris log: --System.out:(0/0)-- --System.err:(1/244)*-- /net/stt-13.ru.oracle.com/export2/stt/newroot/regression/workspaces/1.8.0/1.8.0_fcsb55/j2se/test/sun/security/tools/keytool/console.sh: PASS=\\303\\244\\303\\266\\303\\244\\303\\266\\303\\244\\303\\266\\303\\244\\303\\266: is not an identifier result: Failed. Execution failed: exit code 1 linux log: --System.err:(22/3266)-- rm: cannot remove `kkk': No such file or directory /net/stt-13.ru.oracle.com/export2/stt/newroot/regression/workspaces/1.8.0/1.8.0_fcsb55/j2se/test/sun/security/tools/keytool/console.sh: line 66: /bin/keytool: No such file or directory .
Re: Code review request for JEP-121
Hi, Vinnie, Here are my comments on the latest webrev 04. PBEParameterSpec PBEWithMD5AndDESCipher PBEWithMD5AndTripleDESCipher PBES1Core PBKDF2Core PBEKeyFactory = looks fine. PBEParameters.java = Well, the fields contains the new cipherParam field needed for PBES2 cipher, but the encoding is still for the older PBES1 cipher. = Perhaps it's cleaner to use a separate class for parameters for PBES2 cipher. The ASN.1 syntax is defined in PCKS#5v2.1 Appendix A.2 and B.2 PKCS12PBECipherCore = fine, although as I previously mentioned that it'll be easier to maintain and understand if we can refactor the code with a non-CipherCore object, so that no special handling needed for RC4. Can we file a separate bug/rfe to keep track of this refactoring? PBMAC1Core = Well, the HmacPKCS12PBESHA1 class (which you renamed to PBMAC1Core) implements the PKCS#12 v1 standard and is different from the PBMAC1 algorithms defined in PKCS#5 v2.1. So, the new comments at line 39-40 aren't correct. The two standards, i.e. PKCS#12 and PKCS#5, aren't consistent and have different ways on how the keys are derived. If you look at PKCS#5v2.1, it explicitly specified that the key shall be derived using PBKDF2 and the impl inside HmacPKCS12PBESHA1 relies on the PKCS12PBECipherCore.derive(...) method for deriving the keys. If the goal is about supporting PBMAC1 function defined in PKCS#5v2.1, then we need to have separate classes which use PBKDF2. = The HmacPKCS12PBESHA1 class is used by PKCS12 keystore class. So, we still need to keep it and can't shift it to the impl defined by PKCS#5v2.1. Currently, PKCS#12 only uses SHA1. Well, but things are confusing as is already... SunJCE = Given the above on PBMAC1Core, the // PBMAC1 comment on line 678 isn't correct. I am still thinking about the changes on PBEKey and PBES2Core classes, but thought that I should send you above comments first while I sort my thoughts out. Thanks, Valerie On 10/04/12 03:50, Vincent Ryan wrote: I've made further modifications including adding support to PBEParameterSpec for an AlgorithmParameterSpec object. This is used to convey parameters (in addition to salt and iteration count) to the underlying cipher. For example, AES requires an initialization vector so PBE algorithms that use AES need a mechanism to supply an IV parameter. The latest webrev is at: http://cr.openjdk.java.net/~vinnie/6383200/webrev.04/ On 4 Sep 2012, at 18:04, Vincent Ryan wrote: Thanks Valerie. I'd addressed your comments except the first one. Since RC4 is a stream cipher and RC2 is a block cipher they are handled slightly differently. It would be possible to re-factor this code to simplify it but I'd like to leave that for later as I'm under pressure to meet the next promotion date. The latest webrev is at: http://cr.openjdk.java.net/~vinnie/6383200/webrev.03/ On 08/31/12 03:05 AM, Valerie (Yu-Ching) Peng wrote: Vinnie, PKCS12PBECipherCore.java 1. Is it possible to replace the CipherCore object w/ CipherSpi object so to maximize the code re-use? The new code uses CipherSpi object for RC4 and CipherCore for RC2. Perhaps by using CipherSpi for both RC4 and RC2, we can have less code which would be easier to maintain... PBEKeyFactory 1. line 57, change the initial set size to 17 from 4? PBES2Core.java 1. the impls of the two following engineInit() methods are inconsistent, i.e. engineInit(int, Key, AlgorithmParameterSpec, SecureRandom) - expects IvParameterSpec engineInit(int, Key, AlgorithmParameters, SecureRandom) - expects objects created from PBEParameterSpec 2. The impl of engineGetParameters() currently returns objects created from PBEParameterSpec. It should return whatever is expected in the engineInit(...) calls, I'd think. Will send you the rest of comments later, Valerie On 08/29/12 08:20, Vincent Ryan wrote: On 06/ 1/12 07:18 PM, Vincent Ryan wrote: Hello Valerie, Could you please review these changes for JEP-121: http://cr.openjdk.java.net/~vinnie/6383200/webrev.00/ Thanks. The latest webrev is now available at: http://cr.openjdk.java.net/~vinnie/6383200/webrev.02/ I've incorporated review comments and made some fixes to the implementation of AES-based PBE algorithms. Thanks.
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/10/2012 7:54 PM, Xuelei Fan wrote:
No new webrev. I need a review of how to use SNIMatcher. See bellow
inline comments.
On 10/11/2012 7:38 AM, Brad Wetmore wrote:
On 10/10/2012 5:47 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
You got the others, but missed this one.
I think we discussed the style sometimes ago. If the sentence does not
start with a capital letter, then it does not need a period at the end
of the sentence. I can see both style in other specs.
Updated to add the period.
Sorry, I meant to say copyright change. Boy, did I goof on that one.
One of the copyright dates wasn't right, but the rest were.
I'll respond to the rest tomorrow. I have a sick PC to diagnose. :(
Brad
231: Might suggest some reason text for the
UnsupportedOperationException.
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
No problem.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
addToSNIServerNameList since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
Good point!
Or do it the way you did. This just adds a little extra source overhead.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
See the reply in SSLSocketImpl.java
Ok.
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames(example.com);
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
sun/security/ssl/HelloExtensions.java
=
423: This behavior is currently underspecified.
I think the behavior is specified in RFC 6066:
... If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake.
As means that the server will try to recognize every type of server
name.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/11/2012 12:21 PM, Bradford Wetmore wrote: On 10/10/2012 7:54 PM, Xuelei Fan wrote: No new webrev. I need a review of how to use SNIMatcher. See bellow inline comments. On 10/11/2012 7:38 AM, Brad Wetmore wrote: On 10/10/2012 5:47 AM, Xuelei Fan wrote: new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/ I guess you didn't need to have me as reviewer before going final with the CCC? The CCC has been finalized. ;-) I though you have done with spec review. Anyway, we still can make updates on specification within new bugs. javax/net/ssl/SSLSocketFactory.java === Change look good. 216: I think you need a period at end of sentence here. You got the others, but missed this one. I think we discussed the style sometimes ago. If the sentence does not start with a capital letter, then it does not need a period at the end of the sentence. I can see both style in other specs. Updated to add the period. Sorry, I meant to say copyright change. Boy, did I goof on that one. One of the copyright dates wasn't right, but the rest were. Oops, right, I missed the copyright dates. I double checked the copyright yesterday, but still missed this one. ;-) Xuelei
