Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Xuelei Fan]

About the prefix, it may follow RFC 5056 (See page 7, section 2.1).
   o  Specifications of channel bindings for any secure channels MUST
  provide for a single, canonical octet string encoding of the
  channel bindings.  Under this framework, channel bindings MUST
  start with the channel binding unique prefix followed by a colon
  (ASCII 0x3A).

Xuelei


On 6/9/2020 8:52 AM, Alexey Bakhtin wrote:

Hello Sean,

Thank you for the link. I’ll follow it to create CSR

I could not find any clear document or specification for this Channel Binding 
format.
The only document I found that describes this format is the following:
https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication
So, it is hard to say - is it a standard or Microsoft implementation specific

Regards
Alexey


On 9 Jun 2020, at 18:35, Sean Mullan  wrote:

On 6/8/20 5:33 PM, Alexey Bakhtin wrote:

Hello Sean,
Yes, I think we'll need CSR and release notes as soon as this patch adds new 
property.
I do not know exact process for it, so I will be grateful if you could explain 
me exact steps.


The CSR process is documented at 
https://wiki.openjdk.java.net/display/csr/Main. It should be fairly 
self-explanatory but let me know if you have questions.

For the release note, we can tackle that later once the CSR is approved now I have tagged 
the issue with the "release-note=yes" label so we don't forget it.


This patch was developed to address compatibility issue with new LDAP 
authentication over SSL/TLS announced by Microsoft [1]. It is not related to 
RFC 5801. In my opinion “com.sun.jndi.ldap.tls.cbtype” name looks more suitable 
for this property and should allow backport it to early JDK versions.


Good point about backporting.

What RFC or specification defines the format you are using for the channel binding in 
TlsChannelBinding.java, specifically where the type prefix is encoded as 
"tls-server-end-point:" followed by the binding data? I have looked through 
various RFCs but I can't find exactly where this format is defined, so I am wondering if 
this is a standard encoding or not.

Thanks,
Sean


[1] - 
https://support.microsoft.com/en-au/help/4034879/how-to-add-the-ldapenforcechannelbinding-registry-entry
Regards
Alexey

On 8 Jun 2020, at 22:03, Sean Mullan  wrote:

(resending to all lists on the review)

I'm just catching up a bit on this review.

Sorry if this has mentioned before, but are you planning to write a CSR and release note? 
I think this is needed for the com.sun.jndi.ldap.tls.cbtype property. I'm also wondering 
if this property should be documented in the javadocs, and why it is not a standard 
property (i.e. "java.naming.ldap.tls.cbtype").

I was also wondering what relation this has to the "G2" standard SASL mechanisms defined 
in RFC 5801 [1], and whether that is something we should be using to negotiate this channel 
binding, and if not, why not. Or if this is something that is implementation-specific and will only 
work with Microsoft LDAP technology, in which case, we might want to make that more explicit, 
perhaps by including "microsoft" or something like that in the property name.

Thanks,
Sean

[1] https://tools.ietf.org/html/rfc5801

On 6/8/20 9:07 AM, Aleks Efimov wrote:

Hi Alexey,
I've looked through LdapCtx/LdapClient/SaslBind changes:
Do we need to check if CHANNEL_BINDING is set explicitly for all connection 
types? Maybe we can move the check inside 'if (conn.sock instanceof SSLSocket) 
{' block.
Also, instead of setting CHANNEL_BINDING in context environment and then 
removing it in finally block, it would be better to clone the environment, put 
calculated CHANNEL_BINDING into it, and pass the cloned one to 
Sasl.createSaslClient.
Another suggestion about the code that verifies if both properties are set 
before connection is started:
As you've already mentioned the new code in LdapCtx is only needed for checking 
if timeout is set. Could we try to remove LdapCtx::cbType field and all related 
methods from LdapCtx (this class is already over-complicated and hard to read) 
and replace it with some static method in LdapSasl? It will help to localize 
all changes to LdapSasl except for one line in LdapCtx.
I mean something like this:
Replace
+
+// verify LDAP channel binding property
+if (cbType != null && connectTimeout == -1)
+throw new 
NamingException(TlsChannelBinding.CHANNEL_BINDING_TYPE +
+" property requires " +
+CONNECT_TIMEOUT +
+" property is set.");
With
+ 
LdapSasl.checkCbParameters((String)envprops.get(TlsChannelBinding.CHANNEL_BINDING_TYPE),
 connectTimeout);
And add something like that to LdapSasl (or maybe pass the full env here):
+ public static void checkCbParameters(String cbTypePropertyValue, int 
connectTimeout) throws NamingException {
+ 

Re: FYI: new javadoc tag to document system properties

[Xuelei Fan]

In JCE and JSSE, the public APIs definition (javax.net.ssl) and the 
internal implementation (sun.security.ssl) are separated.  The system 
property can be defined in the internal implementation classes.  I think 
we should add the @systemProperty on the public APIs, right?


The public API class and the implementation class may be not a 
one-to-one map.  Multiple public APIs may be impacted by the system 
property.  How to handle such cases?


Thanks,
Xuelei

On 11/14/2018 2:27 PM, Jonathan Gibbons wrote:
This is an FYI to announce some initial, long-overdue support in javadoc 
for documenting system properties (JDK-5076751).


Currently, system properties are just documented using ad-hoc narrative 
text, which is fine if you know where to look for any given property.


JDK 12 introduces a new inline javadoc tag, {@systemProperty 
/property-name/} which can be used to "declare" the name of a system 
property. You can use this tag as a drop-in replacement for the 
plain-text property name; it will have no overt effect on the narrative 
text, but it will cause the property name to be put into the search 
index and the A-Z index. Thus, property names will become searchable, to 
allow users to easily find the definition of any such system property.


Adding support into javadoc is only part of the story. Now that the 
support is in javadoc, we want to update the API documentation, so that 
the documentation is updated for all Java SE and JDK system properties.


Where should the tag be used?  The tag should be used in the text of the 
defining instance of the property.  This is where the characteristics of 
the system property are described, which may include information like: 
"what is the property for", "how and when is it set", "can it be 
modified", and so on. For example, it could be used in the docs for 
System.getProperties [1] or Networking Properties [2] In general, it 
should -not- be used in situations that merely refer to the system 
property by name.  For example, the docs for Path.toAbsolutePath [3] 
make a reference to the system property user.dir, but that is not a 
definition of the property.


Going forward, in future releases, we expect to explore some 
enhancements to this feature. Here are some of the ideas that have been 
suggested:


  * Create a "summary page" that lists all the system properties. This
    would be somewhat similar to the current top-level "Constant Values"
    page.
  * Add information regarding the "scope" of the definition: is it
    defined by the Java SE specification, or JDK, or JavaFX, etc. This
    information could be used to organize the content on the summary page.
  * Update @see and {@link} to be able to refer to system properties.
  * Allow a short description to be included in the {@systemProperty}
    tag. This text could be included in the search index, the A-Z index,
    and the summary page.

-- Jon


[1]: 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/System.html#getProperties() 

[2]: 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/doc-files/net-properties.html 

[3]: 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Path.html#toAbsolutePath() 

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Xuelei Fan]

Looks fine to me.  Thanks!

Xuelei

On 6/28/2018 5:28 AM, Seán Coffey wrote:

Thanks for reviewing Xuelei,

I do acknowledge that the new TLS v1.3 code has greatly improved the 
logging output for related operations. I think the main drive with this 
enhancement is to use the new JFR API to capture interesting events. We 
can revisit the Logger requirements if there's a strong opinion from 
users. By default, both the new Logger and JFR output will be switched 
off. I've just made an edit to the JFR jfc files to disable these events.


"I may suggest remove this method and use Key.getAlgorithm() directly." 
- Yes, great idea. Done.


Thanks for feedback on Finished.java edits. I did question to myself 
whether fewer edits were sufficient given the client/server nature of 
the handshake.


I've refreshed the webrev :

http://cr.openjdk.java.net/~coffeys/webrev.8148188.v3/webrev/

some jfr side edits also :

* Change label from "X.509 Certificate" to "X509 Certificate" - JFR test 
fails with "." usage
* Move the instance field variable name in CertChainEvent to "certChain" 
- JFR tests discourage use of  "ID" in "certIDs"


regards,
Sean.


On 27/06/2018 21:11, Xuelei Fan wrote:

Finished.java
-
In each handshake, Finished messages are delivered twice.  One from 
client, and the other one from the server side.  Catch Finished 
message and use the final one of them should be sufficient.


In the Finished.java implementation, the signal of the final Finished 
message is the handshakeFinished field is set to true.


Please move line 582:
 582 recordEvent(chc.conContext.conSession);
to line 558:
 556 // handshake context cleanup.
 557 chc.handshakeFinished = true;
 558

Please move line 632:
 632 recordEvent(shc.conContext.conSession);
to line 608:
 606 // handshake context cleanup.
 607 shc.handshakeFinished = true;
 608

Please remove line 838:
 838 recordEvent(shc.conContext.conSession);

Please remove line 984:
 984 recordEvent(chc.conContext.conSession);

No more comment.

Thanks,
Xuelei

On 6/27/2018 11:57 AM, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+    .collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the above 
code will create a new instance.  Is the return value of 
cce.isEnabled() dynamically changed or static?


Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.


Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't 
realized how much the handshaker code had changed. Hopefully, I'll 
get a review from security dev team on that front.


Regards the JFR semantics, I believe the edits should match majority 
of requests . I still have a preference for the test infra design 
for these new logger/JFR tests used in version 1 of webrev. I think 
it makes sense to keep the test functionality together - no sense in 
separating them out into different components IMO. Also, some of the 
edits to the JFR testing were made to test for the new dual 
log/record functionality. I might catch up with you tomorrow to see 
what the best arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and 
should be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The 
purpose of the control attribute is so to provide parameterized 
configuration of events for JMC.  As it is now, the security 
events will be enabled when a user turns on the exception events.

Makes sense. I'll remove that parameter.


X509CertEvent:
You should use milliseconds since epoch to represent a date 
instead of a string value, i.e.


    @Label("Valid From")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validFrom;

    @Label("Valid Until")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validUntil;

The CertificateValidity class operates on Date Object values. I'll 
work with the Date.getTime() method then (and update the Logger 
formatting)

This following annotation adds little value

@Description

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Xuelei Fan]

Finished.java
-
In each handshake, Finished messages are delivered twice.  One from 
client, and the other one from the server side.  Catch Finished message 
and use the final one of them should be sufficient.


In the Finished.java implementation, the signal of the final Finished 
message is the handshakeFinished field is set to true.


Please move line 582:
 582 recordEvent(chc.conContext.conSession);
to line 558:
 556 // handshake context cleanup.
 557 chc.handshakeFinished = true;
 558

Please move line 632:
 632 recordEvent(shc.conContext.conSession);
to line 608:
 606 // handshake context cleanup.
 607 shc.handshakeFinished = true;
 608

Please remove line 838:
 838 recordEvent(shc.conContext.conSession);

Please remove line 984:
 984 recordEvent(chc.conContext.conSession);

No more comment.

Thanks,
Xuelei

On 6/27/2018 11:57 AM, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+    .collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the above 
code will create a new instance.  Is the return value of cce.isEnabled() 
dynamically changed or static?


Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.


Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't realized 
how much the handshaker code had changed. Hopefully, I'll get a review 
from security dev team on that front.


Regards the JFR semantics, I believe the edits should match majority 
of requests . I still have a preference for the test infra design for 
these new logger/JFR tests used in version 1 of webrev. I think it 
makes sense to keep the test functionality together - no sense in 
separating them out into different components IMO. Also, some of the 
edits to the JFR testing were made to test for the new dual log/record 
functionality. I might catch up with you tomorrow to see what the best 
arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and should 
be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose 
of the control attribute is so to provide parameterized 
configuration of events for JMC.  As it is now, the security events 
will be enabled when a user turns on the exception events.

Makes sense. I'll remove that parameter.


X509CertEvent:
You should use milliseconds since epoch to represent a date instead 
of a string value, i.e.


    @Label("Valid From")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validFrom;

    @Label("Valid Until")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validUntil;

The CertificateValidity class operates on Date Object values. I'll 
work with the Date.getTime() method then (and update the Logger 
formatting)

This following annotation adds little value

@Description("Details of Security Property")

I would either remove the annotation, or provide information that 
helps a user understand the event. For instance, what is X509, and 
what kind of certificates are we talking about?
Yes - that looks like the wrong Description. I'll review all of these 
fields.


@Category("Java Application")

I'm a bit worried that we will pollute the "Java Application" 
namespace if we add lots of JDK events in that category. We may want 
to add a new top level category "Java Development Kit", analogous to 
the "Java Virtual Machine" category, and put all security related 
events in subcategory, perhaps called "Security".

Yes - Open to suggestions. "Security" sounds like a good suggestion.


@Label("X509Cert")

The label should be human readable name, with spaces, title cased 
etc. I would recommend "X.509 Certificate". In general, avoid 
abbreviations like "certs" and instead use labels such as 
"Certificate Chain". The label should be short and not a full sentence.


