Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Weijun Wang]

On 8/26/2016 2:52, Artem Smotrakov wrote:

My point is that it's better to run tests which don't modify the default
JSSE configuration in agent VM. It would speed up test execution.


Another benefit of this is that if the test still fails it might reveal 
a real bug.


--Max

Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Weijun Wang]

Can you add a new @run line which is the getNameCount()==1 but 
getParent() != policyPath case?


+ * @run main/othervm TestUnlimited /unlimited exception

No other comment.

On 8/26/2016 6:56, Bradford Wetmore wrote:

Last call:

http://cr.openjdk.java.net/~wetmore/8061842/webrev.03/

Thanks Sean for pointing out my new NPE I just introduced! ("Maybe add a
null check to be cleaner?")  Drat!  I would add a new test case, but
Security.setProperty() doesn't allow for "null."


I always wanted a Security::clearProperty().

--Max

Re: RFR 8074838: Resolve disabled warnings for libj2pkcs11

[Bradford Wetmore]

Looks ok to me...

I had to look at the C declaration one twice.  Too much Java, not enough 
C lately.


Brad



On 8/25/2016 2:20 PM, Anthony Scarpino wrote:

Hi,

Can I get a review of this change to remove the warning suppression and
fix the minor compiler issues that it was hiding in the pkcs11 wrapper
library.

http://cr.openjdk.java.net/~ascarpino/8074838/webrev/

thanks

Tony

Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Bradford Wetmore]

Last call:

http://cr.openjdk.java.net/~wetmore/8061842/webrev.03/

Changes from .02:

added null check for cryptoPolicyProperty
renamed javaHomePolicyPath
tweaked exception wording "files in"
added test for empty string ("").


On 8/24/2016 6:22 PM, Weijun Wang wrote:

+Path javaHomePath = Paths.get(javaHomeProperty, "conf",
"security",
+"policy").normalize();

This is not javaHomePath, but policyPath.


Renamed as javaHomePolicyPath.


You added "cryptoPolicyProperty" to some exceptions, but the exception
titles are "Unexpected jurisdiction policy filename found: " and
"Couldn't parse jurisdiction policy files: ". cryptoPolicyProperty is
not file(s). Maybe "... file(s) in " + cryptoPolicyProperty?


Sure.  Sounds good.

Tony wrote:
> So by having no crypto.policy defined we have no JCA?

With no crypto.policy, that shuts down the JceSecurity init, and the app 
will exit with an ExceptionInInitializerError, like it does if the 
policy files are missing today.


Thanks Sean for pointing out my new NPE I just introduced! ("Maybe add a 
null check to be cleaner?")  Drat!  I would add a new test case, but 
Security.setProperty() doesn't allow for "null."


> Does that mean no
> operations at all (No MessageDigest, etc) or no restrictable crypto
> ops?

Recall that our JCA/JCE Permissions model is based on positive 
permission model checks.  It would mean no CryptoPermissions would be 
created, thus any operation that needs a CryptoPermissions (implies()) 
would fail.  So if you were somehow able to get the system to have no 
policy, there would be no Permissions granted to do any JCE operations.



As an aside, it would only fail JCE (Cipher/Mac/KeyAgreement), JCA would 
be unaffected as it doesn't use permissions.


> Since we know a limited number of countries have import issues, can we
> make no crypto.policy property defined as unlimited policy?  Defining
> the property would be for only limiting the access.  We could get rid of
> the unlimited policy file and just ship a limited policy file.

I'm very hesitant to do that, we need to be limited by default.

On 8/25/2016 6:56 AM, Sean Coffey wrote:
> I missed the "crypto.policy=crypto.policydir-tbd" approach in 1st
> revision. Isn't it going to confuse people studying the code ? This
> value gets replaced at build time - right ?

Yes.

> What about changing it to
> something like :
> "crypto.policy=$crypto.policydir-tbd //replaced at build time"
>
> and having the make files search for that string instead ? I think it
> makes the intent more obvious.

We're doing the same thing with "tbd" during the provider slot 
assignment. I'd like to keep it somewhat similar.


Brad




No other comment.

Thanks
Max

On 8/25/2016 8:21, Bradford Wetmore wrote:

Max/SeanC/SeanM,

The latest update:

http://cr.openjdk.java.net/~wetmore/8061842/webrev.02/

On 8/17/2016 9:26 PM, Wang Weijun wrote:

Before this change, you require default policy in neither export nor
import to be empty but do not care about the getMinimum() result. In
this change, you make sure the final result is not empty. I assume
this is a fix?


I made the change to allow for our traditional (default) export/import
mechanism, but other additional styles could be added/used.  Since we no
longer sign, distros are free to edit, add and/or remove files.  But
before doing any JCE operation, the environment needs to grant
something, otherwise there are no perms and no JCE available.


283 // Did we find a default perms?

What does this line mean?


I've moved to the right position in the file.  I meant did we find a
default perms file, vs an exempt.


296 // This should never happen

But you can still print out the file name.


I'm concerned that the exception might print out the entire path instead
of just the filename, which would include java.home and probably should
not be made available.


Can you rename policydir-tbd to something else? I am afraid it will be
confused with policy.url.1 etc.


Changed to:  crypto.policydir-tbd?


The original README.TXT in unlimited says "are exportable from the
United States." and you have "is exportable." now. Is this intended?
(IANAL)


Changed.


TestUnlimited.java:
45 "// Use the AES are the test Cipher", you mean "Use AES as the test
Cipher"?
51 "throw new Exception ("Unlimited policy is NOT active");". No space
before "(".


Fixed.

Sean Mullan wrote:

 > What about setting the default value to "limited"? And then this
 > would only be changed to "unlimited" if the build --enable-unlimited-
 > crypto option is specified?

I could, but I'm concerned that a build with --enabled-unlimited-crypto
would expect that the compiled-in version default would also be
unlimited and would be surprised with limited.

Upon Max's suggestion above, I've changed the name of the marker to
"crypto.policy=crypto.policydir-tbd."  Does that work for you?

 > Instead of throwing an exception here, I wonder if it 

RFR 8074838: Resolve disabled warnings for libj2pkcs11

[Anthony Scarpino]

Hi,

Can I get a review of this change to remove the warning suppression and 
fix the minor compiler issues that it was hiding in the pkcs11 wrapper 
library.


http://cr.openjdk.java.net/~ascarpino/8074838/webrev/

thanks

Tony

Re: RFR: 8150158: Update bugs.sun.com references to bugs.java.com

[Anthony Scarpino]

Looks fine to me.

Tony

On 08/25/2016 12:55 PM, Sean Mullan wrote:

Real trivial fix, just updating obsolete bug URLs in code comments.

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

$ hg diff --nodates -a -U 1
diff -r 9c7eb3e1799f
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java

---
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java

+++
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java

@@ -152,3 +152,3 @@
  // reinstantiate Mac object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
  Mac mac = this.macAlgorithm;
diff -r 9c7eb3e1799f
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java

---
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java

+++
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java

@@ -114,3 +114,3 @@
  // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
  Signature sig = this.signatureAlgorithm;
diff -r 9c7eb3e1799f
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java

---
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java

+++
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java

@@ -141,3 +141,3 @@
  // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
  Signature sig = this.signatureAlgorithm;
diff -r 9c7eb3e1799f
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java

---
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java

+++
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java

@@ -254,3 +254,3 @@
  // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
  Signature sig = this.signatureAlgorithm;

--Sean

RFR: 8150158: Update bugs.sun.com references to bugs.java.com

[Sean Mullan]

Real trivial fix, just updating obsolete bug URLs in code comments.

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

$ hg diff --nodates -a -U 1
diff -r 9c7eb3e1799f 
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java
--- 
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java
+++ 
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/IntegrityHmac.java

@@ -152,3 +152,3 @@
 // reinstantiate Mac object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
 Mac mac = this.macAlgorithm;
diff -r 9c7eb3e1799f 
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java
--- 
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java
+++ 
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureBaseRSA.java

@@ -114,3 +114,3 @@
 // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
 Signature sig = this.signatureAlgorithm;
diff -r 9c7eb3e1799f 
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java
--- 
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java
+++ 
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureDSA.java

@@ -141,3 +141,3 @@
 // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
 Signature sig = this.signatureAlgorithm;
diff -r 9c7eb3e1799f 
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java
--- 
a/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java
+++ 
b/src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/algorithms/implementations/SignatureECDSA.java

@@ -254,3 +254,3 @@
 // reinstantiate Signature object to work around bug in JDK
-// see: http://bugs.sun.com/view_bug.do?bug_id=4953555
+// see: http://bugs.java.com/view_bug.do?bug_id=4953555
 Signature sig = this.signatureAlgorithm;

--Sean

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Sure Xuelei, I read 
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#InstallationAndCustomization


If I understand correctly, the most settings can be done via 
system/security properties. But there may be some other ways like 
setDefaultHostnameVerifier(), setDefaultSSLSocketFactory() methods.


My point is that it's better to run tests which don't modify the default 
JSSE configuration in agent VM. It would speed up test execution. I 
don't think that it's too hard to identify those tests which modify the 
default JSSE configuration. and run them in a separate JVM. I believe 
most of test are fine now.


I rely on your expertise, and I'll let you decide about it.

Artem

On 08/25/2016 10:45 AM, Xuelei Fan wrote:

On Aug 26, 2016, at 1:33 AM, Artem Smotrakov  wrote:

Hi Xuelei,



On 08/25/2016 10:26 AM, Xuelei Fan wrote:


On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
wrote:

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's pretty 
easy to see if a test calls System.setProperty() or Security.setProperty() 
methods which modify default JSSE configuration.

Is there any other way to modify default JSSE configuration, so that it can't 
be changes then?

Yes.

Could you give an example?

I will give some tomorrow.  If you want to make a research by yourself, please 
read the jsse reference guides.



And even it can only be set via explicit call to setProperty(), run it in agent 
vm is still not safe because we don't know where the set may be used other than 
this test case.

Those tests which modify default JSSE configuration should be run in othervm 
mode. This way they won't affect any other tests.

"Should" is an assumption,but not the reality.  And we do not always make it 
right even we want that.


  It is not only about this test, the impact need to consider all JDK test 
cases in a big picture, which is a huge number and we cannot actually evaluate 
so many test cases.

I think we just need to identify all tests that modify default JSSE 
configuration, and make them run in othervm mode. I believe that currently most 
of them run in othervm mode.


To be safe, all is needed, most is not enough.  As I said, too much test case 
to make the evaluation and make all right.   Reality is reality.  I will try to 
find some out of the jsse components tomorrow. If you want to do by yourself 
you can search https or ocsp or ldaps on all JDK test, it may be able to find 
some hints.

Xuelei


Artem

I will reply more details when I have a PC tomorrow.

Xuelei


Artem



On 08/25/2016 12:43 AM, Xuelei Fan wrote:
On 8/25/2016 2:06 PM, Weijun Wang wrote:
I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no static 
fields update in a test case.

Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:
On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.

If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.

Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max


This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Xuelei Fan]