For details see,
https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html

I think it would be good to separate testing of JFR a

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Xuelei Fan]

KeyUtil.java

167  public static String getKeyAlgorithm(Key key) {

I may suggest remove this method and use Key.getAlgorithm() directly. 
The KeyUtil.getKeyAlgorithm() is incomplete, and may need additional 
maintenance if new key algorithms are added in the future.  For example, 
in JDK 11, "RSASSA-PSS" and "XDH" are added, but we really forgot that 
we may need to update this file as well.


On 6/27/2018 12:14 PM, Seán Coffey wrote:



On 27/06/2018 19:57, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+    .collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the above 
code will create a new instance.  Is the return value of 
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their event.isEnabled 
calls are instance methods and follow a similar pattern. The JFR team 
assure me that the JIT can optimize away such calls.

Got it.



Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got 
strong feedback to also consider use of Logger (esp. in cases where the 
jdk.jfr module is not available)


There are three logging mechanisms: JFR, record event log and the 
component debugging log.  It is a little bit overloaded to me.  If 
jdk.jfr is not available, the existing component debugging log may be 
used instead.  I think you must have weight this decision in the past, 
so I will accept your decision if you want to keep it.


Thanks,
Xuelei


regards,
Sean.



Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't 
realized how much the handshaker code had changed. Hopefully, I'll 
get a review from security dev team on that front.


Regards the JFR semantics, I believe the edits should match majority 
of requests . I still have a preference for the test infra design for 
these new logger/JFR tests used in version 1 of webrev. I think it 
makes sense to keep the test functionality together - no sense in 
separating them out into different components IMO. Also, some of the 
edits to the JFR testing were made to test for the new dual 
log/record functionality. I might catch up with you tomorrow to see 
what the best arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and should 
be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose 
of the control attribute is so to provide parameterized 
configuration of events for JMC.  As it is now, the security events 
will be enabled when a user turns on the exception events.

Makes sense. I'll remove that parameter.


X509CertEvent:
You should use milliseconds since epoch to represent a date instead 
of a string value, i.e.


    @Label("Valid From")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validFrom;

    @Label("Valid Until")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validUntil;

The CertificateValidity class operates on Date Object values. I'll 
work with the Date.getTime() method then (and update the Logger 
formatting)

This following annotation adds little value

@Description("Details of Security Property")

I would either remove the annotation, or provide information that 
helps a user understand the event. For instance, what is X509, and 
what kind of certificates are we talking about?
Yes - that looks like the wrong Description. I'll review all of 
these fields.


@Category("Java Application")

I'm a bit worried that we will pollute the "Java Application" 
namespace if we add lots of JDK events in that category. We may 
want to add a new top level category "Java Development Kit", 
analogous to the "Java Virtual Machine" category, and put all 
security related events in subcategory, perhaps called "Security".

Yes - Open to suggestions. "Security" sounds like a good suggestion.


@Label("X509Cert")

The label should be human readable name, with spaces, title cased 
etc. I would recommend "X.509 Certificate". In general, avoid 
abbreviations like "certs&

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Xuelei Fan]

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+.collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the above 
code will create a new instance.  Is the return value of cce.isEnabled() 
dynamically changed or static?


Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.


Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't realized 
how much the handshaker code had changed. Hopefully, I'll get a review 
from security dev team on that front.


Regards the JFR semantics, I believe the edits should match majority of 
requests . I still have a preference for the test infra design for these 
new logger/JFR tests used in version 1 of webrev. I think it makes sense 
to keep the test functionality together - no sense in separating them 
out into different components IMO. Also, some of the edits to the JFR 
testing were made to test for the new dual log/record functionality. I 
might catch up with you tomorrow to see what the best arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and should 
be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose 
of the control attribute is so to provide parameterized configuration 
of events for JMC.  As it is now, the security events will be enabled 
when a user turns on the exception events.

Makes sense. I'll remove that parameter.


X509CertEvent:
You should use milliseconds since epoch to represent a date instead 
of a string value, i.e.


    @Label("Valid From")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validFrom;

    @Label("Valid Until")
    @Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
    public long validUntil;

The CertificateValidity class operates on Date Object values. I'll 
work with the Date.getTime() method then (and update the Logger 
formatting)

This following annotation adds little value

@Description("Details of Security Property")

I would either remove the annotation, or provide information that 
helps a user understand the event. For instance, what is X509, and 
what kind of certificates are we talking about?
Yes - that looks like the wrong Description. I'll review all of these 
fields.


@Category("Java Application")

I'm a bit worried that we will pollute the "Java Application" 
namespace if we add lots of JDK events in that category. We may want 
to add a new top level category "Java Development Kit", analogous to 
the "Java Virtual Machine" category, and put all security related 
events in subcategory, perhaps called "Security".

Yes - Open to suggestions. "Security" sounds like a good suggestion.


@Label("X509Cert")

The label should be human readable name, with spaces, title cased 
etc. I would recommend "X.509 Certificate". In general, avoid 
abbreviations like "certs" and instead use labels such as 
"Certificate Chain". The label should be short and not a full sentence.


For details see,
https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html

I think it would be good to separate testing of JFR and logging into 
different files / tests. I would prefer that the test name is the 
same as the event prefixed with "Test", i.e TestX509CertificateEvent, 
as this is the pattern used by other JFR tests.


I'll take a look at that pattern again. I've separated out the current 
tests into an (a) outer test to analyze the logger output and (b) the 
inner test which checks for JFR correctness. I did include extra logic 
to make sure that the EventHelper configuration was working correctly. 
"Events.assertField" looks very helpful. Thanks for the pointer.


Let me take on board the suggestions below and get a new webrev out on 
Tuesday.


regards,
Sean.


I reworked one of the tests to how I like to see it:

/*
 * @test
 * @key jfr
 * @library /test/lib
 * @run main/othervm jdk.jfr.event.security.TestX509CertificateEvent
 */
public class TestX509CertificateEvent {

    private static final String CERTIFICATE_1 = "...";
    private static final String CERTIFICATE_2 = "...";

    public static void main(String... args) throws 
CertificateException {


 Recording r = new Recording();
r.enable(EventNames.X509Certificate).withoutStackTrace();
 

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

I have no other comments.

Thanks,
Xuelei

On 3/20/2018 7:50 AM, Adam Petcher wrote:

Latest webrev: http://cr.openjdk.java.net/~apetcher/8181594/webrev.02/

Comments inline below.

In addition, I also changed the name of IntegerModuloP_Base to 
IntegerModuloP, and IntegerModuloP to ImmutableIntegerModuloP.



On 3/11/2018 12:04 PM, Xuelei Fan wrote:

On 2/26/2018 10:39 AM, Adam Petcher wrote:


On 2/23/2018 12:46 PM, Xuelei Fan wrote:


ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the 
future. Looks like only BigIntegerModuloP uses this classes.  I may 
prefer to define private methods for byte array swap in 
BigIntegerModuloP.


It is also used by XDHPublicKeyImpl (in the XDH code review). XDH 
public keys are represented as BigInteger, and I use the array 
reverse method to convert encoded keys to BigInteger.


If it is not widely used by other classes, please have these methods 
in the class where is get called.   The sun.security.util is exported 
to other modules as well, we may not want to add stuff into this 
package unless it is really necessary.


Okay. I put these methods in BigIntegerModuloP and removed the ArrayUtil 
class. This array reverse code will be duplicated where it is used by 
XDH public keys (in the other code review).






MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean 
parameter?


I couldn't come up with a way to implement this without branching 
when the swap parameter is boolean. See 
IntegerPolynomial.conditionalSwap to see how this is implemented in 
arithmetic with an int swap argument. If you (or anyone) can think of 
a way to do this with boolean, let me know.


I added a sentence to the comment above conditionalSwapWith that 
describes why it is an int instead of a boolean.


I did not get the point about the need to avoid branching.  Can you 
have more details?


The goal is to avoid things like if(secret){...}, in order to prevent 
the secrets from leaking into side channels (mostly timing and cache). 
The way this method is used by XDH, the swap parameter is a single bit 
of the private key. By storing this bit as an integer, and then doing 
the swap using only integer arithmetic, we can avoid branching which may 
leak the bits of the key.






Except the conditionalSwapWith() method, I did not get the points 
why we need a mutable version.  Would you please have more 
description of this requirement?


The comment above the class definition has this sentence:

"This interface can be used to improve performance and avoid the 
allocation of a large number of temporary objects."


Do you need more information than this in the comments? The 
performance motivation is so that a.add(b).multiply(c)... can be done 
without allocating a new buffer for each operation. For example, 
without mutable field elements, an X25519 point multiplication would 
allocate around 4,300 temporary arrays totaling 350,000 bytes. If I 
remember correctly, switching the X25519 implementation to mutable 
field elements reduced the point multiplication time by about half.



I see your point.  The benefits is obviously.

OK, why you need the immutable version then? Sounds like the mutable 
version interface is sufficient, including performance. If an 
immutable version is really needed, we can have the implementation 
making the decision.  Accordingly, the conditionalSwapWith() can be 
defined as optional method, if it is not required to be implemented in 
immutable implementation.


It's confusing to me that the immutable and mutable and the base 
versions/interfaces mixed together.  It would be nice if we can 
simplify the interface a little bit.


For internal APIs, sometimes we don't want the same quality level as 
public APIs.  I think this set of class will be widely used by new EC 
curves, ChaCha20/Poly1305, or more in the future.  It would be nice if 
we could do it good at the beginning.


The mutable version adds the conditional swap as well as mutable 
versions of many of the basic operations. The XDH implementation uses 
both the mutable and immutable versions. The immutable version allows me 
to simplify the client code because I don't have to worry about whether 
some value has been modified. For example, the XDH code keeps a 
representation of 0, 1, and the constant that defines the curve as 
immutable values.


So I prefer to have both. It complicates this API a bit, but it allows 
simpler and more robust code in the client of this API.







IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate 
as "(this + b) ^ 2 mod m". 


To be precise, it calculates "((this % p) + (b % p)) % 2^

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

On 2/26/2018 10:39 AM, Adam Petcher wrote:


http://cr.openjdk.java.net/~apetcher/8181594/webrev.01/

See inline below.

On 2/23/2018 12:46 PM, Xuelei Fan wrote:


ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the 
future. Looks like only BigIntegerModuloP uses this classes.  I may 
prefer to define private methods for byte array swap in 
BigIntegerModuloP.


It is also used by XDHPublicKeyImpl (in the XDH code review). XDH public 
keys are represented as BigInteger, and I use the array reverse method 
to convert encoded keys to BigInteger.


If it is not widely used by other classes, please have these methods in 
the class where is get called.   The sun.security.util is exported to 
other modules as well, we may not want to add stuff into this package 
unless it is really necessary.




MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean 
parameter?


I couldn't come up with a way to implement this without branching when 
the swap parameter is boolean. See IntegerPolynomial.conditionalSwap to 
see how this is implemented in arithmetic with an int swap argument. If 
you (or anyone) can think of a way to do this with boolean, let me know.


I added a sentence to the comment above conditionalSwapWith that 
describes why it is an int instead of a boolean.


I did not get the point about the need to avoid branching.  Can you have 
more details?




Except the conditionalSwapWith() method, I did not get the points why 
we need a mutable version.  Would you please have more description of 
this requirement?


The comment above the class definition has this sentence:

"This interface can be used to improve performance and avoid the 
allocation of a large number of temporary objects."


Do you need more information than this in the comments? The performance 
motivation is so that a.add(b).multiply(c)... can be done without 
allocating a new buffer for each operation. For example, without mutable 
field elements, an X25519 point multiplication would allocate around 
4,300 temporary arrays totaling 350,000 bytes. If I remember correctly, 
switching the X25519 implementation to mutable field elements reduced 
the point multiplication time by about half.



I see your point.  The benefits is obviously.

OK, why you need the immutable version then? Sounds like the mutable 
version interface is sufficient, including performance.  If an immutable 
version is really needed, we can have the implementation making the 
decision.  Accordingly, the conditionalSwapWith() can be defined as 
optional method, if it is not required to be implemented in immutable 
implementation.


It's confusing to me that the immutable and mutable and the base 
versions/interfaces mixed together.  It would be nice if we can simplify 
the interface a little bit.


For internal APIs, sometimes we don't want the same quality level as 
public APIs.  I think this set of class will be widely used by new EC 
curves, ChaCha20/Poly1305, or more in the future.  It would be nice if 
we could do it good at the beginning.





IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate 
as "(this + b) ^ 2 mod m". 


To be precise, it calculates "((this % p) + (b % p)) % 2^m" (where p is 
the prime that defines the field, and m is the desired length, in bits). 
Note that the addition here is normal integer addition (not addition in 
GF(p)).


This operation is not used in XDH, but it is used in Poly1305 to add the 
AES encryption of a nonce to a field element. So you can get more 
information about this operation by reading the Poly1305 paper/RFC.


I was not meant to say the function of the method.  I meant that the 
method name is a little bit misleading, not very straightforward to me.



Besides, what's the benefits of the two methods?  Could we just use:
  this.add(b).asByteArray()


No, because that would calculate "((this + b) mod p) mod 2^m". The value 
of (this + b) can be larger than p, so this would not produce the 
desired result.

 >>
I guess, but not very sure, it is for constant time calculation. If 
the function is required, could it be renamed as:


  // the result is inside of the size range
  IntegerModuloP addModSize(IntegerModuloP_Base b, int size)
Or
  // the result is wrapped if outside of the size range
  IntegerModuloP addOnWrap(IntegerModuloP_Base b, int size)

and the use may look like:
  this.addModSize(b, size).asByteArray()



Any attempt to perform the addition in IntegerModuloP and then pull out 
the byte array will not work.
Does it mean if I perform a addition, and cannot get the byte array in 
the follo

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Xuelei Fan]

Looks fine to me.  Thanks!

Xuelei

On 2/23/2018 11:39 AM, Roger Riggs wrote:
Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in the 
properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