> On Aug 26, 2016, at 1:33 AM, Artem Smotrakov  
> wrote:
> 
> Hi Xuelei,
> 
> 
>> On 08/25/2016 10:26 AM, Xuelei Fan wrote:
>> 
>>> On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
>>> wrote:
>>> 
>>> Hi Xuelei,
>>> 
>>> I am not sure why you think it's too strong to follow to it. To me it's 
>>> pretty easy to see if a test calls System.setProperty() or 
>>> Security.setProperty() methods which modify default JSSE configuration.
>>> 
>>> Is there any other way to modify default JSSE configuration, so that it 
>>> can't be changes then?
>> Yes.
> Could you give an example?
I will give some tomorrow.  If you want to make a research by yourself, please 
read the jsse reference guides.


>> And even it can only be set via explicit call to setProperty(), run it in 
>> agent vm is still not safe because we don't know where the set may be used 
>> other than this test case.
> Those tests which modify default JSSE configuration should be run in othervm 
> mode. This way they won't affect any other tests.
"Should" is an assumption,but not the reality.  And we do not always make it 
right even we want that.

>>  It is not only about this test, the impact need to consider all JDK test 
>> cases in a big picture, which is a huge number and we cannot actually 
>> evaluate so many test cases.
> I think we just need to identify all tests that modify default JSSE 
> configuration, and make them run in othervm mode. I believe that currently 
> most of them run in othervm mode.
> 
To be safe, all is needed, most is not enough.  As I said, too much test case 
to make the evaluation and make all right.   Reality is reality.  I will try to 
find some out of the jsse components tomorrow. If you want to do by yourself 
you can search https or ocsp or ldaps on all JDK test, it may be able to find 
some hints.

Xuelei

> Artem
>> I will reply more details when I have a PC tomorrow.
>> 
>> Xuelei
>> 
>>> Artem
>>> 
>>> 
> On 08/25/2016 12:43 AM, Xuelei Fan wrote:
> On 8/25/2016 2:06 PM, Weijun Wang wrote:
> I think Artem is correct here.
> 
> All those tests that modify static fields should run in othervm mode, so
> that *each* runs in its *own* VM.  All other tests, which make no such
> modifications, can share the *same* VM using agentvm.
 It's too strong to follow.  And it is not easy to know there is no static 
 fields update in a test case.
 
 Xuelei
 
>>> On 8/25/2016 13:31, Xuelei Fan wrote:
>>> On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
>>> Those tests which modify default JSSE parameters should be run in
>>> othervm mode. But other tests which don't do that can be safely run in
>>> agent VM. This test doesn't seem to modify any default JSSE parameter.
>> If this test get booted, other test that need to do more customization
>> may not work any more.
>> 
>> For example, boolean variable A is a singleton static field, default is
>> true.  If A get initialized in default mode, it is "true".  It is not
>> possible to customize it to "false" any more in other test in
>> agentvm/samevm mode.
> Shouldn't these "other" tests run in othervm mode?
> 
> Or, do you mean that even if a JSSE test does not modify a static field
> inside the test, it will modify some through innocent JSSE calls, so no
> JSSE test should ever run in agentvm?
> 
> I find this unnatural.
> 
> Thanks
> Max
> 
>> This test has no impact in agentvm/samevm mode unless no test run in
>> agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
>> configuration as this one.   This assumption is too strong to follow.
>> 
>> Xuelei
>> 
>>> Artem
> 

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,


On 08/25/2016 10:26 AM, Xuelei Fan wrote:



On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
wrote:

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's pretty 
easy to see if a test calls System.setProperty() or Security.setProperty() 
methods which modify default JSSE configuration.

Is there any other way to modify default JSSE configuration, so that it can't 
be changes then?


Yes.

Could you give an example?

And even it can only be set via explicit call to setProperty(), run it in agent 
vm is still not safe because we don't know where the set may be used other than 
this test case.
Those tests which modify default JSSE configuration should be run in 
othervm mode. This way they won't affect any other tests.

  It is not only about this test, the impact need to consider all JDK test 
cases in a big picture, which is a huge number and we cannot actually evaluate 
so many test cases.
I think we just need to identify all tests that modify default JSSE 
configuration, and make them run in othervm mode. I believe that 
currently most of them run in othervm mode.


Artem

I will reply more details when I have a PC tomorrow.

Xuelei


Artem



On 08/25/2016 12:43 AM, Xuelei Fan wrote:

On 8/25/2016 2:06 PM, Weijun Wang wrote:
I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no static 
fields update in a test case.

Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.

If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.

Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max


This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Xuelei Fan]

> On Aug 26, 2016, at 12:43 AM, Artem Smotrakov  
> wrote:
> 
> Hi Xuelei,
> 
> I am not sure why you think it's too strong to follow to it. To me it's 
> pretty easy to see if a test calls System.setProperty() or 
> Security.setProperty() methods which modify default JSSE configuration.
> 
> Is there any other way to modify default JSSE configuration, so that it can't 
> be changes then?
> 
Yes.   And even it can only be set via explicit call to setProperty(), run it 
in agent vm is still not safe because we don't know where the set may be used 
other than this test case.  It is not only about this test, the impact need to 
consider all JDK test cases in a big picture, which is a huge number and we 
cannot actually evaluate so many test cases.   I will reply more details when I 
have a PC tomorrow.

Xuelei

> Artem
> 
> 
>> On 08/25/2016 12:43 AM, Xuelei Fan wrote:
>>> On 8/25/2016 2:06 PM, Weijun Wang wrote:
>>> I think Artem is correct here.
>>> 
>>> All those tests that modify static fields should run in othervm mode, so
>>> that *each* runs in its *own* VM.  All other tests, which make no such
>>> modifications, can share the *same* VM using agentvm.
>> It's too strong to follow.  And it is not easy to know there is no static 
>> fields update in a test case.
>> 
>> Xuelei
>> 
 On 8/25/2016 13:31, Xuelei Fan wrote:
> On 8/25/2016 12:31 PM, Artem Smotrakov wrote:
> Those tests which modify default JSSE parameters should be run in
> othervm mode. But other tests which don't do that can be safely run in
> agent VM. This test doesn't seem to modify any default JSSE parameter.
 If this test get booted, other test that need to do more customization
 may not work any more.
 
 For example, boolean variable A is a singleton static field, default is
 true.  If A get initialized in default mode, it is "true".  It is not
 possible to customize it to "false" any more in other test in
 agentvm/samevm mode.