    com/sun/crypto/provider/
    java/util/regex/
    jdk/internal/util/xml/impl/

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the future. 
Looks like only BigIntegerModuloP uses this classes.  I may prefer to 
define private methods for byte array swap in BigIntegerModuloP.



BigIntegerModuloP.java:
===
As this is a class for testing or ptototype purpose,  it might not be a 
part of JDK products, like JRE.  Would you mind move it to a test 
package if you want to keep it?



IntegerModuloP, IntegerModuloP_Base and MutableIntegerModuloP
=
In the security package/context, it may make sense to use 
"IntegerModulo" for the referring to "integers modulo a prime value". 
The class name of "IntegerModuloP_Base" is not a general Java coding 
style.  I may prefer a little bit name changes like:

 IntegerModuloP_Base   -> IntegerModulo
 IntegerModuloP-> ImmutableIntegerModulo
 MutableIntegerModuloP -> MutableIntegerModulo

 IntegerFieldModuloP   -> IntegerModuloField (?)


MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean parameter?

Except the conditionalSwapWith() method, I did not get the points why we 
need a mutable version.  Would you please have more description of this 
requirement?



IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate as 
"(this + b) ^ 2 mod m".  Besides, what's the benefits of the two 
methods?  Could we just use:

  this.add(b).asByteArray()

I guess, but not very sure, it is for constant time calculation.  If the 
function is required, could it be renamed as:


  // the result is inside of the size range
  IntegerModuloP addModSize(IntegerModuloP_Base b, int size)
Or
  // the result is wrapped if outside of the size range
  IntegerModuloP addOnWrap(IntegerModuloP_Base b, int size)

and the use may look like:
  this.addModSize(b, size).asByteArray()


Will review the rest when I understand more about the interfaces design.

Thanks,
Xuelei

On 1/30/2018 8:52 AM, Adam Petcher wrote:

+core-libs-dev


On 1/26/2018 4:06 PM, Adam Petcher wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8181594
Webrev: http://cr.openjdk.java.net/~apetcher/8181594/webrev.00/

This is a code review for the field arithmetic that will be used in 
implementations of X25519/X448 key agreement, the Poly1305 
authenticator, and EdDSA signatures. I believe that the library has 
all the features necessary for X25519/X448 and Poly1305, and I expect 
at most a couple of minor enhancements will be required to support 
EdDSA. There is no public API for this library, so we can change it in 
the future to suit the needs of new algorithms without breaking 
compatibility with external code. Still, I made an attempt to clearly 
structure and document the (internal) API, and I want to make sure it 
is understandable and easy to use.


This is not a general-purpose modular arithmetic library. It will only 
work well in circumstances where the sequence of operations is 
restricted, and where the prime that defines the field has some useful 
structure. Moreover, each new field will require some field-specific 
code that takes into account the structure of the prime and the way 
the field is used in the application. The initial implementation 
includes a field for Poly1305 and the fields for X25519/X448 which 
should also work for EdDSA.


The benefits of using this library are that it is much more efficient 
than using similar operations in BigInteger. Also, many operations are 
branch-free, making them suitable for use in a side-channel resistant 
implementation that does not branch on secrets.


To provide some context, I have attached a code snippet describing how 
this library can be used. The snippet is the constant-time Montgomery 
ladder from my X25519/X448 implementation, which I expect to be out 
for review soon. X25519/X448 only uses standard arithmetic operations, 
and the more unusual features (e.g. add modulo a power of 2) are 
needed by Poly1305.


The field arithmetic (for all fields) is implemented using a 32-bit 
representation similar to the one described in the Ed448 paper[1] (in 
the "Implementation on 32-bit platforms" section). Though my 
implementation uses signed limbs, and grade-school multiplication 
instead of Karatsuba. The argument for correctness is essentially the 
same for all three fields: the magnitude of each 64-bit limb is at 
most 2^(k-1) after reduction, except for the last limb which may have 
a magnitude of up to 2^k. The values of k are between 26 to 28 
(depending on the field), and we can calculate that the maximum 
magnitude for any limb during an add-multiply-carry-reduce 

Re: [10] RFR 8194666: ProblemList update for bugid associated with PreferredKey.java, ConcurrentHashMapTest and SSLSocketParametersTest.sh

[Xuelei Fan]

I'm not very sure of the fix problems of JDK-8176354.  But this 
changeset Looks fine to me.


Thanks,
Xuelei

On 1/4/2018 7:55 PM, Amy Lu wrote:
Please review this minor cleanup for test/jdk/ProblemList.txt on bugid 
that associated with tests.


bug: https://bugs.openjdk.java.net/browse/JDK-8194666
webrev: http://cr.openjdk.java.net/~amlu/8194666/webrev.00/

Thanks,
Amy

--- old/test/jdk/ProblemList.txt2018-01-05 11:41:00.0 +0800
+++ new/test/jdk/ProblemList.txt2018-01-05 11:41:00.0 +0800
@@ -264,7 +264,7 @@
  
  sun/security/krb5/auto/UnboundSSL.java  8180265 windows-all

  sun/security/provider/KeyStore/DKSTest.sh   8180266 
windows-all
-sun/security/ssl/X509KeyManager/PreferredKey.java   8176354 
generic-all
+sun/security/ssl/X509KeyManager/PreferredKey.java   8190333 
generic-all
  
  
  
@@ -363,12 +363,12 @@
  
  com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java   8169942 linux-i586,macosx-all,windows-x64
  
-javax/rmi/PortableRemoteObject/8146975/RmiIiopReturnValueTest.java 8169737 generic-all

+javax/rmi/PortableRemoteObject/8146975/RmiIiopReturnValueTest.java 8194663 
generic-all
   
-org/omg/CORBA/OrbPropertiesTest.java			8175177 generic-all

+org/omg/CORBA/OrbPropertiesTest.java   8194663 
generic-all
  
-javax/rmi/PortableRemoteObject/ConcurrentHashMapTest.java   8080643 generic-all

+javax/rmi/PortableRemoteObject/ConcurrentHashMapTest.java   8194663 
generic-all
  
-javax/rmi/ssl/SSLSocketParametersTest.sh8162906 generic-all

+javax/rmi/ssl/SSLSocketParametersTest.sh8194663 
generic-all
  
  

Re: 9 RFR of JDK-8176337: Mark several tests as intermittently failing

[Xuelei Fan]

> sun/security/tools/keytool/DefaultSignatureAlgorithm.java
> javax/net/ssl/DTLS/CipherSuite.java
The above two updates look fine to me.

Thanks,
Xuelei

On 3/7/2017 7:36 PM, Hamlin Li wrote:

Would you please review below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8176337

webrev: http://cr.openjdk.java.net/~mli/8176337/webrev.00/


These tests are failing intermittently, they should be marked
accordingly with
@key intermittent

java/nio/channels/FileChannel/Transfer.java (JDK-8140263
)
java/nio/channels/FileChannel/Transfers.java (JDK-8140263
)
java/io/FileInputStream/LargeFileAvailable.java (JDK-8174810
)
java/nio/channels/FileChannel/LoopingTruncate.java (JDK-8173386
)
java/lang/ProcessBuilder/Basic.java (JDK-8171426
)
sun/security/tools/keytool/DefaultSignatureAlgorithm.java (JDK-8172657
)
javax/net/ssl/DTLS/CipherSuite.java (JDK-8174072
)


Thank you

-Hamlin

Re: RFR 8170900: Issue with FilePermission::implies for wildcard flag(-)

[Xuelei Fan]

I think the note is an example, may not need an additional CCC.

For easier reading, I may use a contrast example.  For example, "Note 
that this means "/-" implies "/foo" but not "foo".".


Use the one you like, I'm OK with the either.

Xuelei

On 12/21/2016 3:58 PM, Wang Weijun wrote:



On Dec 22, 2016, at 4:39 AM, Xuelei Fan <xuelei@oracle.com> wrote:

I'm trying to understand this update.  Does "/-" imply "/foo"?


Yes.



Does the following spec can be used to explain the new added note?

* if the wildcard flag is "-", the simple pathname's path
* must be recursively inside the wildcard pathname's path.


Yes.

But the precise meaning of "recursively inside" is different between the 
pre-jdk9 and jdk9 behaviors. The @implNote explains more.

--Max



Xuelei

On 12/19/2016 11:25 PM, Wang Weijun wrote:

Ping again.


On Dec 14, 2016, at 1:53 PM, Wang Weijun <weijun.w...@oracle.com> wrote:

An clarification is added to FilePermission::implies:

* @implNote
  
* a simple {@code npath} is recursively inside a wildcard {@code npath}
* if and only if {@code simple_npath.relativize(wildcard_npath)}
- * is a series of one or more "..". An invalid {@code FilePermission} does
+ * is a series of one or more "..". Note that this means "/-" does not
+ * imply "foo". An invalid {@code FilePermission} does
* not imply any object except for itself.

The newly added sentence is

Note that this means "/-" does not imply "foo".

JCK has agreed to update their test.

Since this is just a clarification inside an @implNote and no spec is updated, 
I suppose no CCC is needed. Please confirm.

Thanks
Max

Re: RFR 8170900: Issue with FilePermission::implies for wildcard flag(-)

[Xuelei Fan]

I'm trying to understand this update.  Does "/-" imply "/foo"?

Does the following spec can be used to explain the new added note?

 * if the wildcard flag is "-", the simple pathname's path
 * must be recursively inside the wildcard pathname's path.

Xuelei

On 12/19/2016 11:25 PM, Wang Weijun wrote:

Ping again.


On Dec 14, 2016, at 1:53 PM, Wang Weijun  wrote:

An clarification is added to FilePermission::implies:

 * @implNote
   
 * a simple {@code npath} is recursively inside a wildcard {@code npath}
 * if and only if {@code simple_npath.relativize(wildcard_npath)}
- * is a series of one or more "..". An invalid {@code FilePermission} does
+ * is a series of one or more "..". Note that this means "/-" does not
+ * imply "foo". An invalid {@code FilePermission} does
 * not imply any object except for itself.

The newly added sentence is

 Note that this means "/-" does not imply "foo".

JCK has agreed to update their test.

Since this is just a clarification inside an @implNote and no spec is updated, 
I suppose no CCC is needed. Please confirm.

Thanks
Max

Re: RFR 8168979: @implNote for invalid FilePermission

[Xuelei Fan]

On 12/13/2016 5:45 PM, Wang Weijun wrote:

A major behavior change is that <> now implies an invalid 
permission, I hope this is good to minimize incompatibility.
Looks like two sides of the same coin.  If there is an invalid > (not existing in practice, I think), it now implies all;  if no 
change on this point, <> does not imply invalid one.  The 
existing behavior looks more safe to me.  What's you concern to make 
this behavior change?


Otherwise, looks fine to me.

Xuelei

Re: RFR 9: 8169416: SSLSessionImpl finalize overhead

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

> On 23 Nov 2016, at 5:41 AM, Roger Riggs  wrote:
> 
> Adding security-dev...
> 
> Please review this change to remove an ineffective finalizer for SSLSessions.
> The finalizer removes bindings from the SSLSession of a table that is also
> freed when the SSLSession is being freed. There is no point in removing them 
> explicitly.
> It only delays freeing memory and increases the overhead due to the tracking
> of finalizable objects.  Details are in the issue.
> 
> Webrev:
>  http://cr.openjdk.java.net/~rriggs/webrev-ssl-finalize-8169416
> 
> Issue:
>  https://bugs.openjdk.java.net/browse/JDK-8169416
> 
> Thanks, Roger
>> 
> 

Re: RFR: 8154304: NullpointerException at LdapReferralException.getReferralContext

[Xuelei Fan]

Looks nice to me.  It would be nice to update the copyright date, too.

Thanks,
Xuelei

On 4/15/2016 10:13 PM, Seán Coffey wrote:
> I need to correct another issue related to JDK-8149450. If a
> getReferralContext call is made on a ReferralContext that doesn't
> contain any referrals (URI fields) then a null scenario is possible.
> There's a question mark around how compliant the LDAP BER response is
> but I'd like the JDK to handle the NPE and communicate back by throwing
> NamingException :
> 
> bug ID :https://bugs.openjdk.java.net/browse/JDK-8154304
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8154304.jdk9/webrev/
> 
> -- 
> Regards,
> Sean.
> 

Re: RFR: 8149450: LdapCtx.processReturnCode() throwing Null Pointer Exception

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

On 4/10/2016 9:41 PM, Sean Coffey wrote:
> Looking to fix this issue. Better checks for the referrrals field.
> 
> bugID : https://bugs.openjdk.java.net/browse/JDK-8149450
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8149450.jdk9/webrev/
> 
> regards,
> Sean.

Re: Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

Thanks!

Xuelei

On 3/23/2016 9:44 PM, Wang Weijun wrote:
> 
>> On Mar 23, 2016, at 7:23 PM, Xuelei Fan <xuelei@oracle.com> wrote:
>>
>> On 3/23/2016 5:44 PM, Wang Weijun wrote:
>>> Then why not fix the 2 bugs in a single changeset?
>>>
>> Both need spec update approval.  As they are completely different spec
>> update, better to update in 2 enhancements.
>>
>> As you have concerns here, I removed ObjectIdentifier.java from this
>> update.  See the new webrev:
>>
>>   http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/
> 
> Looks good to me.
> 
> --Max
> 
>>
>> Xuelei
> 

Re: Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

Hi Attila,

Good catch about the comparing.  I updated the code in my local
workspace, and I would ask for code review in another enhancement update
soon.

Thanks,
Xuelei

On 3/23/2016 9:15 PM, Attila Szegedi wrote:
> Sorry for beating the dead horse of ObjectIdentifier.java change, but
> I’d suggest that if that code is later revisited, it be changed to
> "first.compareTo(BigInteger.TWO) > 0" instead of “… == 1”.
> 
> Comparing the return value of compareTo to zero (instead of relying on
> specific set of return values) is the “suggested idiom for performing
> these comparisons" as per BigInteger JavaDoc[1] and consistent with the
> contract of Comparable.compareTo (even though same BigInteger JavaDoc
> also explicitly specifies that the return values in this particular case
> are indeed -1, 0, and 1). 
> 
> Attila.
> 
> [1] 
> http://docs.oracle.com/javase/8/docs/api/java/math/BigInteger.html#compareTo-java.math.BigInteger-
> 
>> On 23 Mar 2016, at 12:23, Xuelei Fan <xuelei@oracle.com
>> <mailto:xuelei@oracle.com>> wrote:
>>
>> On 3/23/2016 5:44 PM, Wang Weijun wrote:
>>> Then why not fix the 2 bugs in a single changeset?
>>>
>> Both need spec update approval.  As they are completely different spec
>> update, better to update in 2 enhancements.
>>
>> As you have concerns here, I removed ObjectIdentifier.java from this
>> update.  See the new webrev:
>>
>>   http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/
>>
>> Xuelei
>>
>>> --Max
>>>
>>>> 在 2016年3月23日，17:06，Xuelei Fan <xuelei@oracle.com
>>>> <mailto:xuelei@oracle.com>> 写道：
>>>>
>>>>> On 3/23/2016 3:34 PM, Wang Weijun wrote:
>>>>>
>>>>>> On Mar 23, 2016, at 12:48 PM, Xuelei Fan <xuelei@oracle.com
>>>>>> <mailto:xuelei@oracle.com>> wrote:
>>>>>>
>>>>>> On 3/23/2016 12:10 PM, Wang Weijun wrote:
>>>>>>> Only 3 files touched. Are you going to make the
>>>>>>> s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files
>>>>>>> with another bug fix?
>>>>>> There are also uses in security components.  I will make the
>>>>>> update in another bug.
>>>>>
>>>>> I see. But why is ObjectIdentifier.java included in this fix?
>>>> It happens that the other bug touch those files, but
>>>> ObjectIdentifier.java is not related to that bug.
>>>>
>>>> Does it make sense?
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>>> In you only keep BigInteger and BigDecimal, then I have no other
>>>>> comment.
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>>
>>>>>>> Thanks
>>>>>>> Max
>>>>>>>
>>>>>>>> On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei@oracle.com
>>>>>>>> <mailto:xuelei@oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the update for the supporting of BigInteger.TWO:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~xuelei/8152237/webrev/
>>>>>>>>
>>>>>>>> BigInteger.valueOf(2) is a common BigInteger value used in
>>>>>>>> binary and cryptography operation calculation.  The
>>>>>>>> BigInteger.TWO is not exported, and hence BigInteger.valueOf(2)
>>>>>>>> is used instead in applications and JDK components.  The export
>>>>>>>> of static BigInteger.TWO can improve performance and simplify
>>>>>>>> existing code.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Xuelei
>>>>
>>>
>>
> 

Re: Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

On 3/23/2016 5:44 PM, Wang Weijun wrote:
> Then why not fix the 2 bugs in a single changeset?
> 
Both need spec update approval.  As they are completely different spec
update, better to update in 2 enhancements.

As you have concerns here, I removed ObjectIdentifier.java from this
update.  See the new webrev:

   http://cr.openjdk.java.net/~xuelei/8152237/webrev.01/

Xuelei

> --Max
> 
>> 在 2016年3月23日，17:06，Xuelei Fan <xuelei@oracle.com> 写道：
>>
>>> On 3/23/2016 3:34 PM, Wang Weijun wrote:
>>>
>>>> On Mar 23, 2016, at 12:48 PM, Xuelei Fan <xuelei@oracle.com> wrote:
>>>>
>>>> On 3/23/2016 12:10 PM, Wang Weijun wrote:
>>>>> Only 3 files touched. Are you going to make the 
>>>>> s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with 
>>>>> another bug fix?
>>>> There are also uses in security components.  I will make the update in 
>>>> another bug.
>>>
>>> I see. But why is ObjectIdentifier.java included in this fix?
>> It happens that the other bug touch those files, but
>> ObjectIdentifier.java is not related to that bug.
>>
>> Does it make sense?
>>
>> Thanks,
>> Xuelei
>>
>>> In you only keep BigInteger and BigDecimal, then I have no other comment.
>>>
>>> Thanks
>>> Max
>>>
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>> On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei@oracle.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please review the update for the supporting of BigInteger.TWO:
>>>>>>
>>>>>>  http://cr.openjdk.java.net/~xuelei/8152237/webrev/
>>>>>>
>>>>>> BigInteger.valueOf(2) is a common BigInteger value used in binary and 
>>>>>> cryptography operation calculation.  The BigInteger.TWO is not exported, 
>>>>>> and hence BigInteger.valueOf(2) is used instead in applications and JDK 
>>>>>> components.  The export of static BigInteger.TWO can improve performance 
>>>>>> and simplify existing code.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>
> 

Re: Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

On 3/23/2016 3:34 PM, Wang Weijun wrote:
> 
>> On Mar 23, 2016, at 12:48 PM, Xuelei Fan <xuelei@oracle.com> wrote:
>>
>> On 3/23/2016 12:10 PM, Wang Weijun wrote:
>>> Only 3 files touched. Are you going to make the 
>>> s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another 
>>> bug fix?
>>>
>> There are also uses in security components.  I will make the update in 
>> another bug.
> 
> I see. But why is ObjectIdentifier.java included in this fix?
> 
It happens that the other bug touch those files, but
ObjectIdentifier.java is not related to that bug.

Does it make sense?

Thanks,
Xuelei

> In you only keep BigInteger and BigDecimal, then I have no other comment.
> 
> Thanks
> Max
> 
>>
>> Thanks,
>> Xuelei
>>
>>> Thanks
>>> Max
>>>
>>>> On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei@oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Please review the update for the supporting of BigInteger.TWO:
>>>>
>>>>   http://cr.openjdk.java.net/~xuelei/8152237/webrev/
>>>>
>>>> BigInteger.valueOf(2) is a common BigInteger value used in binary and 
>>>> cryptography operation calculation.  The BigInteger.TWO is not exported, 
>>>> and hence BigInteger.valueOf(2) is used instead in applications and JDK 
>>>> components.  The export of static BigInteger.TWO can improve performance 
>>>> and simplify existing code.
>>>>
>>>> Thanks,
>>>> Xuelei
>>>
>>
> 

Re: Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

On 3/23/2016 12:10 PM, Wang Weijun wrote:

Only 3 files touched. Are you going to make the 
s/BigInteger.valueOf(2)/BigInteger.TWO/ changes in other files with another bug 
fix?

There are also uses in security components.  I will make the update in 
another bug.


Thanks,
Xuelei


Thanks
Max


On Mar 23, 2016, at 11:26 AM, Xuelei Fan <xuelei@oracle.com> wrote:

Hi,

Please review the update for the supporting of BigInteger.TWO:

   http://cr.openjdk.java.net/~xuelei/8152237/webrev/

BigInteger.valueOf(2) is a common BigInteger value used in binary and 
cryptography operation calculation.  The BigInteger.TWO is not exported, and 
hence BigInteger.valueOf(2) is used instead in applications and JDK components. 
 The export of static BigInteger.TWO can improve performance and simplify 
existing code.

Thanks,
Xuelei

Code Review Request, 8152237 Support BigInteger.TWO

[Xuelei Fan]

Hi,

Please review the update for the supporting of BigInteger.TWO:

   http://cr.openjdk.java.net/~xuelei/8152237/webrev/

BigInteger.valueOf(2) is a common BigInteger value used in binary and 
cryptography operation calculation.  The BigInteger.TWO is not exported, 
and hence BigInteger.valueOf(2) is used instead in applications and JDK 
components.  The export of static BigInteger.TWO can improve performance 
and simplify existing code.


Thanks,
Xuelei

Re: [9] Review request for 8085979: Make some DTLS feature functional tests work also for TLS protocol

[Xuelei Fan]

On 6/9/2015 11:31 PM, Konstantin Shefov wrote:
 Xuelei, thanks for reviewing
 
 typo is corrected:
 http://cr.openjdk.java.net/~kshefov/8085979/webrev.01/
 
Why there are old UnSupportedCiphersTest.java files?  If you have not
committed the changeset, you can hg forget to undo a previous hg
add.  As would avoid the unnecessary old file logs.

Xuelei

 -Konstantin
 
 On 06/09/2015 06:07 PM, Xuelei Fan wrote:
 On 6/9/2015 10:57 PM, Xuelei Fan wrote:
 Looks fine to me.  Nice port to TLS protocols.

 A very minior comment about the class name.
 TLSUnSupportedCiphersTest.java:
 replease UnSupported with Unsupported.

 typo: replace UnSupported with Unsupported.

 Thanks,
 Xuelei

 On 6/8/2015 11:04 PM, Konstantin Shefov wrote:
 Hello,

 Please review distribution of some DTLS feature tests to TLS protocol.
 Some DTLS tests may also be used to test the same functionality in TLS
 protocol and its versions.
 It is test only improvement.

 bug: https://bugs.openjdk.java.net/browse/JDK-8085979
 webrev: http://cr.openjdk.java.net/~kshefov/8085979/webrev.00/


 Thanks
 -Konstantin
 

Re: [9] Review request for 8085979: Make some DTLS feature functional tests work also for TLS protocol

[Xuelei Fan]

On 6/9/2015 10:57 PM, Xuelei Fan wrote:
 Looks fine to me.  Nice port to TLS protocols.
 
 A very minior comment about the class name.
 TLSUnSupportedCiphersTest.java:
 replease UnSupported with Unsupported.
 
typo: replace UnSupported with Unsupported.

 Thanks,
 Xuelei
 
 On 6/8/2015 11:04 PM, Konstantin Shefov wrote:
 Hello,

 Please review distribution of some DTLS feature tests to TLS protocol.
 Some DTLS tests may also be used to test the same functionality in TLS
 protocol and its versions.
 It is test only improvement.

 bug: https://bugs.openjdk.java.net/browse/JDK-8085979
 webrev: http://cr.openjdk.java.net/~kshefov/8085979/webrev.00/


 Thanks
 -Konstantin
 

Re: [9] Review request for 8085979: Make some DTLS feature functional tests work also for TLS protocol

[Xuelei Fan]

On 6/9/2015 11:47 PM, Xuelei Fan wrote:
 On 6/9/2015 11:31 PM, Konstantin Shefov wrote:
 Xuelei, thanks for reviewing

 typo is corrected:
 http://cr.openjdk.java.net/~kshefov/8085979/webrev.01/

 Why there are old UnSupportedCiphersTest.java files?  If you have not
 committed the changeset, you can hg forget to undo a previous hg
 add.  As would avoid the unnecessary old file logs.
 
Ooops, my fault.  The old files are the previous DTLS tests.  It's OK.
Thanks for the update.

Thanks,
Xuelei

 Xuelei
 
 -Konstantin

 On 06/09/2015 06:07 PM, Xuelei Fan wrote:
 On 6/9/2015 10:57 PM, Xuelei Fan wrote:
 Looks fine to me.  Nice port to TLS protocols.

 A very minior comment about the class name.
 TLSUnSupportedCiphersTest.java:
 replease UnSupported with Unsupported.

 typo: replace UnSupported with Unsupported.

 Thanks,
 Xuelei