>>> 
>>> Shouldn't these "other" tests run in othervm mode?
>>> 
>>> Or, do you mean that even if a JSSE test does not modify a static field
>>> inside the test, it will modify some through innocent JSSE calls, so no
>>> JSSE test should ever run in agentvm?
>>> 
>>> I find this unnatural.
>>> 
>>> Thanks
>>> Max
>>> 
 
 This test has no impact in agentvm/samevm mode unless no test run in
 agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
 configuration as this one.   This assumption is too strong to follow.
 
 Xuelei
 
> Artem
> 

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Artem Smotrakov]

Hi Xuelei,

I am not sure why you think it's too strong to follow to it. To me it's 
pretty easy to see if a test calls System.setProperty() or 
Security.setProperty() methods which modify default JSSE configuration.


Is there any other way to modify default JSSE configuration, so that it 
can't be changes then?


Artem


On 08/25/2016 12:43 AM, Xuelei Fan wrote:

On 8/25/2016 2:06 PM, Weijun Wang wrote:

I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no 
static fields update in a test case.


Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:

Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.


If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.


Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max



This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the 
same

configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Xuelei Fan]

Just a quick reply.

1. The close is necessary.
2. Run in othervm is safer.  Please see my comments in other replies.

If you still have questions, I will reply with more details tomorrow when I 
work on a PC.

Xuelei



> On Aug 25, 2016, at 11:00 PM, Svetlana Nikandrova 
>  wrote:
> 
> Hi Artem, Xuelei,
> 
> thank you for your comments. Please see inline.
> 
>> On 25.08.2016 4:10, Xuelei Fan wrote:
>> 
>> 
>>> On 8/25/2016 3:55 AM, Artem Smotrakov wrote: 
>>> Hi Svetlana, 
>>> 
>>> Thank you for cleaning up this test. I have a couple of comments (mostly 
>>> about the original test). 
>>> 
>>> 1. I see that the test tries to connect to a server three times, but the 
>>> server accept only first connection, and then it stops. So test cases 
>>> #2-3 fail just because the connection was refused. The original test 
>>> behaves like this. This looks like a bug to me. What do you think? 
>>> Should the server have a loop of three iterations?
>> I think it's the purpose to test behavior of the close server socket. 
> 
> Yes, the test checks that if connection was closed by server during handshake 
> client will get exceptions from handshake, read and write. We don't need to 
> connect 3 times to check it.
>   
>> 
>>> 2. Here is server's code: 
>>> 
>>>   95 @Override 
>>>   96 public void run() { 
>>>   97 try (Socket s = serverSocket.accept()) { 
>>>   98 System.out.println("Server accepted connection"); 
>>>   99 // wait a bit before closing the socket to give 
>>>  100 // the client time to send its hello message 
>>>  101 Thread.currentThread().sleep(100); 
>>>  102 s.close(); 
>>>  103 System.out.println("Server closed socket, done."); 
>>>  104 } catch (Exception e) { 
>>>  105 throw new RuntimeException("Problem in test 
>>> execution", e); 
>>>  106 } 
>>>  107 } 
>>> 
>>> Not sure if it is a good assumption to expect that ClientHello is 
>>> received in 100 milliseconds. It might read first data, and then close 
>>> the socket. It also doesn't seem to be necessary to call close() there.
> Yes, you are right. Calling close() is unnecessary in try-with-resource 
> block. Removed.
>> It is not expected to perform handshaking for this test.  Get connection, 
>> and immediately close the socket before handshaking, and then see what 
>> happens in client side (connect/read/write). 
>> 
>> I have the same concern that 100 MS may be an issue.  Please consider to 
>> make an improvement. 
> I also not totally happy with 100 MS assumption. As Artem suggested I've 
> created separate issue for that improvement: JDK-8164804 
>> 
>> 
>> BTW, please run the test in othervm mode. 
> 
> I must admitted I'm confused. AFAIK othervm is forced for tests that change 
> configuration. This test uses default one.  
> It won't kill me to add it to this test but I'd be grateful if we could 
> clarify this for future testdev.
> 
> Thanks,
> Svetlana 
> 
>> 
>> Xuelei 
>> 
>>> Otherwise, the webrev looks good to me, but please note that I am not an 
>>> official reviewer. You may want to fix the issues above, or we can just 
>>> file a new bug. 
>>> 
>>> Artem 
>>> 
 On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote: 
 Hello, 
 
 please review this test bug fix. Test failed because of staled threads 
 left after execution. 
 Added try-with-resources statements to make sure test closes it's 
 resources. Also as test is overall quite old-fashioned I've done some 
 refactoring (hope now it looks better). 
 
 JBS: 
 https://bugs.openjdk.java.net/browse/JDK-8164533 
 Webrev: 
 http://cr.openjdk.java.net/~snikandrova/8164533/webrev.00/ 
  
 
 Thank you, 
 Svetlana 
> 

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Svetlana Nikandrova]

Hi Artem, Xuelei,

thank you for your comments. Please see inline.

On 25.08.2016 4:10, Xuelei Fan wrote:



On 8/25/2016 3:55 AM, Artem Smotrakov wrote:

Hi Svetlana,

Thank you for cleaning up this test. I have a couple of comments (mostly
about the original test).

1. I see that the test tries to connect to a server three times, but the
server accept only first connection, and then it stops. So test cases
#2-3 fail just because the connection was refused. The original test
behaves like this. This looks like a bug to me. What do you think?
Should the server have a loop of three iterations?


I think it's the purpose to test behavior of the close server socket.


Yes, the test checks that if connection was closed by server during 
handshake client will get exceptions from handshake, read and write. We 
don't need to connect 3 times to check it.





2. Here is server's code:

  95 @Override
  96 public void run() {
  97 try (Socket s = serverSocket.accept()) {
  98 System.out.println("Server accepted connection");
  99 // wait a bit before closing the socket to give
 100 // the client time to send its hello message
 101 Thread.currentThread().sleep(100);
 102 s.close();
 103 System.out.println("Server closed socket, done.");
 104 } catch (Exception e) {
 105 throw new RuntimeException("Problem in test
execution", e);
 106 }
 107 }

Not sure if it is a good assumption to expect that ClientHello is
received in 100 milliseconds. It might read first data, and then close
the socket. It also doesn't seem to be necessary to call close() there.

Yes, you are right. Calling close() is unnecessary in try-with-resource 
block. Removed.
It is not expected to perform handshaking for this test.  Get 
connection, and immediately close the socket before handshaking, and 
then see what happens in client side (connect/read/write).


I have the same concern that 100 MS may be an issue.  Please consider 
to make an improvement.
I also not totally happy with 100 MS assumption. As Artem suggested I've 
created separate issue for that improvement: JDK-8164804 



BTW, please run the test in othervm mode.


I must admitted I'm confused. AFAIK othervm is forced for tests that 
change configuration. This test uses default one.
It won't kill me to add it to this test but I'd be grateful if we could 
clarify this for future testdev.


Thanks,
Svetlana



Xuelei


Otherwise, the webrev looks good to me, but please note that I am not an
official reviewer. You may want to fix the issues above, or we can just
file a new bug.

Artem

On 08/24/2016 11:21 AM, Svetlana Nikandrova wrote:

Hello,

please review this test bug fix. Test failed because of staled threads
left after execution.
Added try-with-resources statements to make sure test closes it's
resources. Also as test is overall quite old-fashioned I've done some
refactoring (hope now it looks better).

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


Thank you,
Svetlana

Re: RFR: 8151893: Add security property to configure XML Signature secure validation mode

[Xuelei Fan]

On 8/25/2016 10:14 PM, Sean Mullan wrote:

On 08/25/2016 09:47 AM, Xuelei Fan wrote:

http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.01/


Looks fine to me except the following minor comment.

java.security
-
 818 #   AlgConstraint
 819 #   "disallowAlg" Uri
 ...
 829 # For AlgConstraint, Uri is the algorithm URI String that is not
allowed.

The "disallowAlg" has said the same thing as line 829.  As you did not
explain other options, may be this one can also be removed.  Minor
comment.


I thought this one deserved a little bit of explanation, since unlike
our other JCE APIs, XML Signature algorithms are specified as URIs and
not Strings like "MD5", so I wanted to avoid confusion with the other
properties that take algorithm Strings. I'd like to leave this one in.


It's OK.

Xuelei

Re: RFR: 8151893: Add security property to configure XML Signature secure validation mode

[Sean Mullan]

On 08/25/2016 09:47 AM, Xuelei Fan wrote:

http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.01/


Looks fine to me except the following minor comment.

java.security
-
 818 #   AlgConstraint
 819 #   "disallowAlg" Uri
 ...
 829 # For AlgConstraint, Uri is the algorithm URI String that is not
allowed.

The "disallowAlg" has said the same thing as line 829.  As you did not
explain other options, may be this one can also be removed.  Minor comment.


I thought this one deserved a little bit of explanation, since unlike 
our other JCE APIs, XML Signature algorithms are specified as URIs and 
not Strings like "MD5", so I wanted to avoid confusion with the other 
properties that take algorithm Strings. I'd like to leave this one in.


As previously mentioned, the plan is still to add a section to one of 
our user guides which will go into more details on what each restriction 
means.



Policy.java
---
  73   StringTokenizer st = new StringTokenizer(entry);

StringTokenizer is a legacy class per its specification, may be better
to use String.split:

  String[] st = entry.split("\\s");


Ok.


 103   disallowedRefUriSchemes.add(scheme.toLowerCase());
Better to use toLowerCase(Locale.ENGLISH).  There are issues to use
toUpperCase/toLowerCase when comparing two case-insensitive strings. See
JDK-6972387.


Ok.


The same comment for other use of toLowerCase().


Ok.

Thanks,
Sean

Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Sean Coffey]