 On 6/8/2015 11:04 PM, Konstantin Shefov wrote:
 Hello,

 Please review distribution of some DTLS feature tests to TLS protocol.
 Some DTLS tests may also be used to test the same functionality in TLS
 protocol and its versions.
 It is test only improvement.

 bug: https://bugs.openjdk.java.net/browse/JDK-8085979
 webrev: http://cr.openjdk.java.net/~kshefov/8085979/webrev.00/


 Thanks
 -Konstantin

 

Re: [9] Review request for 8072515: Test Task: Develop new tests for JEP 219: Datagram Transport Layer Security (DTLS)

[Xuelei Fan]

Looks fine to me.

It's nice to keep each line not exceed 80 characters.  For example

-  * @run main/othervm -Dtest.security.protocol=DTLS -Dtest.mode=norm
DTLSBufferOverflowUnderflowTest
+  * @run main/othervm -Dtest.security.protocol=DTLS
+  * -Dtest.mode=norm DTLSBufferOverflowUnderflowTest

Thanks,
Xuelei

On 6/2/2015 8:15 PM, Konstantin Shefov wrote:
 Hello,
 
 Please review new tests fro DTLS feature for JDK 9:
 
 bug: https://bugs.openjdk.java.net/browse/JDK-8072515
 webrev: http://cr.openjdk.java.net/~kshefov/8072515/webrev.00/
 
 
 Thanks
 -Konstantin
 

Re: Swing Dev Replace concat String to append in StringBuilder parameters

[Xuelei Fan]

Pavel, thanks for the checking and confirm.

Look back to the benchmark code:

private StringBuilder createBuilder(String... values) {
StringBuilder text = new StringBuilder();
text.append(values[0]).append(values[1])
.append(values[2]).append(values[3])
.append(values[4]).append(values[5]);
return text;
}

private StringBuilder createBuilderWithConcat(String... values) {
StringBuilder text = new StringBuilder();
text.append(values[0] + values[1])
.append(values[2] + values[3])
.append(values[4]+ values[5]);
return text;
}

Comparing to createBuilder, the code similar to (values[0] + values[1])
in createBuilderWithConcat may take additional memory and CPU cycles to
create new StringBuffer objects. As may be able to explain the benchmark
result.

Therefore, I think it might not be necessary to make this change
(replacing string + operator with StringBuilder append()).

Xuelei

On 8/26/2014 4:41 PM, Pavel Rappo wrote:
 It's exactly the way it's been working since 1.6 I believe.
 
 source
 
 public class Optimization {
 
 public String concat(String... strings) {
 return #:  + strings[0] + strings[2] + strings[3] + ...;
 }
 }
 
 compiled into
 
 public class Optimization {
   public Optimization();
 Code:
0: aload_0
1: invokespecial #1  // Method 
 java/lang/Object.init:()V
4: return
 
   public java.lang.String concat(java.lang.String...);
 Code:
0: new   #2  // class java/lang/StringBuilder
3: dup
4: invokespecial #3  // Method 
 java/lang/StringBuilder.init:()V
7: ldc   #4  // String #:
9: invokevirtual #5  // Method 
 java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
   12: aload_1
   13: iconst_0
   14: aaload
   15: invokevirtual #5  // Method 
 java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
   18: aload_1
   19: iconst_2
   20: aaload
   21: invokevirtual #5  // Method 
 java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
   24: aload_1
   25: iconst_3
   26: aaload
   27: invokevirtual #5  // Method 
 java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
   30: ldc   #6  // String ...
   32: invokevirtual #5  // Method 
 java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
   35: invokevirtual #7  // Method 
 java/lang/StringBuilder.toString:()Ljava/lang/String;
   38: areturn
 }
 
 -Pavel
 
 On 26 Aug 2014, at 06:20, Xuelei Fan xuelei@oracle.com wrote:
 
 I was wondering, is it nice to address it in Java compiler to use string
 builder for the string + operator?

 Xuelei


 On 8/26/2014 11:28 AM, Wang Weijun wrote:
 New webrevs available at

  http://cr.openjdk.java.net/~weijun/8055723/client/webrev.01/
  http://cr.openjdk.java.net/~weijun/8055723/core/webrev.01/  

 There are only 2 now. Everything non-client is in core.

 Everyone, please do code review quickly because the patch touches too many 
 files and any delay could mean re-merge.

 *Otávio*: If there is only small change in feedback, tell me to update my 
 own repo and you don't need to generate the big patch again.

 I see you still include that demo file.

 Thanks
 Max



 

Re: Swing Dev Replace concat String to append in StringBuilder parameters

[Xuelei Fan]

I was wondering, is it nice to address it in Java compiler to use string
builder for the string + operator?

Xuelei


On 8/26/2014 11:28 AM, Wang Weijun wrote:
 New webrevs available at
 
   http://cr.openjdk.java.net/~weijun/8055723/client/webrev.01/
   http://cr.openjdk.java.net/~weijun/8055723/core/webrev.01/  
 
 There are only 2 now. Everything non-client is in core.
 
 Everyone, please do code review quickly because the patch touches too many 
 files and any delay could mean re-merge.
 
 *Otávio*: If there is only small change in feedback, tell me to update my own 
 repo and you don't need to generate the big patch again.
 
 I see you still include that demo file.
 
 Thanks
 Max
 
 

Re: JDK-8041679 Replace uses of StringBuffer with StringBuilder within the JDK

[Xuelei Fan]

 - security

http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-security/webrev/

Looks fine to me.  Thanks for making this update.

Xuelei

On 5/12/2014 6:03 PM, Paul Sandoz wrote:
 Hi,
 
 This is a request for review of Otavio's patch replacing StringBuffer
 with StringBuilder within OpenJDK. (I also need to review it.)
 
 It covers many areas and i have grouped the patches into such areas to
 aid reviewing. When commenting please including core-libs.
 
 Jtreg tests showed no regressions, but when reviewing we need to be
 mindful of the context e.g. if the buffer escapes and can cross thread
 boundaries. 
 
 This is also an experiment to see if we can review the whole thing under
 one bug :-) if things prove problematic and slow we can split it
 out. Many files are touched but there are not many changes to each file
 and changes are very formulaic.
 
 I have also included ASM related changes, but i suspect we may have to
 leave those alone since such changes will make it more difficult to pull
 in ASM from upstream.
  
 -
 
 Otavio, for the record can you reply to this thread posting your single
 (uber) patch as textual attachment? (IIUC such attachments should now
 be supported by the email server).
 
 Paul.
 
 - core
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-core/webrev/
 
 - io
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-io/webrev/
 
 - management
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-management/webrev/
 
 - rmi
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-rmi/webrev/
 
 - security
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-security/webrev/
 
 
 - tools
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-tools/webrev/
 
 
 - graphics/media
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-media/webrev/
 
 
 - asm
 http://cr.openjdk.java.net/~psandoz/jdk9/sb/JDK-8041679-buffer-to-builder-asm/webrev/

Re: JDK 9 RFR of JDK-8027063 SecurityManger.getClassContext returns a raw type

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

On 1/7/2014 4:53 AM, Joe Darcy wrote:

Hello,

Please review the simple change to fix JDK-8027063
SecurityManger.getClassContext returns a raw type, which changes a
signature of a protected method in SecurityManger to remove a use of raw
types in the core libraries:

--- a/src/share/classes/java/lang/SecurityManager.javaMon Jan 06
11:48:32 2014 -0800
+++ b/src/share/classes/java/lang/SecurityManager.javaMon Jan 06
12:51:52 2014 -0800
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1995, 2011, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1995, 2014, Oracle and/or its affiliates. All rights
reserved.
   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or modify it
@@ -307,7 +307,7 @@
   *
   * @return  the execution stack.
   */