Looks good Brad. One comment :

You might hit an NPE here but I guess the Exception handling in the 
parent call would handle it :


JceSecurity.java :
 254 if (Paths.get(cryptoPolicyProperty).getNameCount() != 1) {

Maybe add a null check to be cleaner ?

I missed the "crypto.policy=crypto.policydir-tbd" approach in 1st 
revision. Isn't it going to confuse people studying the code ? This 
value gets replaced at build time - right ? What about changing it to 
something like :

"crypto.policy=$crypto.policydir-tbd //replaced at build time"

and having the make files search for that string instead ? I think it 
makes the intent more obvious.


Regards,
Sean.

On 25/08/2016 01:21, Bradford Wetmore wrote:

Max/SeanC/SeanM,

The latest update:

http://cr.openjdk.java.net/~wetmore/8061842/webrev.02/

On 8/17/2016 9:26 PM, Wang Weijun wrote:
Before this change, you require default policy in neither export nor 
import to be empty but do not care about the getMinimum() result. In 
this change, you make sure the final result is not empty. I assume 
this is a fix?


I made the change to allow for our traditional (default) export/import 
mechanism, but other additional styles could be added/used.  Since we 
no longer sign, distros are free to edit, add and/or remove files.  
But before doing any JCE operation, the environment needs to grant 
something, otherwise there are no perms and no JCE available.



283 // Did we find a default perms?

What does this line mean?


I've moved to the right position in the file.  I meant did we find a 
default perms file, vs an exempt.



296 // This should never happen

But you can still print out the file name.


I'm concerned that the exception might print out the entire path 
instead of just the filename, which would include java.home and 
probably should not be made available.


Can you rename policydir-tbd to something else? I am afraid it will 
be confused with policy.url.1 etc.


Changed to:  crypto.policydir-tbd?

The original README.TXT in unlimited says "are exportable from the 
United States." and you have "is exportable." now. Is this intended? 
(IANAL)


Changed.


TestUnlimited.java:
45 "// Use the AES are the test Cipher", you mean "Use AES as the 
test Cipher"?
51 "throw new Exception ("Unlimited policy is NOT active");". No 
space before "(".


Fixed.

Sean Mullan wrote:

 > What about setting the default value to "limited"? And then this
 > would only be changed to "unlimited" if the build --enable-unlimited-
 > crypto option is specified?

I could, but I'm concerned that a build with 
--enabled-unlimited-crypto would expect that the compiled-in version 
default would also be unlimited and would be surprised with limited.


Upon Max's suggestion above, I've changed the name of the marker to 
"crypto.policy=crypto.policydir-tbd."  Does that work for you?


 > Instead of throwing an exception here, I wonder if it would make more
 > sense to assume a default value of "limited" if the property is not
 > set or is empty.

We could, but see above.

Sean Coffey wrote:

> Please include the exception 'e' in your last exception here.

Again, I'm concerned about outputting java.home, so I'm just going to 
output the final directory name.


> 3. Test case.
>
> The TestUnlimited.java testcase seems to be lacking. Do you want to
> test other values for crypto.policy ? 'limited' would be one.
> Throwing in some dummy value would also be good so that the exception
> handling code gets exercised.

Done.

 * @run main/othervm TestUnlimited limited fail
 * @run main/othervm TestUnlimited unlimited pass
 * @run main/othervm TestUnlimited NosuchDir exception
 * @run main/othervm TestUnlimited . exception
 * @run main/othervm TestUnlimited /tmp/unlimited exception
 * @run main/othervm TestUnlimited ../policy/unlimited exception
 * @run main/othervm TestUnlimited ./unlimited exception

> It needs to be run in ovm mode since you're setting a Security
> property.

Yes, good catch.

Brad

Re: RFR: 8151893: Add security property to configure XML Signature secure validation mode

[Xuelei Fan]

>> http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.01/

Looks fine to me except the following minor comment.

java.security
-
 818 #   AlgConstraint
 819 #   "disallowAlg" Uri
 ...
 829 # For AlgConstraint, Uri is the algorithm URI String that is not 
allowed.


The "disallowAlg" has said the same thing as line 829.  As you did not 
explain other options, may be this one can also be removed.  Minor comment.



Policy.java
---
  73   StringTokenizer st = new StringTokenizer(entry);

StringTokenizer is a legacy class per its specification, may be better 
to use String.split:


  String[] st = entry.split("\\s");


 103   disallowedRefUriSchemes.add(scheme.toLowerCase());
Better to use toLowerCase(Locale.ENGLISH).  There are issues to use 
toUpperCase/toLowerCase when comparing two case-insensitive strings. See 
JDK-6972387.


The same comment for other use of toLowerCase().


Xuelei

Re: RFR: 8151893: Add security property to configure XML Signature secure validation mode

[Sean Mullan]

On 08/24/2016 10:25 PM, Xuelei Fan wrote:

On 8/25/2016 7:57 AM, Sean Mullan wrote:

I posted an updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.0


I guess the link should be:

http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.01/


Yes, thanks.

--Sean



Xuelei


I found an existing bug in the dsig implementation and wanted to fix it
with this change. There are 2 ways to register an element's ID
attributes:

1. Using javax.xml.crypto.dom.DOMCryptoContext.setIdAttributeNS()
2. Using one of the org.w3c.dom.Element.setIdAttribute*() methods

The DOMURIDereferencer needs to check both mechanisms when searching for
Id attributes (it was only checking the first). Fix has been applied to
lines 91-96 of DOMURIDereferencer in the updated webrev.

--Sean

On 08/24/2016 03:17 PM, Sean Mullan wrote:

Please review this fix to add a new security property that allows you to
configure the individual restrictions that are enabled by the XML
Signature secure validation mode.

bug: https://bugs.openjdk.java.net/browse/JDK-8151893
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8151893/webrev.00/

Thanks,
Sean

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Xuelei Fan]