-protected native Class[] getClassContext();
+protected native Class?[] getClassContext();

  /**
   * Returns the class loader of the most recently executing method
from

A clean build succeeds and the SecurityManager tests pass after this
change.

Thanks,

-Joe

hg: jdk8/tl/jdk: 7093640: Enable client-side TLS 1.2 by default

[xuelei . fan]

Changeset: 8d35f0985dd7
Author:xuelei
Date:  2013-12-18 16:46 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8d35f0985dd7

7093640: Enable client-side TLS 1.2 by default
Reviewed-by: weijun, mullan, wetmore

! src/share/classes/sun/security/ssl/ProtocolVersion.java
! src/share/classes/sun/security/ssl/SSLContextImpl.java
! src/share/classes/sun/security/ssl/SunJSSE.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/CustomizedDefaultProtocols.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/DefaultEnabledProtocols.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/IllegalProtocolProperty.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/NoOldVersionContext.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/SSLContextVersion.java
- test/sun/security/ssl/javax/net/ssl/SSLContextVersion.java

hg: jdk8/tl/jdk: 8014266: regression test AsyncSSLSocketClose.java time out.

[xuelei . fan]

Changeset: 40d0ccd00f87
Author:xuelei
Date:  2013-11-14 16:08 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/40d0ccd00f87

8014266: regression test AsyncSSLSocketClose.java time out.
Reviewed-by: xuelei
Contributed-by: Rajan Halade rajan.hal...@oracle.com

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/AsyncSSLSocketClose.java

hg: jdk8/tl/jdk: 8023147: Test DisabledShortRSAKeys.java intermittent failed

[xuelei . fan]

Changeset: 1158d504e39e
Author:xuelei
Date:  2013-11-13 01:14 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1158d504e39e

8023147: Test DisabledShortRSAKeys.java intermittent failed
Reviewed-by: mullan

! test/sun/security/ssl/javax/net/ssl/TLSv12/DisabledShortRSAKeys.java

hg: jdk8/tl/jdk: 8026119: Regression test DHEKeySizing.java failing intermittently

[xuelei . fan]

Changeset: fb202a8e83c9
Author:xuelei
Date:  2013-10-13 21:10 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fb202a8e83c9

8026119: Regression test DHEKeySizing.java failing intermittently
Reviewed-by: weijun

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java

hg: jdk8/tl/jdk: 6956398: make ephemeral DH key match the length of the certificate key

[xuelei . fan]

Changeset: 0d5f4f1782e8
Author:xuelei
Date:  2013-10-07 18:46 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0d5f4f1782e8

6956398: make ephemeral DH key match the length of the certificate key
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/ServerHandshaker.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/DHKeyExchange/DHEKeySizing.java

hg: jdk8/tl/jdk: 8025123: SNI support in Kerberos cipher suites

[xuelei . fan]

Changeset: 3fca37c636be
Author:xuelei
Date:  2013-10-01 20:25 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3fca37c636be

8025123: SNI support in Kerberos cipher suites
Reviewed-by: weijun, xuelei
Contributed-by: Artem Smotrakov artem.smotra...@oracle.com

! src/share/classes/sun/security/ssl/ClientHandshaker.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/KerberosClientKeyExchange.java
! src/share/classes/sun/security/ssl/krb5/KerberosClientKeyExchangeImpl.java
! test/sun/security/krb5/auto/SSL.java

hg: jdk8/tl/jdk: 8024501: sun.security.mscapi.Key has no definition of serialVersionUID

[xuelei . fan]

Changeset: c9083205e6eb
Author:xuelei
Date:  2013-09-10 21:31 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c9083205e6eb

8024501: sun.security.mscapi.Key has no definition of serialVersionUID
Reviewed-by: weijun

! src/windows/classes/sun/security/mscapi/Key.java

hg: jdk8/tl/jdk: 8024444: Change to use othervm mode of tests in SSLEngineImpl

[xuelei . fan]

Changeset: f80b8af9c218
Author:xuelei
Date:  2013-09-09 19:07 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f80b8af9c218

802: Change to use othervm mode of tests in SSLEngineImpl
Reviewed-by: mullan

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseEngineException.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseInboundException.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/CloseStart.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/DelegatedTaskWrongException.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/EmptyExtensionData.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/EngineEnforceUseClientMode.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/RehandshakeFinished.java

hg: jdk8/tl/jdk: 7188657: There should be a way to reorder the JSSE ciphers

[xuelei . fan]

Changeset: 0f47f9f622d9
Author:xuelei
Date:  2013-09-07 17:05 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0f47f9f622d9

7188657: There should be a way to reorder the JSSE ciphers
Reviewed-by: weijun, wetmore

! src/share/classes/javax/net/ssl/SSLParameters.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
! src/share/classes/sun/security/ssl/SSLServerSocketImpl.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
+ test/sun/security/ssl/javax/net/ssl/SSLParameters/UseCipherSuitesOrder.java

hg: jdk8/tl/jdk: 8024068: sun/security/ssl/javax/net/ssl/ServerName/IllegalSNIName.java fails

[xuelei . fan]

Changeset: ead6babac5a9
Author:xuelei
Date:  2013-09-01 20:00 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ead6babac5a9

8024068: sun/security/ssl/javax/net/ssl/ServerName/IllegalSNIName.java fails
Reviewed-by: weijun

! test/sun/security/ssl/javax/net/ssl/ServerName/IllegalSNIName.java

hg: jdk8/tl/jdk: 8023881: IDN.USE_STD3_ASCII_RULES option is too strict to use Unicode in IDN.toASCII

[xuelei . fan]

Changeset: cdf68747b0fb
Author:xuelei
Date:  2013-08-29 18:58 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cdf68747b0fb

8023881: IDN.USE_STD3_ASCII_RULES option is too strict to use Unicode in 
IDN.toASCII
Reviewed-by: michaelm

! src/share/classes/java/net/IDN.java
+ test/java/net/IDN/UseSTD3ASCIIRules.java

hg: jdk8/tl/jdk: 8020842: IDN do not throw IAE when hostname ends with a trailing dot

[xuelei . fan]

Changeset: 096e7c665857
Author:xuelei
Date:  2013-08-19 17:42 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/096e7c665857

8020842: IDN do not throw IAE when hostname ends with a trailing dot
Reviewed-by: weijun, michaelm

! src/share/classes/java/net/IDN.java
+ test/java/net/IDN/IllegalArg.java
+ test/sun/security/ssl/javax/net/ssl/ServerName/IllegalSNIName.java

hg: jdk8/tl/jdk: 8023230: The impl of KerberosClientKeyExchange maybe not exist

[xuelei . fan]

Changeset: 21a25911f7f7
Author:xuelei
Date:  2013-08-19 18:49 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/21a25911f7f7

8023230: The impl of KerberosClientKeyExchange maybe not exist
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/KerberosClientKeyExchange.java

hg: jdk8/tl/jdk: 8022487: DEREncodedKeyValue.supportedKeyTypes should be private

[xuelei . fan]

Changeset: ea4f4164422e
Author:xuelei
Date:  2013-08-11 18:21 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ea4f4164422e

8022487: DEREncodedKeyValue.supportedKeyTypes should be private
Reviewed-by: mullan

! 
src/share/classes/com/sun/org/apache/xml/internal/security/keys/content/DEREncodedKeyValue.java

hg: jdk8/tl/jdk: 8013809: deadlock in SSLSocketImpl between between write and close

[xuelei . fan]

Changeset: 8c7cf4926157
Author:xuelei
Date:  2013-08-07 06:42 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8c7cf4926157

8013809: deadlock in SSLSocketImpl between between write and close
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/SSLSocketImpl.java

hg: jdk8/tl/jdk: 7127524: P11TlsPrfGenerator has anonymous inner class with serialVersionUID

[xuelei . fan]

Changeset: d6de149b9f20
Author:xuelei
Date:  2013-08-01 07:34 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d6de149b9f20

7127524: P11TlsPrfGenerator has anonymous inner class with serialVersionUID
Reviewed-by: vinnie

! src/share/classes/sun/security/pkcs11/P11TlsPrfGenerator.java

hg: jdk8/tl/jdk: 8021841: Remove SSLEngineDeadlock.java from problem list

[xuelei . fan]

Changeset: 613cc7beba64
Author:xuelei
Date:  2013-07-29 19:36 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/613cc7beba64

8021841: Remove SSLEngineDeadlock.java from problem list
Reviewed-by: wetmore

! test/ProblemList.txt

hg: jdk8/tl/jdk: 8019359: To comment why not use no_renegotiation to reject client initiated renegotiation

[xuelei . fan]

Changeset: 60d1994f63f7
Author:xuelei
Date:  2013-06-27 19:22 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/60d1994f63f7

8019359: To comment why not use no_renegotiation to reject client initiated 
renegotiation
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/ServerHandshaker.java

hg: jdk8/tl/jdk: 8017157: catch more exception in test RejectClientRenego

[xuelei . fan]

Changeset: a44bd993ce93
Author:xuelei
Date:  2013-06-20 07:48 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a44bd993ce93

8017157: catch more exception in test RejectClientRenego
Reviewed-by: vinnie

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/RejectClientRenego.java

hg: jdk8/tl/jdk: 7188658: Add possibility to disable client initiated renegotiation

[xuelei . fan]

Changeset: a76858faad59
Author:xuelei
Date:  2013-06-19 02:33 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a76858faad59

7188658: Add possibility to disable client initiated renegotiation
Reviewed-by: weijun, wetmore

! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/NoImpactServerRenego.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLSocketImpl/RejectClientRenego.java

hg: jdk8/tl/jdk: 8000456: Add programmatic deadlock detection in SSLEngineDeadlock

[xuelei . fan]

Changeset: 2d9da733014f
Author:xuelei
Date:  2013-06-18 18:50 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2d9da733014f

8000456: Add programmatic deadlock detection in SSLEngineDeadlock
Reviewed-by: wetmore

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineDeadlock.java

hg: jdk8/tl/jdk: 8014618: Need to strip leading zeros in TlsPremasterSecret of DHKeyAgreement

[xuelei . fan]

Changeset: 6407106f1b1c
Author:xuelei
Date:  2013-05-30 22:02 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6407106f1b1c

8014618: Need to strip leading zeros in TlsPremasterSecret of DHKeyAgreement
Reviewed-by: xuelei
Contributed-by: Pasi Eronen p...@iki.fi

! src/share/classes/com/sun/crypto/provider/DHKeyAgreement.java
! src/share/classes/sun/security/pkcs11/P11KeyAgreement.java
! src/share/classes/sun/security/pkcs11/P11Signature.java
! src/share/classes/sun/security/pkcs11/P11Util.java
! src/share/classes/sun/security/util/KeyUtil.java
+ test/com/sun/crypto/provider/TLS/TestLeadingZeroes.java
+ test/sun/security/pkcs11/tls/TestLeadingZeroesP11.java

hg: jdk8/tl/jdk: 7160837: DigestOutputStream does not turn off digest calculation when close() is called

[xuelei . fan]

Changeset: 8402ef8fabde
Author:ascarpino
Date:  2013-05-30 22:19 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8402ef8fabde

7160837: DigestOutputStream does not turn off digest calculation when close() 
is called
Reviewed-by: mullan, xuelei

! src/share/classes/java/security/DigestOutputStream.java
! src/share/classes/javax/crypto/CipherInputStream.java
! src/share/classes/javax/crypto/CipherOutputStream.java
+ test/javax/crypto/Cipher/CipherStreamClose.java

hg: jdk8/tl/jdk: 6750584: Cipher.wrap/unwrap methods should define UnsupportedOperationException

[xuelei . fan]

Changeset: 918d9ac17740
Author:ascarpino
Date:  2013-05-30 14:11 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/918d9ac17740

6750584: Cipher.wrap/unwrap methods should define UnsupportedOperationException
Reviewed-by: mullan

! src/share/classes/javax/crypto/Cipher.java
! src/share/classes/javax/crypto/CipherSpi.java

Code review request, JDK-8010814, More buffers are stored or returned without cloning

[Xuelei Fan]

Hi,

There is another fix to avoid the use of mutable objects.

webrev: http://cr.openjdk.java.net/~xuelei/8010814/webrev.00/

Thanks,
Xuelei

Re: Code review request, JDK-8010814, More buffers are stored or returned without cloning

[Xuelei Fan]

On 5/16/2013 5:22 PM, Weijun Wang wrote:
 Hi Xuelei
 
 I'm busy on something else, so probably have no time (or cannot
 concentrate) to read in details.
 
Not urgent fix, so please review these request only when you available.

 In my opinion, there are several cases as to whether to clone or not:
 
 1. MUST NOT clone, because the value must be shared so that change at
 one side appears on the other side.
 
 2. MUST clone, public available methods that leads to security issues.
 
 3. So so, although public methods, cannot be used to do bad things.
 
 4. Not an issue. Internal methods.
 
 In the patch, are the first two cases #1 or #3.
 
It's #2.

For #1 and #3, I added a few comment lines in 8010815.

 Sorry I also have no time to read
 
   JDK-8010814, More buffers are stored or returned without cloning
   http://cr.openjdk.java.net/~xuelei/8010815/webrev.00/
 
 but I am not sure if those env cases belong to #1.
 
I think both fixes are for #2.

Xuelei

 Thanks
 Max
 
 
 On 5/16/13 5:08 PM, Xuelei Fan wrote:
 Hi,

 There is another fix to avoid the use of mutable objects.

 webrev: http://cr.openjdk.java.net/~xuelei/8010814/webrev.00/

 Thanks,
 Xuelei

hg: jdk8/tl/jdk: 8005535: SSLSessionImpl should have protected finalize()

[xuelei . fan]

Changeset: 76998d11a643
Author:xuelei
Date:  2013-05-13 05:41 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/76998d11a643

8005535: SSLSessionImpl should have protected finalize()
Reviewed-by: weijun, wetmore

! src/share/classes/sun/security/ssl/SSLSessionImpl.java

hg: jdk8/tl/jdk: 8005598: (reopened) Need to clone array of input/output parameters

[xuelei . fan]

Changeset: 46db0e633240
Author:xuelei
Date:  2013-05-13 06:05 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/46db0e633240

8005598: (reopened) Need to clone array of input/output parameters
Reviewed-by: weijun

! src/share/classes/com/sun/jndi/dns/DnsContext.java
! src/share/classes/com/sun/jndi/ldap/BasicControl.java

Code review request, JDK-8010815, some constructors issues in com.sun.jndi.toolkit

[Xuelei Fan]

Hi,

There is some constructors issues about mutable objects in
com.sun.jndi.toolkit.

webrev: http://cr.openjdk.java.net/~xuelei/8010815/webrev.00/

Thanks,
Xuelei

Code review request: 8005598 (reopened) Need to clone array of input/output parameters

[Xuelei Fan]

Hi,

It's a correction of previous fix of JDK-8003265:

  http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4472a641b4dc

Which introduced compiler warning and error, and backouted later.

Thanks,
Xuelei

Re: Code review request: 8005598 (reopened) Need to clone array of input/output parameters

[Xuelei Fan]

Oops, here is the webrev:

http://cr.openjdk.java.net/~xuelei/8005598/webrev.00/

Xuelei

On 5/9/2013 10:30 AM, Xuelei Fan wrote:
 Hi,
 
 It's a correction of previous fix of JDK-8003265:
 
   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4472a641b4dc
 
 Which introduced compiler warning and error, and backouted later.
 
 Thanks,
 Xuelei
 

hg: jdk8/tl/jdk: 8006935: Need to take care of long secret keys in HMAC/PRF compuation

[xuelei . fan]

Changeset: 7bdb3e186497
Author:xuelei
Date:  2013-04-18 22:23 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7bdb3e186497

8006935: Need to take care of long secret keys in HMAC/PRF compuation
Reviewed-by: valeriep

! src/share/classes/com/sun/crypto/provider/TlsPrfGenerator.java

hg: jdk8/tl/jdk: 7030966: Support AEAD CipherSuites

[xuelei . fan]

Changeset: def2e05299b7
Author:xuelei
Date:  2013-03-01 02:34 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/def2e05299b7

7030966: Support AEAD CipherSuites
Reviewed-by: weijun, wetmore, valeriep

! src/share/classes/com/sun/crypto/provider/TlsKeyMaterialGenerator.java
! src/share/classes/sun/security/internal/spec/TlsKeyMaterialParameterSpec.java
! src/share/classes/sun/security/internal/spec/TlsKeyMaterialSpec.java
! src/share/classes/sun/security/pkcs11/P11TlsKeyMaterialGenerator.java
+ src/share/classes/sun/security/ssl/Authenticator.java
! src/share/classes/sun/security/ssl/CipherBox.java
! src/share/classes/sun/security/ssl/CipherSuite.java
! src/share/classes/sun/security/ssl/EngineInputRecord.java
! src/share/classes/sun/security/ssl/EngineOutputRecord.java
! src/share/classes/sun/security/ssl/EngineWriter.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/InputRecord.java
! src/share/classes/sun/security/ssl/JsseJce.java
! src/share/classes/sun/security/ssl/MAC.java
! src/share/classes/sun/security/ssl/OutputRecord.java
! src/share/classes/sun/security/ssl/Record.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java
! test/sun/security/ec/TestEC.java
! test/sun/security/pkcs11/fips/CipherTest.java
! test/sun/security/pkcs11/sslecc/CipherTest.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
+ test/sun/security/ssl/javax/net/ssl/TLSv12/ShortRSAKeyGCM.java
! test/sun/security/ssl/sanity/ciphersuites/CipherSuitesInOrder.java
! test/sun/security/ssl/sanity/interop/CipherTest.java
! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java

hg: jdk8/tl/jdk: 8006265: Add test SSLEngineDeadlock.java to ProblemList

[xuelei . fan]

Changeset: edb7e34a0531
Author:xuelei
Date:  2013-01-14 18:31 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/edb7e34a0531

8006265: Add test SSLEngineDeadlock.java to ProblemList
Reviewed-by: weijun

! test/ProblemList.txt

hg: jdk8/tl/jdk: 7109274: Restrict the use of certificates with RSA keys less than 1024 bits

[xuelei . fan]

Changeset: 645d774b683a
Author:xuelei
Date:  2012-12-28 00:48 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/645d774b683a

7109274: Restrict the use of certificates with RSA keys less than 1024 bits
Summary: This restriction is applied via the Java Security property, 
jdk.certpath.disabledAlgorithms. This will impact providers that adhere to 
this security property.
Reviewed-by: mullan

! src/share/lib/security/java.security-linux
! src/share/lib/security/java.security-macosx
! src/share/lib/security/java.security-solaris
! src/share/lib/security/java.security-windows
! 
test/java/security/cert/CertPathBuilder/targetConstraints/BuildEEBasicConstraints.java
! test/java/security/cert/pkix/policyChanges/TestPolicy.java
! test/sun/security/provider/certpath/DisabledAlgorithms/CPBuilder.java
! 
test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorEndEntity.java
! 
test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorIntermediate.java
! 
test/sun/security/provider/certpath/DisabledAlgorithms/CPValidatorTrustAnchor.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/ClientHandshaker/RSAExport.java
+ test/sun/security/ssl/javax/net/ssl/TLSv12/DisabledShortRSAKeys.java
! test/sun/security/ssl/javax/net/ssl/TLSv12/ShortRSAKey512.java
! test/sun/security/tools/jarsigner/concise_jarsigner.sh

hg: jdk8/tl/jdk: 8003265: Need to clone array of input/output parameters

[xuelei . fan]

Changeset: 4472a641b4dc
Author:xuelei
Date:  2012-12-28 03:50 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4472a641b4dc

8003265: Need to clone array of input/output parameters
Reviewed-by: mullan

! src/share/classes/com/sun/jndi/dns/DnsContext.java
! src/share/classes/com/sun/jndi/ldap/BasicControl.java

hg: jdk8/tl/jdk: 8004184: security tests leave JSSEServer running

[xuelei . fan]

Changeset: ead651efb271
Author:xuelei
Date:  2012-12-03 06:00 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ead651efb271

8004184: security tests leave JSSEServer running
Summary: Use othervm mode to release resources,  and correct the system 
properties issues in JSSE
Reviewed-by: chegar

! test/sun/security/pkcs11/sslecc/ClientJSSEServerJSSE.java

hg: jdk8/tl/jdk: 8004019: Removes unused method HandshakeHash.setCertificateVerifyAlg()

[xuelei . fan]

Changeset: 46c627801490
Author:xuelei
Date:  2012-11-28 05:18 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/46c627801490

8004019: Removes unused method HandshakeHash.setCertificateVerifyAlg()
Summary: certification verification in HandshakeHash was abandoned during TLS 
1.2 implementation
Reviewed-by: xuelei, weijun
Contributed-by: Florian Weimer fwei...@redhat.com

! src/share/classes/sun/security/ssl/ClientHandshaker.java
! src/share/classes/sun/security/ssl/HandshakeHash.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/MAC.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
! src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java

hg: jdk8/tl/jdk: 8003950: Adds missing Override annotations and removes unnecessary imports in sun.security.ssl

[xuelei . fan]

Changeset: f7d45462b225
Author:xuelei
Date:  2012-11-24 04:09 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7d45462b225

8003950: Adds missing Override annotations and removes unnecessary imports in 
sun.security.ssl
Reviewed-by: xuelei
Contributed-by: Florian Weimer fwei...@redhat.com

! src/share/classes/sun/security/ssl/AppInputStream.java
! src/share/classes/sun/security/ssl/AppOutputStream.java
! src/share/classes/sun/security/ssl/BaseSSLSocketImpl.java
! src/share/classes/sun/security/ssl/ByteBufferInputStream.java
! src/share/classes/sun/security/ssl/CipherBox.java
! src/share/classes/sun/security/ssl/CipherSuite.java
! src/share/classes/sun/security/ssl/CipherSuiteList.java
! src/share/classes/sun/security/ssl/ClientHandshaker.java
! src/share/classes/sun/security/ssl/DHClientKeyExchange.java
! src/share/classes/sun/security/ssl/ECDHClientKeyExchange.java
! src/share/classes/sun/security/ssl/ECDHCrypt.java
! src/share/classes/sun/security/ssl/EngineInputRecord.java
! src/share/classes/sun/security/ssl/EngineOutputRecord.java
! src/share/classes/sun/security/ssl/EngineWriter.java
! src/share/classes/sun/security/ssl/ExtensionType.java
! src/share/classes/sun/security/ssl/HandshakeHash.java
! src/share/classes/sun/security/ssl/HandshakeInStream.java
! src/share/classes/sun/security/ssl/HandshakeMessage.java
! src/share/classes/sun/security/ssl/HandshakeOutStream.java
! src/share/classes/sun/security/ssl/Handshaker.java
! src/share/classes/sun/security/ssl/HelloExtension.java
! src/share/classes/sun/security/ssl/HelloExtensions.java
! src/share/classes/sun/security/ssl/InputRecord.java
! src/share/classes/sun/security/ssl/JsseJce.java
! src/share/classes/sun/security/ssl/KerberosClientKeyExchange.java
! src/share/classes/sun/security/ssl/KeyManagerFactoryImpl.java
! src/share/classes/sun/security/ssl/Krb5Helper.java
! src/share/classes/sun/security/ssl/OutputRecord.java
! src/share/classes/sun/security/ssl/ProtocolList.java
! src/share/classes/sun/security/ssl/ProtocolVersion.java
! src/share/classes/sun/security/ssl/RSAClientKeyExchange.java
! src/share/classes/sun/security/ssl/RSASignature.java
! src/share/classes/sun/security/ssl/RenegotiationInfoExtension.java
! src/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
! src/share/classes/sun/security/ssl/SSLContextImpl.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
! src/share/classes/sun/security/ssl/SSLServerSocketFactoryImpl.java
! src/share/classes/sun/security/ssl/SSLServerSocketImpl.java
! src/share/classes/sun/security/ssl/SSLSessionContextImpl.java
! src/share/classes/sun/security/ssl/SSLSessionImpl.java
! src/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
! src/share/classes/sun/security/ssl/ServerNameExtension.java
! src/share/classes/sun/security/ssl/SessionId.java
! src/share/classes/sun/security/ssl/SunJSSE.java
! src/share/classes/sun/security/ssl/SunX509KeyManagerImpl.java
! src/share/classes/sun/security/ssl/SupportedEllipticCurvesExtension.java
! src/share/classes/sun/security/ssl/SupportedEllipticPointFormatsExtension.java
! src/share/classes/sun/security/ssl/TrustManagerFactoryImpl.java
! src/share/classes/sun/security/ssl/UnknownExtension.java
! src/share/classes/sun/security/ssl/X509KeyManagerImpl.java
! src/share/classes/sun/security/ssl/X509TrustManagerImpl.java

hg: jdk8/tl/jdk: 8003951: Removes unused variables in sun.security.ssl

[xuelei . fan]

Changeset: d30c13172254
Author:xuelei
Date:  2012-11-24 04:27 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d30c13172254

8003951: Removes unused variables in sun.security.ssl
Reviewed-by: xuelei
Contributed-by: Florian Weimer fwei...@redhat.com

! src/share/classes/sun/security/ssl/HandshakeMessage.java
! src/share/classes/sun/security/ssl/JsseJce.java
! src/share/classes/sun/security/ssl/SSLServerSocketImpl.java
! src/share/classes/sun/security/ssl/SSLSessionContextImpl.java
! src/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java
! src/share/classes/sun/security/ssl/X509TrustManagerImpl.java

hg: jdk8/tl/jdk: 8003587: Warning cleanup in package javax.net.ssl

[xuelei . fan]

Changeset: 25e5df117021
Author:xuelei
Date:  2012-11-18 01:31 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/25e5df117021

8003587: Warning cleanup in package javax.net.ssl
Summary: Removes unnecessary imports and adds missing Override annotations
Reviewed-by: xuelei
Contributed-by: Florian Weimer fwei...@redhat.com

! src/share/classes/javax/net/ssl/HandshakeCompletedEvent.java
! src/share/classes/javax/net/ssl/HostnameVerifier.java
! src/share/classes/javax/net/ssl/HttpsURLConnection.java
! src/share/classes/javax/net/ssl/KeyManagerFactory.java
! src/share/classes/javax/net/ssl/SSLContext.java
! src/share/classes/javax/net/ssl/SSLContextSpi.java
! src/share/classes/javax/net/ssl/SSLEngineResult.java
! src/share/classes/javax/net/ssl/SSLParameters.java
! src/share/classes/javax/net/ssl/SSLPermission.java
! src/share/classes/javax/net/ssl/SSLServerSocketFactory.java
! src/share/classes/javax/net/ssl/SSLSession.java
! src/share/classes/javax/net/ssl/SSLSocket.java
! src/share/classes/javax/net/ssl/SSLSocketFactory.java
! src/share/classes/javax/net/ssl/TrustManagerFactory.java
! src/share/classes/javax/net/ssl/X509KeyManager.java

Re: [8] Code Review Request for CR 7167056 - Clarify that BasicPermission names that contain non-wildcard asterisks are not invalid

[Xuelei Fan]

Looks fine to me.

Xuelei

On 11/16/2012 10:54 PM, Sean Mullan wrote:
 This change affects components in the security and core libs areas.
 
 This is a minor specification clarification to avoid the use of the terms
 valid and invalid when describing the syntax for wildcard names in
 java.security.BasicPermission and various subclasses. This could be implied 
 that
 these wildcard names are illegal and the constructors should throw an 
 exception.
 However, they are acceptable, but simply won't be treated as wildcards by the
 implies method when matching against other permissions.
 
 bug : http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7167056
 webrev: http://cr.openjdk.java.net/~mullan/webrevs/7167056/webrev.00/
 
 Thanks,
 Sean
 

hg: jdk8/tl/jdk: 8001569: Regression test GetPeerHost uses static port number

[xuelei . fan]

Changeset: 9edfa0e761b9
Author:xuelei
Date:  2012-11-09 01:15 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9edfa0e761b9

8001569: Regression test GetPeerHost uses static port number
Reviewed-by: weijun

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/ServerHandshaker/GetPeerHost.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/ServerHandshaker/GetPeerHostClient.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/ServerHandshaker/GetPeerHostServer.java

hg: jdk8/tl/jdk: 8001466: Nightly regression test failure of SSLSocketSNISensitive.java

[xuelei . fan]

Changeset: e782f3c383fe
Author:xuelei
Date:  2012-10-24 08:25 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e782f3c383fe

8001466: Nightly regression test failure of SSLSocketSNISensitive.java
Reviewed-by: weijun

! test/sun/security/ssl/javax/net/ssl/ServerName/SSLSocketSNISensitive.java

hg: jdk8/tl/jdk: 7200295: CertificateRequest message is wrapping when using large numbers of Certs

[xuelei . fan]

Changeset: a58585051c4b
Author:xuelei
Date:  2012-09-26 21:05 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a58585051c4b

7200295: CertificateRequest message is wrapping when using large numbers of 
Certs
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/HandshakeMessage.java
! src/share/classes/sun/security/ssl/HandshakeOutStream.java
! src/share/classes/sun/security/ssl/Record.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/X509TrustManagerImpl/CertRequestOverflow.java

hg: jdk8/tl/jdk: 7199066: Typo in method name

[xuelei . fan]

Changeset: 88a4f699d233
Author:xuelei
Date:  2012-09-18 06:51 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/88a4f699d233

7199066: Typo in method name
Reviewed-by: mullan

! src/share/classes/sun/security/ssl/SSLContextImpl.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
! src/share/classes/sun/security/ssl/SSLServerSocketFactoryImpl.java
! src/share/classes/sun/security/ssl/SSLServerSocketImpl.java
! src/share/classes/sun/security/ssl/SSLSocketFactoryImpl.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java

hg: jdk8/tl/jdk: 7195733: TEST_BUG: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java failing

[xuelei . fan]

Changeset: b7b33a3c9df0
Author:xuelei
Date:  2012-09-04 02:24 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b7b33a3c9df0

7195733: TEST_BUG: 
sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java 
failing
Reviewed-by: chegar, alanb, xuelei
Contributed-by: Eric Wang yiming.w...@oracle.com

! 
test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/B6216082.java

hg: jdk8/tl/jdk: 7185576: Need to consider the connection timeout at test/com/sun/jndi/ldap/InvalidLdapFilters.java

[xuelei . fan]

Changeset: e0e7cc711bda
Author:xuelei
Date:  2012-07-24 03:31 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e0e7cc711bda

7185576: Need to consider the connection timeout at 
test/com/sun/jndi/ldap/InvalidLdapFilters.java
Reviewed-by: vinnie

! test/com/sun/jndi/ldap/InvalidLdapFilters.java

hg: jdk8/tl/jdk: 7180038: regression test failure, SSLEngineBadBufferArrayAccess.java

[xuelei . fan]

Changeset: 3ae91286f313
Author:xuelei
Date:  2012-07-03 20:29 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3ae91286f313

7180038: regression test failure, SSLEngineBadBufferArrayAccess.java
Reviewed-by: weijun

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java

hg: jdk8/tl/jdk: 7166487: checkSequenceNumber method never called within readRecord of SSLEngineImpl

[xuelei . fan]

Changeset: cdcbd22cfb9d
Author:xuelei
Date:  2012-06-19 17:28 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cdcbd22cfb9d

7166487: checkSequenceNumber method never called within readRecord of 
SSLEngineImpl
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/SSLEngineImpl.java

hg: jdk8/tl/jdk: 7174244: NPE in Krb5ProxyImpl.getServerKeys()

[xuelei . fan]

Changeset: f8e72d7ff37d
Author:xuelei
Date:  2012-06-06 18:18 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f8e72d7ff37d

7174244: NPE in Krb5ProxyImpl.getServerKeys()
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/SSLContextImpl.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
! src/share/classes/sun/security/ssl/krb5/Krb5ProxyImpl.java
+ test/sun/security/ssl/sanity/ciphersuites/CipherSuitesInOrder.java

hg: jdk8/tl/jdk: 7172149: ArrayIndexOutOfBoundsException from Signature.verify

[xuelei . fan]

Changeset: 713b10821c3d
Author:xuelei
Date:  2012-06-06 18:39 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/713b10821c3d

7172149: ArrayIndexOutOfBoundsException from Signature.verify
Summary: take care of integer addition overflow
Reviewed-by: xuelei, wetmore
Contributed-by: Jonathan Lu luc...@linux.vnet.ibm.com

! src/share/classes/java/security/Signature.java
+ test/java/security/Signature/VerifyRangeCheckOverflow.java

hg: jdk8/tl/jdk: 7145960: sun/security/mscapi/ShortRSAKey1024.sh failing on windows

[xuelei . fan]

Changeset: 9fe6ebbe5895
Author:xuelei
Date:  2012-05-17 21:59 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/9fe6ebbe5895

7145960: sun/security/mscapi/ShortRSAKey1024.sh failing on windows
Reviewed-by: vinnie, wetmore

! test/sun/security/mscapi/ShortRSAKey1024.sh
! test/sun/security/mscapi/ShortRSAKey512.sh
! test/sun/security/mscapi/ShortRSAKey768.sh

hg: jdk8/tl/jdk: 7167988: PKIX CertPathBuilder in reverse mode doesn't work if more than one trust anchor is specified

[xuelei . fan]

Changeset: df3152beef2f
Author:xuelei
Date:  2012-05-14 07:26 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/df3152beef2f

7167988: PKIX CertPathBuilder in reverse mode doesn't work if more than one 
trust anchor is specified
Reviewed-by: mullan

! src/share/classes/sun/security/provider/certpath/SunCertPathBuilder.java
+ test/sun/security/provider/certpath/ReverseBuilder/ReverseBuild.java

hg: jdk8/tl/jdk: 7166570: JSSE certificate validation has started to fail for certificate chains

[xuelei . fan]

Changeset: 0f63f3390ac9
Author:xuelei
Date:  2012-05-08 18:08 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0f63f3390ac9

7166570: JSSE certificate validation has started to fail for certificate chains
Reviewed-by: wetmore

! src/share/classes/sun/security/validator/SimpleValidator.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/X509TrustManagerImpl/BasicConstraints.java

hg: jdk8/tl/jdk: 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites

[xuelei . fan]

Changeset: 41d3f7509e00
Author:xuelei
Date:  2012-05-04 17:28 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41d3f7509e00

7153184: NullPointerException when calling 
SSLEngineImpl.getSupportedCipherSuites
Reviewed-by: weijun

! src/share/classes/sun/security/ssl/SSLContextImpl.java

hg: jdk8/tl/jdk: 7158688: Typo in SSLContext Spec

[xuelei . fan]

Changeset: 71fdf32fdc65
Author:xuelei
Date:  2012-05-01 03:48 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/71fdf32fdc65

7158688: Typo in SSLContext Spec
Reviewed-by: weijun, wetmore

! src/share/classes/javax/net/ssl/SSLContext.java

hg: jdk8/tl/jdk: 6996372: synchronizing handshaking hash

[xuelei . fan]

Changeset: f0842ed897c3
Author:xuelei
Date:  2012-04-27 04:25 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f0842ed897c3

6996372: synchronizing handshaking hash
Summary: remove the unnecessary synchronization. Also reviewed by David 
Schlosnagle (schlo...@gmail.com)
Reviewed-by: weijun

! src/share/classes/sun/security/util/DisabledAlgorithmConstraints.java

hg: jdk8/tl/jdk: 7147407: remove never used debug code in DnsClient.java

[xuelei . fan]

Changeset: a4e3dde9a8a7
Author:xuelei
Date:  2012-02-21 05:44 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a4e3dde9a8a7

7147407: remove never used debug code in DnsClient.java
Reviewed-by: vinnie

! src/share/classes/com/sun/jndi/dns/DnsClient.java

hg: jdk8/tl/jdk: 7145837: a little performance improvement on the usage of SecureRandom

[xuelei . fan]

Changeset: 45804d661008
Author:xuelei
Date:  2012-02-15 23:45 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/45804d661008

7145837: a little performance improvement on the usage of SecureRandom
Reviewed-by: chegar, wetmore

! src/share/classes/sun/security/ssl/CipherSuite.java

hg: jdk8/tl/jdk: 7144781: incorrect URLs in JSSE java doc

[xuelei . fan]

Changeset: da8b8ee281f9
Author:xuelei
Date:  2012-02-10 22:17 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/da8b8ee281f9

7144781: incorrect URLs in JSSE java doc
Reviewed-by: wetmore, skannan

! src/share/classes/javax/net/ssl/ExtendedSSLSession.java
! src/share/classes/javax/net/ssl/SSLParameters.java

hg: jdk8/tl/jdk: 7132248: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/CookieHttpsClientTest.java failing

[xuelei . fan]

Changeset: d383b5d128e3
Author:xuelei
Date:  2012-01-23 04:44 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d383b5d128e3

7132248: 
sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/CookieHttpsClientTest.java
 failing
Reviewed-by: alanb

! 
test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/CookieHttpsClientTest.java

hg: jdk8/tl/jdk: 7106773: 512 bits RSA key cannot work with SHA384 and SHA512

[xuelei . fan]

Changeset: 11e52d5ba64e
Author:xuelei
Date:  2012-01-12 03:39 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/11e52d5ba64e

7106773: 512 bits RSA key cannot work with SHA384 and SHA512
Reviewed-by: weijun

! src/share/classes/sun/security/pkcs11/P11Cipher.java
! src/share/classes/sun/security/pkcs11/P11Key.java
! src/share/classes/sun/security/pkcs11/P11RSACipher.java
! src/share/classes/sun/security/pkcs11/P11Signature.java
! src/share/classes/sun/security/ssl/ClientHandshaker.java
! src/share/classes/sun/security/ssl/ServerHandshaker.java
! src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java
! src/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
+ src/share/classes/sun/security/util/KeyLength.java
+ src/share/classes/sun/security/util/Length.java
! src/windows/classes/sun/security/mscapi/Key.java
! src/windows/classes/sun/security/mscapi/RSACipher.java
! src/windows/classes/sun/security/mscapi/RSASignature.java
+ test/sun/security/mscapi/ShortRSAKey1024.sh
+ test/sun/security/mscapi/ShortRSAKey512.sh
+ test/sun/security/mscapi/ShortRSAKey768.sh
+ test/sun/security/mscapi/ShortRSAKeyWithinTLS.java
! test/sun/security/pkcs11/KeyStore/ClientAuth.java
! test/sun/security/pkcs11/KeyStore/ClientAuth.sh
! test/sun/security/ssl/javax/net/ssl/SSLContextVersion.java
+ test/sun/security/ssl/javax/net/ssl/TLSv12/ShortRSAKey512.java

hg: jdk8/tl/jdk: 7113275: compatibility issue with MD2 trust anchor and old X509TrustManager

[xuelei . fan]

Changeset: 82151e860a64
Author:xuelei
Date:  2011-11-23 03:40 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/82151e860a64

7113275: compatibility issue with MD2 trust anchor and old X509TrustManager
Summary: also reviewed by dennis...@oracle.com
Reviewed-by: mullan

! src/share/classes/sun/security/ssl/SSLContextImpl.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/MD2InTrustAnchor.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/TrustTrustedCert.java

hg: jdk8/tl/jdk: 7111548: unexpected debug log message

[xuelei . fan]

Changeset: 5c7c83a6ee24
Author:xuelei
Date:  2011-11-14 01:21 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5c7c83a6ee24

7111548: unexpected debug log message
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/SSLSocketImpl.java

hg: jdk8/tl/jdk: 7106277: Brokenness in the seqNumberOverflow of MAC

[xuelei . fan]

Changeset: 30900a1a9cfc
Author:xuelei
Date:  2011-10-30 20:07 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/30900a1a9cfc

7106277: Brokenness in the seqNumberOverflow of MAC
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/MAC.java

hg: jdk8/tl/jdk: 7092375: Security Libraries don't build with javac -Werror

[xuelei . fan]

Changeset: ffa762153af4
Author:xuelei
Date:  2011-09-28 15:10 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ffa762153af4

7092375: Security Libraries don't build with javac -Werror
Summary: Changes to security related java and make files to remove warnings
Reviewed-by: xuelei
Contributed-by: kurchi.subhra.ha...@oracle.com

! make/java/security/Makefile
! make/javax/Makefile
! make/javax/others/Makefile
+ make/javax/security/Makefile
! make/org/ietf/jgss/Makefile
! make/sun/security/other/Makefile
! src/share/classes/java/security/Signature.java
! src/share/classes/javax/security/auth/PrivateCredentialPermission.java
! src/share/classes/javax/security/auth/Subject.java
! src/share/classes/javax/security/auth/SubjectDomainCombiner.java
! src/share/classes/javax/security/auth/kerberos/DelegationPermission.java
! src/share/classes/javax/security/auth/kerberos/ServicePermission.java
! src/share/classes/javax/security/auth/login/LoginContext.java
! src/share/classes/javax/security/auth/x500/X500Principal.java
! src/share/classes/javax/security/cert/CertificateEncodingException.java
! src/share/classes/javax/security/cert/CertificateException.java
! src/share/classes/javax/security/cert/CertificateExpiredException.java
! src/share/classes/javax/security/cert/CertificateNotYetValidException.java
! src/share/classes/javax/security/cert/CertificateParsingException.java
! src/share/classes/javax/security/cert/X509Certificate.java
! src/share/classes/javax/security/sasl/Sasl.java
! src/share/classes/javax/smartcardio/TerminalFactory.java
! src/share/classes/sun/security/ec/ECPublicKeyImpl.java
! src/share/classes/sun/security/validator/PKIXValidator.java
! src/share/classes/sun/security/validator/SimpleValidator.java
! src/share/classes/sun/security/x509/X509CertImpl.java

Re: Request for review: 7084245: Update usages of InternalError to use exception chaining

[Xuelei Fan]

I reviewed the security part. In general, it looks fine. But sometimes,
from my very personal view, the exception-chain might be over used. For
example, the following code:

  try {
  md = MessageDigest.getInstance(SHA);
  } catch (NoSuchAlgorithmException nsae) {
  throw new InternalError(internal error: SHA-1 not available.);
  }

The exception message and stack is simple and easy to understand. If
using exception-chain, the message chain looks like:
   internal error: SHA-1 not available.
   - SHA ... not available

I think the information is redundant, and expose unnecessary call stack.

Exceptions thrown by a method should be defined at an abstraction level
consistent with what the method does, not necessarily with the low-level
details of how it is implemented. Please also refer to item 61, Throw
exceptions appropriate to the abstraction, in Effective Java 2nd Editor.

It seems that the webrev offline now, I cannot access it.

Thanks,
Xuelei

On 8/29/2011 3:35 AM, Sebastian Sickelmann wrote:
 Hi, here is a webrev[1] for some cleanup that i want to integrated in
 tl-repositories.
 
 Alan Bateman had scanned the changes and gave me some good input[3] for
 further discussion here:
 
 The changes to java.util.concurrent should go through Doug Lea's
 upstream CVS. Alan told me that Chris Hegarty is on this topic already.
 The suggested changes for this is here[2].
 
 I have changed some classes in awt / sun.java2d maybe someone of the
 2d-dev maillinglist can look at these changes.
 I also changed some classes in java/securtiy maybe someone of
 security-dev maillinglist can look at these changes.
 
 Let me know if there is a need to split/rebase the main-webrev[1] to
 review and/or push it individually.
 
 Mostly the patch changes exception-chains. But there are some places
 where the patch changes behavoir:
 
 - I removed some printstackTraces in
 sun.java2d.pipe.LoopPipe.getStrokesSpans and sun.misc.Launcher (Alan
 told me that kumar maybe want to have a look at it?).
 - I changed java.text.Format.clone not to return null. I think it will
 never happen. But if so throwing an InternalError seems to be better
 than returning null and let all the extended classes crash in there
 clone Method with a NullPointerException. And so catching an Exception
 in java.text.DecimalFormat.clone is unnecessary.
 
 -- Sebastian
 
 [1] http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_main_0/
 [2]
 http://oss-patches.24.eu/openjdk8/InternalError/part2/7084245_concurrent_0/
 [3]
 http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-August/007563.html