On 8/25/2016 2:06 PM, Weijun Wang wrote:

I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so
that *each* runs in its *own* VM.  All other tests, which make no such
modifications, can share the *same* VM using agentvm.

It's too strong to follow.  And it is not easy to know there is no 
static fields update in a test case.


Xuelei


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:

Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.


If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.


Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field
inside the test, it will modify some through innocent JSSE calls, so no
JSSE test should ever run in agentvm?

I find this unnatural.

Thanks
Max



This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem

Re: RFR 8164533: [TEST_BUG] sun/security/ssl/SSLSocketImpl/CloseSocket.java failed with "Error while cleaning up threads after test"

[Weijun Wang]

I think Artem is correct here.

All those tests that modify static fields should run in othervm mode, so 
that *each* runs in its *own* VM. All other tests, which make no such 
modifications, can share the *same* VM using agentvm.


On 8/25/2016 13:31, Xuelei Fan wrote:

On 8/25/2016 12:31 PM, Artem Smotrakov wrote:

Those tests which modify default JSSE parameters should be run in
othervm mode. But other tests which don't do that can be safely run in
agent VM. This test doesn't seem to modify any default JSSE parameter.


If this test get booted, other test that need to do more customization
may not work any more.

For example, boolean variable A is a singleton static field, default is
true.  If A get initialized in default mode, it is "true".  It is not
possible to customize it to "false" any more in other test in
agentvm/samevm mode.


Shouldn't these "other" tests run in othervm mode?

Or, do you mean that even if a JSSE test does not modify a static field 
inside the test, it will modify some through innocent JSSE calls, so no 
JSSE test should ever run in agentvm?


I find this unnatural.

Thanks
Max



This test has no impact in agentvm/samevm mode unless no test run in
agentvm/samevm mode or all tests run in agentvm/samevm mode has the same
configuration as this one.   This assumption is too strong to follow.

Xuelei


Artem