Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

[Seán Coffey]

Hi Valerie,

many thanks for the thorough review. I've taken all your feedback on 
board with the latest push. Some of the test anomalies were a result of 
previous iterations of test edits I had been making.


Regarding the extra edits in 
"src/java.base/share/lib/security/default.policy", I had assumed it 
would be ok to tidy up the module under edit but I've reverted the 
unrelated changes now.


I was doubtful about removing the AccessController.doPrivileged blocks 
around the InnocuousThread.newSystemThread calls given that I wasn't 
sure which path the calling code would come from but on re-examination, 
I think it's ok to remove. The module provides the necessary permissions 
already and use of InnocuousThread solves the original permissions 
issue. Thanks for spotting!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


Just curious, why split the loop into 2 and set the SecurityManager in 
between the two loops? Can't we just set the policy/security manager 
before the loop?
The test infra requires various permissions including the problem 
setContextClassLoader permission. I figured it was better to set up the 
test infra first and then trigger the security manager.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:


Sufficient permissions missing if this code was ever to run with 
SecurityManager.

Cleanest approach appears to be use of InnocuousThread to create the 
cleaner/poller threads.
Test case coverage extended to cover the SecurityManager scenario.

Reviewer request: @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:


129: permission java.lang.RuntimePermission 
"accessClassInPackage.com.sun.crypto.provider";
130: permission java.lang.RuntimePermission 
"accessClassInPackage.jdk.internal.misc";
131: permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:


140: permission java.security.SecurityPermission 
"clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission 
"getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:


950: AccessController.doPrivileged((PrivilegedAction) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:


954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:


1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:


1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:


1210:
1211: this.token = token;
1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:


54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }

Move this if-check block of code up before the for-loop?

-

PR: 

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads

[Seán Coffey]

Thank for the feedback Peter. Comments inline.

On 22/06/2021 22:40, Peter Firmstone wrote:

Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a 
configuration file with the SunPKCS11 Provider by default - We have 
tests to register Providers while SecurityManager is in place. This 
failed for SunPKCS11.


When you see an AccessControlException, I'd recommend setting the 
following security debug property, so you can capture the 
ProtectionDomain that failed the access check: 
-Djava.security.debug=access:failure  Clearly there's a 
ProtectionDomain on the calling threads stack that doesn't have the 
necessary permission.  If you knew which one it was, you could just 
grant it java.lang.RuntimePermission "setContextClassLoader" 
permission in the policy file.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki 
ProtectionDomain lacks the permission and rightly so IMO. The default 
policy doesn't grant "setContextClassLoader" permission to any JDK 
module. It's not required when we use InnocuousThread.


In NativeResourceCleaner the original constructor is setting the 
Context ClassLoader of the calling thread to null, prior to calling 
the Thread superclass constructor, so that both the calling thread and 
new thread will nave a null context ClassLoader.  In your new 
implementation, you are asserting the context class loader of the 
created thread is null, without setting the context ClassLoader of the 
original calling thread, is that the intended behaviour?


Alternatively you could set the context ClassLoader of the calling 
thread to null using a PrivilegedAction, prior to creating the new 
thread and restore it?
Use of InnocuousThread is made in various JDK classes for similar 
purpose where daemon threads need to be run with limited privilege. 
Similar use seen in networking and ldap classes.




If the original intent was to set the context ClassLoader of the new 
thread to null, then why not just do that, rather than use an assertion?
InnocuousThread sets this to null. The assert is just a belt and braces 
approach which is a useful check during test runs. Again, similar 
approach done in other JDK libraries.


If assertions are not enabled it may run with a non null context 
ClassLoader?   What are the consequences?  It appears to me, the fix 
has created a bigger problem, rather than fixed it.  Just my 2 cents.


see above. We shouldn't have an issue. A non-null classloader would lead 
to classloader memory leak in some environments.


regards,
Sean.



We use SecurityManager by default following principles of least 
privilege (only the code paths we need to execute), and the original 
reported bug is a non problem for us, we would capture the missing 
permission and grant it, these are permission grants for Java 16:


grant codebase "jrt:/jdk.crypto.cryptoki"
{
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";

};

grant codebase "jrt:/jdk.crypto.ec"
{
    permission java.security.SecurityPermission 
"putProviderProperty.SunEC";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.jca";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.pkcs";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math.intpoly";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.x509";

};

Good call making NativeResourceCleaner implement Runnable instead of 
extending Thread though.



[1]

access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki 
)

 jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
 
 java.security.Permissions@7006c658 (
 ("java.io.FilePermission" "<>" "read")
 ("java.net.SocketPermission" "localhost:0" "listen,resolve")
 ("java.security.SecurityPermission" "clearProviderProperties.*")
 ("java.security.SecurityPermission" 
"getProperty.auth.login.defaultCallbackHandler")

 ("java.security.SecurityPermission" "putProviderProperty.*")
 ("java.security.SecurityPermission" "authProvider.*")
 ("java.security.SecurityPermission" "removeProviderProperty.*")
 ("java.util.PropertyPermission" "java.specification.version" "read")
 ("java.util.PropertyPermission" "java.vm.vendor" "read")
 ("java.util.PropertyPermission" "path.separator" "read")
 ("java.util.PropertyPermission" "os.version" "read")
 ("java.util.PropertyPermission" "java.vendor.url" "read")
 ("java.util.PropertyPermission" "java.vm.name" "read")
 ("java.util.PropertyPermission" "java.vm.specification.version" "read")
 ("java.util.PropertyPermission" "os.name" "read")
 ("java.util.PropertyPermission" 
"sun.security.pkcs11.allowSingleThreadedModules" "read")
 ("java.util.PropertyPermission" 

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Thanks for the review Hai-May. I've implemented all your suggestions.

The CSR was approved late on Friday so I'll now submit this via PR on 
github infra.


regards,
Sean.

On 28/08/2020 21:08, Hai-May Chao wrote:
JarSigner.java #953: The output debug message can be removed from the 
code.

JavaUtilZipFileAccess.java #44: Change posixPerms to extraAttrs.
ZipFile.java #661: Suggest to keep the comment and update it with the 
additional 4 bits for symlink.


The rest of code changes and CSR look good.

Thanks,
Hai-May


On Aug 28, 2020, at 7:17 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


I've been poking around the zip internals and am now able to locate 
the 16 bits of interest. The position of these actual bits does 
appear to move around from one test run to another. For now, I guess 
it's sufficient to look for the pattern of interest in the signed zip 
file. New testcase added.


http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.

As long as that*byte*  is there and it’s not difficult to locate, we can 
manually add the*bit*  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: 8249694 - [TestBug] java/lang/StringBuffer/HugeCapacity.java and j/l/StringBuilder/HugeCapacity.java tests shouldn't be @ignore-d

[Seán Coffey]

Looks fine to me.

regards,
Sean.

On 03/09/2020 11:40, Fernando Guallini wrote:

Hi Sean,
Right, it also applies for these tests, changes:
--- a/test/jdk/java/lang/StringBuffer/HugeCapacity.java
+++ b/test/jdk/java/lang/StringBuffer/HugeCapacity.java
- * @requires os.maxMemory >= 6G
+ * @requires (sun.arch.data.model == "64" & os.maxMemory >= 6G)
--- a/test/jdk/java/lang/StringBuilder/HugeCapacity.java
+++ b/test/jdk/java/lang/StringBuilder/HugeCapacity.java
- * @requires os.maxMemory >= 6G
+ * @requires (sun.arch.data.model == "64" & os.maxMemory >= 6G)

Regards,
Fernando

On 1 Sep 2020, at 17:25, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Wouldn't you require the sun.arch.data.model == "64" jtreg config in 
these tests also ?


regards,
Sean.

On 28/08/2020 19:13, Fernando Guallini wrote:








Hi,

May I please get reviews and a sponsor for this trivial change:

webrev: http://cr.openjdk.java.net/~fguallini/8249694/webrev.00/
Testbug: https://bugs.openjdk.java.net/browse/JDK-8249694

Tests do not need to have ‘@ignore' because with @requires 
os.maxMemory is enough to ensure they will not be executed if memory 
requirements are not satisfied. They run in Mach5 with no issues.


Thanks

-Fernando

Re: RFR: 8249694 - [TestBug] java/lang/StringBuffer/HugeCapacity.java and j/l/StringBuilder/HugeCapacity.java tests shouldn't be @ignore-d

[Seán Coffey]

Wouldn't you require the sun.arch.data.model == "64" jtreg config in 
these tests also ?


regards,
Sean.

On 28/08/2020 19:13, Fernando Guallini wrote:








Hi,

May I please get reviews and a sponsor for this trivial change:

webrev: http://cr.openjdk.java.net/~fguallini/8249694/webrev.00/
Testbug: https://bugs.openjdk.java.net/browse/JDK-8249694

Tests do not need to have ‘@ignore' because with @requires os.maxMemory is 
enough to ensure they will not be executed if memory requirements are not 
satisfied. They run in Mach5 with no issues.

Thanks

-Fernando

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

On 28/08/2020 16:18, Weijun Wang wrote:

1. Add a comment on how to generate ZIPBYTES in the test. Not the byte[] 
declaration but how the original ZIP file is generated.

I'll add a comment block to the test:

    /*
 * Created using the createByteArray utility method.
 * The zipfile itself was created via this example:
 * $ ls -l z
 * lrwxrwxrwx 1 test test 4 Aug 27 18:33 z -> ../z
 * $ zip -ry test.zip z
 */




2. Does this require a CSR? The POSIX permission one had one.


Fair point. I've logged one, just to be safe.

regards,
Sean.



Thanks,
Max


On Aug 28, 2020, at 10:17 AM, Seán Coffey  wrote:

I've been poking around the zip internals and am now able to locate the 16 bits 
of interest. The position of these actual bits does appear to move around from 
one test run to another. For now, I guess it's sufficient to look for the 
pattern of interest in the signed zip file. New testcase added.

http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.


As long as that *byte* is there and it’s not difficult to locate, we can 
manually add the *bit*
  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: JDK-8249699: java/io/ByteArrayOutputStream/MaxCapacity.java should use @requires instead of @ignore

[Seán Coffey]

Apologies. Meant to reply yesterday. Your edit looks fine to me.

regards,
Sean.

On 27/08/2020 16:41, Fernando Guallini wrote:

Thanks Sean, updated webrev here: 
http://cr.openjdk.java.net/~fguallini/8249699/webrev.01/

Regards,
Fernando
- Original Message -
From: sean.cof...@oracle.com
To: fernando.guall...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Wednesday, 26 August, 2020 7:39:25 PM GMT +00:00 GMT Britain, Ireland, 
Portugal
Subject: Re: RFR: JDK-8249699: java/io/ByteArrayOutputStream/MaxCapacity.java 
should use @requires instead of @ignore

test/jdk/java/util/Base64/TestEncodingDecodingLength.java is an example
of another test using -Xmx8g. Do you want to push the os.maxMemory
requirement up to 10g perhaps ? It might avoid border line resource
failures. Also I think it might need a "sun.arch.data.model == "64" "
requirement :

@requires (sun.arch.data.model == "64" & os.maxMemory >= 10g)

regards,
Sean.

On 26/08/2020 18:17, Fernando Guallini wrote:

Hi,

Could I please get reviews and a sponsor for:

webrev: http://cr.openjdk.java.net/~fguallini/8249699/webrev.00/ 

Testbug: https://bugs.openjdk.java.net/browse/JDK-8249699 


The test was ignored due to ‘huge memory requirements’, but with the jtreg 
current version, tests can be only run when system requirements are satisfied 
as opposed to being always excluded. It runs and passes in Mach5


Thanks

-Fernando

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

I've been poking around the zip internals and am now able to locate the 
16 bits of interest. The position of these actual bits does appear to 
move around from one test run to another. For now, I guess it's 
sufficient to look for the pattern of interest in the signed zip file. 
New testcase added.


http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.

As long as that*byte*  is there and it’s not difficult to locate, we can 
manually add the*bit*  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Thanks for the review Max. Comments inline..

On 27/08/2020 14:45, Weijun Wang wrote:

I’m OK with using one warning, but prefer it to a little more formal like 
"POSIX file permission and/or symlink attributes detected…”.

One nit in ZipFile.java:

1098 // only set posix perms value via ZipEntry constructor for 
now
1099 @Override
1100 public int getExtraAttributes(ZipEntry ze) {

Maybe you can just remove the comment.

Do you also want to rename the “posixPermsDetected" field and loacl variable 
“perms” in JarSigner.java?


Good points. Edits made.

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



I’m not sure about the test but if zipfs is able to keep permissions inside a 
zip file then that POSIX byte (or whatever it’s named) is already there and we 
can modify it to include more bits.


Looks like it was a conscious design decision to only allow recording of 
POSIX permission bits for this field (& 0xFFF). I don't see anything 
about symlink support in zipfs docs.


regards,
Sean.



No other comment.

Thanks,
Max



On Aug 27, 2020, at 3:26 AM, Seán Coffey  wrote:

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

regards,
Sean.

On 27/08/2020 07:42, Seán Coffey wrote:

Hi Max,

I looked at updating the warning string but figured that it might have been of 
no interest to end users. How about this edit then ?

+{"posix.attributes.detected", "POSIX file permission attributes detected. 
These attributes are ignored when signing and are not protected by the signature."},


replace with:

+{"extra.attributes.detected", "POSIX file permission/symlink attributes 
detected. These attributes are ignored when signing and are not protected by the signature."},

regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max


On Aug 26, 2020, at 2:26 PM, Seán Coffey  wrote:

This is a follow on from the recent 8218021 fix. The jarsigner tool removes 
symlink attribute data from zipfiles when signing them. jarsigner should 
preserve this data. The fix involves preserving the 16 bits associated with the 
file attributes (instead of the current 12). That's done in ZipFile. All other 
changes are just a refactor of the variable name.

I haven't been able to automate a test for this since zipfs doesn't seem to 
support symlinks. Manual testing looks good.

https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

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

regards,
Sean.

On 27/08/2020 07:42, Seán Coffey wrote:

Hi Max,

I looked at updating the warning string but figured that it might have 
been of no interest to end users. How about this edit then ?


+    {"posix.attributes.detected", "POSIX file permission 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


>> replace with:
+    {"extra.attributes.detected", "POSIX file permission/symlink 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max

On Aug 26, 2020, at 2:26 PM, Seán Coffey  
wrote:


This is a follow on from the recent 8218021 fix. The jarsigner tool 
removes symlink attribute data from zipfiles when signing them. 
jarsigner should preserve this data. The fix involves preserving the 
16 bits associated with the file attributes (instead of the current 
12). That's done in ZipFile. All other changes are just a refactor 
of the variable name.


I haven't been able to automate a test for this since zipfs doesn't 
seem to support symlinks. Manual testing looks good.


https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html 



regards,
Sean.

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Hi Max,

I looked at updating the warning string but figured that it might have 
been of no interest to end users. How about this edit then ?


+    {"posix.attributes.detected", "POSIX file permission attributes 
detected. These attributes are ignored when signing and are not 
protected by the signature."},


>> replace with:
+    {"extra.attributes.detected", "POSIX file permission/symlink 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max


On Aug 26, 2020, at 2:26 PM, Seán Coffey  wrote:

This is a follow on from the recent 8218021 fix. The jarsigner tool removes 
symlink attribute data from zipfiles when signing them. jarsigner should 
preserve this data. The fix involves preserving the 16 bits associated with the 
file attributes (instead of the current 12). That's done in ZipFile. All other 
changes are just a refactor of the variable name.

I haven't been able to automate a test for this since zipfs doesn't seem to 
support symlinks. Manual testing looks good.

https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

Re: RFR: JDK-8249699: java/io/ByteArrayOutputStream/MaxCapacity.java should use @requires instead of @ignore

[Seán Coffey]

test/jdk/java/util/Base64/TestEncodingDecodingLength.java is an example 
of another test using -Xmx8g. Do you want to push the os.maxMemory 
requirement up to 10g perhaps ? It might avoid border line resource 
failures. Also I think it might need a "sun.arch.data.model == "64" " 
requirement :


@requires (sun.arch.data.model == "64" & os.maxMemory >= 10g)

regards,
Sean.

On 26/08/2020 18:17, Fernando Guallini wrote:

Hi,

Could I please get reviews and a sponsor for:

webrev: http://cr.openjdk.java.net/~fguallini/8249699/webrev.00/ 

Testbug: https://bugs.openjdk.java.net/browse/JDK-8249699 


The test was ignored due to ‘huge memory requirements’, but with the jtreg 
current version, tests can be only run when system requirements are satisfied 
as opposed to being always excluded. It runs and passes in Mach5


Thanks

-Fernando

RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

This is a follow on from the recent 8218021 fix. The jarsigner tool 
removes symlink attribute data from zipfiles when signing them. 
jarsigner should preserve this data. The fix involves preserving the 16 
bits associated with the file attributes (instead of the current 12). 
That's done in ZipFile. All other changes are just a refactor of the 
variable name.


I haven't been able to automate a test for this since zipfs doesn't seem 
to support symlinks. Manual testing looks good.


https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

Re: RFR: JDK-8249691: jdk/lambda/vm/StrictfpDefault.java file can be removed

[Seán Coffey]

Looks fine to me Evan.

regards,
Sean.

On 17/08/2020 16:25, Evan Whelan wrote:

Hi all,

  


This is a small fix that helps with some test cleanup. One redundant test file 
has been removed.

  


Webrev found at: http://cr.openjdk.java.net/~kravikumar/8249691/webrev/

  


Link to JBS issue: https://bugs.openjdk.java.net/browse/JDK-8249691

  


Any follow up questions are welcomed.

  


Thanks,

Evan

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks for the review Alan. I'm in contact with Max already about 
possible follow up enhancements in this area. It would be worked via a 
follow on JBS record.


Regarding the error message, I'm fine with your suggestion. We can go 
with this then:
"POSIX file permission attributes detected. These attributes are ignored 
when signing and are not protected by the signature."


regards,
Sean.

On 02/07/2020 08:59, Alan Bateman wrote:

On 30/06/2020 14:51, Seán Coffey wrote:


:

During the CSR review, a suggestion was made to have jarsigner 
preserve such attributes by default. Warnings about these attributes 
will also be added during signing and verify operations (if detected).


Yes, signing should be additive so the original proposal to drop 
information from the UNIX extra block would be surprising. The 
intersection of those using zip/other tools to create zip files and 
then signing them with jarsigner is probably small but it would still 
be confusing for signing to loose information. Having jarsigner refuse 
to sign these zip files by default, with an option to override, would 
be a reasonable approach. The current proposal to printing a warning 
seems okay too.


I've skimmed through webrev.8218021.v5 which has this warning:

"POSIX file permission attributes detected. Note that these attributes 
are unsigned and not protected by the signature."


I realize you've agreed this with the other Reviewers but I think that 
"Note that these attributes are unsigned ..." is confusing as it could 
be interpreted to mean that they have to be signed by some other 
means, or even that the warning is because they are using unsigned 
values.


It might be better to tweak the second part to make it a bit clearer, 
up to you but something like "These attributes are ignored when 
signing and are not protected by the signature".


-Alan

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks for the review Max. All edits made bar the 
"Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);" 
suggested edit. That's already in a finally block.


latest webrev: 
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v5/webrev/


I plan to push once I have a clean test run.

regards,
Sean.

On 01/07/2020 03:02, Weijun Wang wrote:

-
src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java
@@ -248,7 +248,7 @@ private static X509CRL getCRL(URIName name) throws 
CertStoreException {
 debug.println("Trying to fetch CRL from DP " + uri);
 }

 Event.report("event.crl.check", uri.toString());
 Event.report(Event.ReporterCategory.CRLCHECK,"event.crl.check", 
uri.toString());
  
Please add a whitespace after CRLCHECK.

-
src/java.base/share/classes/sun/security/provider/certpath/OCSP.java
@@ -234,7 +234,7 @@ static OCSPResponse check(List certIds, URI 
responderURI,
 debug.println("connecting to OCSP service at: " + url);
 }

 Event.report("event.ocsp.check", url.toString());
 Event.report(Event.ReporterCategory.CRLCHECK,"event.ocsp.check", 
url.toString());
  
White space after CRLCHECK.

-
src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
 int perms = JUZFA.getPosixPerms(ze);
 if (!posixPermsDetected && perms != -1) {
 posixPermsDetected = true;
 Event.report(Event.ReporterCategory.POSIXPERMS, "true");
  
The method signature is "report(category, type, values...)". There is no need to define a type or value here but I feel like more normal to call 'report(POSIXPERMS, "detected")' because the 2nd argument is type instead of value.

-
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1054,7 +1063,7 @@ private void displayMessagesAndResult(boolean isSigning) {
 notYetValidCert || chainNotValidated || hasExpiredCert ||
 hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0) ||
 (disabledAlg != 0) || aliasNotInStore || notSignedByAlias ||
 tsaChainNotValidated || permsDetected ||
  
I'd prefer move the permsDetected check and the if block for it out of this big block, since it's not a fatal warning and just informational. You can move it into "if (hasExpiringCert".

-
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1805,6 +1820,7 @@ void signJar(String jarName, String alias)
 fos.close();
 }

 Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);
  
Maybe in a final block?

-
test/jdk/sun/security/util/Resources/Usages.java
@@ -75,7 +75,7 @@
 "(?m)rb[ \\n]*\\.getString[ \\n]*\\([ \\n]*\"(.*?)\"\\)");

 static Pattern EVENT_OCSP_CRL = Pattern.compile(
 "Event\\.report\\(.*,\"(.*?)\",");
  
If you agree to add a whitespace in the calls in OSCP.java and DistributionFetcher.java, you might want to add a whitespace (or "\s*") here as well.


Thanks,
Max



On Jun 30, 2020, at 9:51 PM, Seán Coffey  wrote:

Thanks Lance.

During the CSR review, a suggestion was made to have jarsigner preserve such 
attributes by default. Warnings about these attributes will also be added 
during signing and verify operations (if detected).

webrev: https://cr.openjdk.java.net/~coffeys/webrev.8218021.v4/webrev/

regards,
Sean.

On 22/06/2020 17:17, Lance Andersen wrote:

HI Sean,

Looks OK based on our exchanges.  Thank you for your time on this one!

Best
Lance


On Jun 22, 2020, at 7:22 AM, Seán Coffey  wrote:

Thanks Lance.

I've updated the patch with some extra offline feedback from yourself and Max.
A new warning is printed with use of the new flag. A warning is also printed 
when file posix permissions are detected on resources being signed. Test 
updated for that also.

https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance


On Jun 12, 2020, at 6:21 AM, Seán Coffey  wrote:

Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've removed the 
problem references to zip file name extensions. Instead, there's a new JDK 
implementation specific jarsigner option: -keepposixperms

https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com






Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks Lance.

During the CSR review, a suggestion was made to have jarsigner preserve 
such attributes by default. Warnings about these attributes will also be 
added during signing and verify operations (if detected).


webrev: https://cr.openjdk.java.net/~coffeys/webrev.8218021.v4/webrev/

regards,
Sean.

On 22/06/2020 17:17, Lance Andersen wrote:

HI Sean,

Looks OK based on our exchanges.  Thank you for your time on this one!

Best
Lance

On Jun 22, 2020, at 7:22 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Thanks Lance.

I've updated the patch with some extra offline feedback from yourself 
and Max.
A new warning is printed with use of the new flag. A warning is also 
printed when file posix permissions are detected on resources being 
signed. Test updated for that also.


https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance

On Jun 12, 2020, at 6:21 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've 
removed the problem references to zip file name extensions. 
Instead, there's a new JDK implementation specific jarsigner 
option: -keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




 
<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>





<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Thanks Lance.

I've updated the patch with some extra offline feedback from yourself 
and Max.
A new warning is printed with use of the new flag. A warning is also 
printed when file posix permissions are detected on resources being 
signed. Test updated for that also.


https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance

On Jun 12, 2020, at 6:21 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've 
removed the problem references to zip file name extensions. Instead, 
there's a new JDK implementation specific jarsigner option: 
-keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've removed 
the problem references to zip file name extensions. Instead, there's a 
new JDK implementation specific jarsigner option: -keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

I've also created JDK-8238688 to track the specification clarification 
request. I'll start an RFR for that shortly.


Regards,
Sean.

On 07/02/20 14:23, Seán Coffey wrote:

I've introduced such a class: FactoryInitializationError

Also added a new simple testcase method check for case where this new 
exception would be exercised : testBadContextCall(Hashtable)


http://cr.openjdk.java.net/~coffeys/webrev.8223260.v4/webrev/ 
<http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223260.v4/webrev/>


Regards,
Sean.

On 07/02/20 10:45, Alan Bateman wrote:



On 07/02/2020 10:36, Peter Levart wrote:

:

Well, theoretically, some implementation of InitialContextFactory 
could, in its constructor, use a not well-behaved dynamic Proxy 
which could throw UndeclaredThrowableException with 
NoInitialContextException as a cause and such exception would get 
unwrapped later in NamingManager.getInitialContext() where it would 
mistakenly be identified as an exception constructed in 
NamingManager.getFactory method. Using your own sub-type of 
RuntimeException for tunneling the NoInitialContextException through 
the functional interface (lambda) would eliminate even this remote 
possibility. 
The concern is more that UndeclaredThrowableException is specified 
for handling exceptions on proxy interfaces. The question in one of 
the replies was whether there is a more appropriate exception to 
carry the checked exception. A static nested exception class would be 
fine.


-Alan

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

I've introduced such a class: FactoryInitializationError

Also added a new simple testcase method check for case where this new 
exception would be exercised : testBadContextCall(Hashtable)


http://cr.openjdk.java.net/~coffeys/webrev.8223260.v4/webrev/ 



Regards,
Sean.

On 07/02/20 10:45, Alan Bateman wrote:



On 07/02/2020 10:36, Peter Levart wrote:

:

Well, theoretically, some implementation of InitialContextFactory 
could, in its constructor, use a not well-behaved dynamic Proxy which 
could throw UndeclaredThrowableException with 
NoInitialContextException as a cause and such exception would get 
unwrapped later in NamingManager.getInitialContext() where it would 
mistakenly be identified as an exception constructed in 
NamingManager.getFactory method. Using your own sub-type of 
RuntimeException for tunneling the NoInitialContextException through 
the functional interface (lambda) would eliminate even this remote 
possibility. 
The concern is more that UndeclaredThrowableException is specified for 
handling exceptions on proxy interfaces. The question in one of the 
replies was whether there is a more appropriate exception to carry the 
checked exception. A static nested exception class would be fine.


-Alan

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

On 6 February 2020 21:13:52 GMT, Peter Levart  wrote:
>
>
>On 2/6/20 6:54 PM, Seán Coffey wrote:
>> the current proposal is still:
>> http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/
>
>But wasn't this one already better:
>
>https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/

D'oh! You're right Peter. v3 is my preference (and most up to date) at the 
moment. I'll look at Alan's comment in the morning. I could create a custom 
exception if the current UndeclaredThrowableException is not a good use.

Regards,
Sean.

>Or do you prefer the v2 so that spec change and behavior change would
>be 
>synchronized?
>
>
>Regards, Peter
>
>>
>> I'd like to make the specification change in a follow on bug ID (if 
>> that works for people)
>>
>> Regards,
>> Sean.
>>
>> On 06/02/20 17:49, Alan Bateman wrote:
>>> On 06/02/2020 15:32, Seán Coffey wrote:
>>>> InitialContextFactory itself is an extremely simple interface with 
>>>> one method. I've browsed some code bases and could only find a
>small 
>>>> number of such Factories implementations with simple logic to
>return 
>>>> a Context. Applications will most likely delegate to the same 
>>>> Factory over and over also (albeit, it's all controlled by
>HashTable 
>>>> parameters). The new caching logic should help memory pressure here
>
>>>> and not hinder it. I'm not seeing a major concern with current 
>>>> solution as a result.
>>> Okay, although I could imagine a non-JDK InitialContextFactory 
>>> implementation keeping a graph of objects alive. I saw your mail 
>>> about separating the clarification in the APIs docs so does it mean 
>>> the proposal on the table is the last webrev or is there a new
>version?
>>>
>>> -Alan
>>

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

the current proposal is still:
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v2/webrev/

I'd like to make the specification change in a follow on bug ID (if that 
works for people)


Regards,
Sean.

On 06/02/20 17:49, Alan Bateman wrote:

On 06/02/2020 15:32, Seán Coffey wrote:
InitialContextFactory itself is an extremely simple interface with 
one method. I've browsed some code bases and could only find a small 
number of such Factories implementations with simple logic to return 
a Context. Applications will most likely delegate to the same Factory 
over and over also (albeit, it's all controlled by HashTable 
parameters). The new caching logic should help memory pressure here 
and not hinder it. I'm not seeing a major concern with current 
solution as a result.
Okay, although I could imagine a non-JDK InitialContextFactory 
implementation keeping a graph of objects alive. I saw your mail about 
separating the clarification in the APIs docs so does it mean the 
proposal on the table is the last webrev or is there a new version?


-Alan

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

On 05/02/20 16:51, Daniel Fuchs wrote:

On 05/02/2020 15:31, Peter Levart wrote:
I think this is about allow the InitialContextFactory instance to be 
GC'ed when the thread is long lived. It might not be a concern for 
the LDAP or the other providers in the JDK.


-Alan


Ah, I see. You mean when the ClassLoader is long lived. So this is a 
normal matter of trying to release the InitialContextFactory and 
objects held by it when there is a memory pressure and not the matter 
of avoiding class loader leaks? In this case it would pay only if 
InitialContextFactory holds more memory than SoftReference itself...


Regards, Peter


Hi Peter,

Yes the concern is that the InitialContextFactory would be kept around
until the ClassLoader itself is GC'ed, even if the factory was used once
and not needed anymore. Could that become an issue in case the factory 
is loaded e.g. by the System ClassLoader?
But it's a good point that it might not be a concern if the 
InitialContextFactory instance in itself doesn't retain much memory.
InitialContextFactory itself is an extremely simple interface with one 
method. I've browsed some code bases and could only find a small number 
of such Factories implementations with simple logic to return a Context. 
Applications will most likely delegate to the same Factory over and over 
also (albeit, it's all controlled by HashTable parameters). The new 
caching logic should help memory pressure here and not hinder it. I'm 
not seeing a major concern with current solution as a result.


regards,
Sean.



best regards,

-- daniel

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Thanks again for the review Peter. There's an off-thread conversation 
around whether the ClassLoaderValue should hold SoftReferences to the 
Factory that's stored with the classloader. I think we're looking at a 
possible leak otherwise.

i.e. ClassLoaderValue>

I'm looking into that now.

Also - I'm hoping to port this to JDK 11u also so I might spin the 
specification changes into a different bug ID.


regards,
Sean.

On 03/02/2020 09:05, Peter Levart wrote:

Hi Seán,

On 2/1/20 12:22 AM, Seán Coffey wrote:

The following is also possible:

    // 1st try finding a ServiceLoader.Provider with type() 
of correct name

    factory = loader
    .stream()
    .filter(p -> p.type().getName().equals(className))
    .findFirst()
    .map(ServiceLoader.Provider::get)
    .or( // if that doesn't yield any result, 
instantiate the services
 // one by one and search for implementation 
class of correct name

    () -> loader
    .stream()
    .map(ServiceLoader.Provider::get)
    .filter(f -> 
f.getClass().getName().equals(className))

    .findFirst()
    ).orElse(null);

So what do you think?


ok - possible I guess but I think it's highly unlikely ? It looks 
like alot of extra care for a case that shouldn't happen. I'll stick 
with your earlier suggestion unless you or others object.


For the 3 InitialContextFactory implementations in JDK 
(DnsContextFactory, RegistryContextFactory, LdapCtxFactory), none uses 
the provider() static method convention, so for them the 
Provider.type()s are actually the same as their implementation 
classes. Should other InitialContextFactory service providers use the 
provider() static method convention (they may do this only if they are 
provided as Java modules I think), the InitialContextFactory sub-type 
name searched for in the NamingManager.getInitialContext() method is 
the provider type name, and not the implementation class name of the 
InitialContextFactory. They are usually the same, but in case of 
provider() static method convention, they may or may not be. This is 
not a problem for JDK supplied implementations and I don't think for 
any other current implementation. But anyway, I think this distinction 
should be spelled out in the specification of the 
NamingManager.getInitialContext() method and this is an opportunity to 
add some text for that. For example:


Index: src/java.naming/share/classes/javax/naming/spi/NamingManager.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===
--- src/java.naming/share/classes/javax/naming/spi/NamingManager.java 
(revision 57904:0905868db490c87df463258166762797374e5a96)
+++ src/java.naming/share/classes/javax/naming/spi/NamingManager.java 
(revision 57904+:0905868db490+)

@@ -644,7 +660,9 @@
  * 
  * First, the {@linkplain java.util.ServiceLoader 
ServiceLoader}

  * mechanism tries to locate an {@code InitialContextFactory}
- * provider using the current thread's context class 
loader
+ * provider for which the {@linkplain 
ServiceLoader.Provider#type()}
+ * returns a type with name equal to {@code 
Context.INITIAL_CONTEXT_FACTORY}
+ * environment property and using the current thread's 
context class loader
  * Failing that, this implementation tries to locate a 
suitable

  * {@code InitialContextFactory} using a built-in mechanism
  * 
@@ -662,7 +680,7 @@
  * @return A non-null initial context.
  * @exception NoInitialContextException If the
  *  {@code Context.INITIAL_CONTEXT_FACTORY} property
- * is not found or names a nonexistent
+ * is not found or names a nonexistent {@linkplain 
ServiceLoader.Provider#type()},

  * class or a class that cannot be instantiated,
  *  or if the initial context could not be created for 
some other

  *  reason.




current webrev: 
https://cr.openjdk.java.net/~coffeys/webrev.8223260.v3/webrev/




Otherwise, I think this webrev looks good now.


regards,
Sean. 


Regards, Peter

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Peter,

thanks again for your review. comments inline..

On 31/01/2020 17:16, Peter Levart wrote:

Hi Seán,

On 1/31/20 2:16 PM, Seán Coffey wrote:
Thanks for the review Peter. All good points! My latest patch 
contains adjustments based on feedback from you and others to date.


* Incorporate use of ClassLoaderValue -
 -- I'd appreciate feedback on whether I'm using it correctly.


Yes, you used it correctly. You are very verbose about using it, but 
that's just style. I, for example, would re-write this:


 712 ClassLoaderValue.Sub key = 
FACTORIES_CACHE.sub(className);

 713 try {
 714 factory = key.computeIfAbsent(loader, (ld, ky) -> {
 715 String cn = ky.key();
 716 InitialContextFactory fac = getFactory(cn);
 717 return fac;
 718 });
 719 } catch (UndeclaredThrowableException e) {
 720 if (e.getUndeclaredThrowable() instanceof 
NoInitialContextException) {
 721 throw (NoInitialContextException) 
e.getUndeclaredThrowable();

 722 }
 723 }

... into this:

    var key = FACTORIES_CACHE.sub(className);
    try {
    factory = key.computeIfAbsent(loader, (ld, ky) -> 
getFactory(ky.key()));

    } catch (UndeclaredThrowableException e) {
    if (e.getUndeclaredThrowable() instanceof 
NoInitialContextException) {
    throw (NoInitialContextException) 
e.getUndeclaredThrowable();

    } else {
    throw e;
    }
    }

Yes - looks much neater. I've edited the patch to that effect.


Notice also that I added:

    } else {
    throw e;
    }

You have two options. Either UndeclaredThrowableException is possible 
only when you wrap NoInitialContextException with it in getFactory() 
in which case you could simply do unconditional:


    } catch (UndeclaredThrowableException e) {
    throw (NoInitialContextException) 
e.getUndeclaredThrowable();

    }

...or UndeclaredThrowableException is possible also to be thrown from 
code called by getFactory() (in theory, it is). In this case you would 
want to re-throw it here instead of swallowing it.
Yes - I was wondering if I should be concerned about other call sites 
that might throw UndeclaredThrowableException. You're right, best to be 
on the safe side and re-throw. Code edited.



* Use of ServiceLoader.stream()


 737 factory = loader
 738 .stream()
 739 .map(ServiceLoader.Provider::get)
 740 .filter(f -> 
f.getClass().getName().equals(className))

 741 .findFirst()
 742 .orElse(null);

Here you instantiate InitialContextFactory instances until you get one 
with implementation class with correct className. But Provider.type() 
should already return the type of the service without instantiating 
it. So you could write the following instead:


    factory = loader
    .stream()
    .filter(p -> p.type().getName().equals(className))
    .findFirst()
    .map(ServiceLoader.Provider::get)
    .orElse(null);
That makes sense. It actually explains a test failure I was seeing 
earlier today while trying to expand test coverage for this issue. Off 
mail thread, Daniel Fuchs suggested I use a more concrete URLClassLoader 
example. I've introduced extra testing to test for multiple 
Context.INITIAL_CONTEXT_FACTORY values. I was getting unexpected 
initialization values since the stream function was instantiating 
DummyContextFactory for filter function (when in fact 
DummyContextFactory2 ended up being the correct Factory) . Thanks! I've 
adopted this change.


This should instantiate just the service that is found and not any 
other. ServiceLoader.Provider.type() is specified to return:


 * Returns the provider type. There is no guarantee that this 
type is
 * accessible or that it has a public no-args constructor. The 
{@link
 * #get() get()} method should be used to obtain the provider 
instance.

 *
 *  When a module declares that the provider class is 
created by a
 * provider factory then this method returns the return type 
of its

 * public static "{@code provider()}" method.

So in theory this method could return a super-type of the service 
implementation class. But one could argue that the name of the service 
provider type is the one we should be searching for, not the 
implementation class of the service. Perhaps the service declares a 
single provider type and then at instantiation time it dynamically 
chooses an implementation class depending on the environment 
(architecture perhaps). It would be interesting to see whether 
provider

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Thanks for the review Peter. All good points! My latest patch contains 
adjustments based on feedback from you and others to date.


* Incorporate use of ClassLoaderValue -
 -- I'd appreciate feedback on whether I'm using it correctly.
* Use of ServiceLoader.stream()
* adjusted test for both the ServiceLoader and legacy classpath load 
approach

* insert use of sleep which testing GC.

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

regards,
Sean.

On 30/01/2020 07:59, Peter Levart wrote:

Hi Seán,

WeakHashMap is not safe to be called concurrently. Even get() method, 
although it may seem read-only, can modify internal state (expunging 
stale Weak entries), so holding a READ lock while accessing 
WeakHashMap it is wrong.


getInitialContext() static method is called with an env Hashtable 
which means it can be called with different keys/values for the same 
TCCL. So caching of InitialContextFactory is just performed for the 
1st call for a particular TCCL. Subsequent calls for the same TCCL and 
different class names are not cached. Is this the behavior you are 
pursuing? You could cache the factory using (TCCL, class name) as a 
compound key.


Also, by caching in a WeakHashMap, 
you make a strong reference to InitialContextFactory from class loader 
of the NamingManager.class and such InitialContextFactory may 
indirectly reference the ClassLoader key of the same entry, so it will 
never go away because NamingManager class is never going away. You 
should at least use a WeakHashMapWeakReference> for that.


Shameless plug: there is a JDK internal class 
jdk.internal.loader.ClassLoaderValue which you might be able to use 
for caching if a part of your key is a ClassLoader. From the javadoc:


 * ClassLoaderValue allows associating a
 * {@link #computeIfAbsent(ClassLoader, BiFunction) computed} non-null 
value with
 * a {@code (ClassLoader, keys...)} tuple. The associated value, as 
well as the
 * keys are strongly reachable from the associated ClassLoader so care 
should be
 * taken to use such keys and values that only reference types 
resolvable from
 * the associated ClassLoader. Failing that, ClassLoader leaks are 
inevitable.


So if you know that the InitialContextFactory instance is always 
resolvable (by class name) from the ClassLoader you are using for the 
caching key (the TCCL), then this utility might be just right for your 
purpose.


Regards, Peter


On 1/29/20 6:22 PM, Seán Coffey wrote:

Thanks for the reviews. I found an issue with the new test also -

it's loading the custom factory class via the non-serviceloader 
approach. I was hoping to exercise ServiceLoader here. I'll address 
this and the comments raised and revert with a new patch shortly.


Regards,
Sean.

On 29/01/20 16:27, Alan Bateman wrote:

On 29/01/2020 15:55, Daniel Fuchs wrote:

Hi Seán,


http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/
A WeakHashKey with the TCCL as the key should be okay here.


If the TCCL is the key then there are good chances that the
concrete factory class is expected to be loaded by the TCCL.

If that happens then the value will reference the key and
nothing will ever get garbage collected.
I don't know how much JNDI is used much beyond LDAP these days but 
you are right that a factory with a strong ref to the TCCL would 
prevent it from being GC'ed.  The internal WeakPairMap might be 
useful here.


-Alan

Re: RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Thanks for the reviews. I found an issue with the new test also -

it's loading the custom factory class via the non-serviceloader 
approach. I was hoping to exercise ServiceLoader here. I'll address this 
and the comments raised and revert with a new patch shortly.


Regards,
Sean.

On 29/01/20 16:27, Alan Bateman wrote:

On 29/01/2020 15:55, Daniel Fuchs wrote:

Hi Seán,


http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/
A WeakHashKey with the TCCL as the key should be okay here.


If the TCCL is the key then there are good chances that the
concrete factory class is expected to be loaded by the TCCL.

If that happens then the value will reference the key and
nothing will ever get garbage collected.
I don't know how much JNDI is used much beyond LDAP these days but you 
are right that a factory with a strong ref to the TCCL would prevent 
it from being GC'ed.  The internal WeakPairMap might be useful here.


-Alan

RFR: 8223260: NamingManager should cache InitialContextFactory

[Seán Coffey]

Looks like we can improve performance in this area. I've put together a 
testcase which exercises the ServiceLoader and keeps track of whether 
we're able to cache or not.


https://bugs.openjdk.java.net/browse/JDK-8223260
http://cr.openjdk.java.net/~coffeys/webrev.8223260.v1/webrev/

regards,
Sean.

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi Philipp,

thanks for the reply .. comments inline

On 19/01/20 16:09, Philipp Kunz wrote:

Hi Sean,

I figure that distinguishing zips and jars is ambiguous in a certain
way. After having signed a zip, it contains a manifest and is also a
jar file according to the specs. That would mean we would end up with
two kinds of jar files, those with permissions and those without,
depending on the file name extension of the files. That a tool named
jarsigner is successfully applied to a file is for me convincing enough
to consider that file a jar file or jarsigned would reject it however
it name ends.

Yes, both zip and jar files enjoy the same format. jar files being created
predominantly with the java based API/tools.


Now when signing jar files (namely those with zip file name extension
but depending on the further discussion not necessary limited to those)
with permissions, I wonder if those permissions should also be subject
of the signature. I would consider a change of an executable flag for
example a change of the signed file just the same as a changed byte of
contents. The way jarsigner works now, loosing the permissions, we know
at least that the files contained in signed jars have certain
permission flags (none or defaults) immediately after having signed
them even though later manipulation would not be detected and, hence,
cannot be trusted even with a valid signature.
historically, jar tool never recorded these permissions. As such, it 
should make

no difference to how jarsigner operates on such files. The original report
highlights the loss of data on files created with the zip utility. It 
think it's reasonable

for jarsigner to preserve attributes in such a case.


Far as I understand the current situation, jars and zips could be told
from one another only before signing depending on the presence or
absence of a manifest. After signing, however, a manifest is always
there. Looking at it that way, the "much more consideration" you
mention would have to take place now, wouldn't it?

I'm not sure if there's much change in this area post fix. The classic
jar tool doesn't add or read the file attributes at hand. The name check
was useful to prevent any unintended behavioural changes in this area.


I also wonder why the "if (filename.endsWith(".zip"))" piece of code
does not occur only in JarSigner. Why is it in ZipFile, too? One such
kind of if statement should have been enough, at first glance. There
may be a good and convincing explanation but I don't see it just like
that. On the other hand, having this if condition duplicated into
ZipFile looks like it could have an undesired side-effect elsewhere. I
also kind of miss another test case to make sure permission flags
continue to be disregarded for "non-zip" jar files.

fair point. I should be able to remove this code. I can also improve the
test case.


And the comment of Michael Osipov is really frightening. Did you know
that i and I are two different characters in turkish both of which have
yet other capital/small counterparts? At that point maybe discussing
the file name extension distinction could easily become lengthier than
a discussion about consequences of supporting permissions for all jar
files.

frightening ??  Yes, I've worked on such issues before. Thanks to Michael
for pointing that out. I should be able to do a comparison using Locale.ROOT


You mention "the case, at hand is unique here" and that made to occur
to me that if there is maybe only one person affected that he or she
could work around it more easily than changing the jdk. Is it really
important for most others? And is it really a bug at all?

Another idea to work around compatibility issues would be to introduce
a flag for jarsigner, for example -p like with tar. Yet another idea
could be to refire the jar file specs and jar and jarsigner tools docs
accordingly.

Let me have a think about this. A new flag in jarsigner may help.

regards,
Sean.


Regards,
Philipp


On Fri, 2020-01-17 at 13:07 +, Seán Coffey wrote:

Hi Philipp,

On 17/01/2020 12:40, Philipp Kunz wrote:

Hi Sean,

Nice patch. I wonder why permissions should be preserved only in
zip
files. Jar files also are zip files, according to the jar file
specs,
and hence, shouldn't jar files benefit of preserving permissions,
too?

Thanks for your comments. The jar tool has never been interested in
the
posix
permissions fields for the individual entries. Such a change could
yield
more
interoperability issues. Such a change would also need much more
consideration

The zip tool on the other hand has always populated this field and I
think the case
at hand is unique here (preserving attributes already created by
non-java tools)

The file name extension is most often zip for zip files and jar for
jar
files but is that really a safe assumption? I would not expect it
always. Removing

Yes, I didn't see any easy way to distinguish a zip file from a jar
file
withou

Re: RFR: 8237508: Simplify JarFile.isInitializing

[Seán Coffey]

Looks good to me Claes - thanks for fixing.

Regards,
Sean.

On 20/01/20 12:15, Claes Redestad wrote:

Makes sense to keep even trivial logic out of the access bridge. Let's
also clean up the placement of the static variable and the pre-existing
use of the unconventional "final static" combo:

http://cr.openjdk.java.net/~redestad/8237508/open.01/

/Claes

On 2020-01-20 12:35, Daniel Fuchs wrote:

Hi Claes,

Looks OK to me but I'd have a slight preference to a solution
that confines the hack to the JarFile class.

Would making the isIntializing field private and introducing
a static boolean isInitializing() method in JarFile bring the
same benefits WRT startup?

best regards,

-- daniel

On 20/01/2020 11:16, Claes Redestad wrote:

Hi,

JDK-8234466[1] introduced isInitializing to JarFile, which cause a 
small
startup regression in various tests due increasing the number of 
classes

loaded and earlier lambda bootstrapping. The regression can be resolved
by not explicitly initializing the thread local variable to a non-null
value.

Webrev: http://cr.openjdk.java.net/~redestad/8237508/open.00/
Bug:https://bugs.openjdk.java.net/browse/JDK-8237508

Testing: tier1-3

Thanks!

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8234466

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi Philipp,

On 17/01/2020 12:40, Philipp Kunz wrote:

Hi Sean,

Nice patch. I wonder why permissions should be preserved only in zip
files. Jar files also are zip files, according to the jar file specs,
and hence, shouldn't jar files benefit of preserving permissions, too?
Thanks for your comments. The jar tool has never been interested in the 
posix
permissions fields for the individual entries. Such a change could yield 
more
interoperability issues. Such a change would also need much more 
consideration


The zip tool on the other hand has always populated this field and I 
think the case
at hand is unique here (preserving attributes already created by 
non-java tools)


The file name extension is most often zip for zip files and jar for jar
files but is that really a safe assumption? I would not expect it
always. Removing
Yes, I didn't see any easy way to distinguish a zip file from a jar file 
without being
more invasive and scanning file attributes for that file. I could take 
that approach

if it's deemed necessary.

regards,
Sean.



if (zf.getName().toLowerCase().endsWith(".zip")) {

along with similar code in ZipFile would avoid discussing that question
and the test would not have to check that files with another name
extension than zip don't preserve permissions.

Philipp


On Fri, 2020-01-17 at 10:59 +0000, Seán Coffey wrote:

Hi,

Looking to introduce some JDK private functionality which will help
preserve internal zip file attribute permissions when jarsigner is
run
on a zip file. Some of the logic is taken from the recent work
carried
out in this area for zipfs API.

https://bugs.openjdk.java.net/browse/JDK-8218021
http://cr.openjdk.java.net/~coffeys/webrev.8218021/webrev/

regards,
Sean.

RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi,

Looking to introduce some JDK private functionality which will help 
preserve internal zip file attribute permissions when jarsigner is run 
on a zip file. Some of the logic is taken from the recent work carried 
out in this area for zipfs API.


https://bugs.openjdk.java.net/browse/JDK-8218021
http://cr.openjdk.java.net/~coffeys/webrev.8218021/webrev/

regards,
Sean.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks Alan. Updates made and changes pushed.

regards,
Sean.

On 13/01/2020 18:50, Alan Bateman wrote:

On 13/01/2020 10:28, Seán Coffey wrote:
some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, 
the EventHelper utility class should never initialize the logging 
framework early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
Thanks for the update. JAR file verification is tricky and important 
not to attempt to run arbitrary code while doing that, esp. anything 
that might need to load a class or resource from the class path. So I 
think the approach (in v5) looks okay.  A minor nit in JarFile is that 
it should be "static final".  Also you might want to replace or change 
the @summary in both tests to make it clearer that the tests attempt 
to trigger class loading from the class loader during JAR file 
verification.


-Alan.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks for the reviews. All callers of EventHelper log methods are 
ensuring that isLoggingSecurity() is true before proceeding. I've added 
an assert null check in the 4 logger methods to ensure expectations are 
in place.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Hope this helps,
Sean.

On 13/01/2020 14:38, Daniel Fuchs wrote:

On 13/01/2020 14:06, Chris Hegarty wrote:
I’m going to ask, since I cannot find the answer myself. Why are some 
securityLogger::log invocations guarded with isLoggingSecurity, and 
others not? With this change shouldn’t all invocations be guarded, 
since it is isLoggingSecurity that assigns securityLogger a value?


Argh! Well spotted chris!

-- daniel

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, the 
EventHelper utility class should never initialize the logging framework 
early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/

regards,
Sean.

On 16/12/2019 14:15, Seán Coffey wrote:
The recent crypto event logging mechanism (JDK-8148188) has introduced 
a regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger 
to initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. 
I've added a new ServiceLoader test which has concurrent threads. One 
is loading providers and another is initializing JarFile verifiers. 
Hope it helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

The recent crypto event logging mechanism (JDK-8148188) has introduced a 
regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger to 
initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. I've 
added a new ServiceLoader test which has concurrent threads. One is 
loading providers and another is initializing JarFile verifiers. Hope it 
helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

--
Regards,
Sean.

Re: RFR: 8232984: Upgrading Joni License version to 2.1.16

[Seán Coffey]

Looks fine to me. I can push this for you.

Regards,
Sean.

On 04/11/19 10:04, Kiran Ravikumar wrote:

Hi,

Please review this patch to update the joni version to 2.1.16.


JBS: https://bugs.openjdk.java.net/browse/JDK-8232984

License File: https://github.com/jruby/joni/blob/joni-2.1.16/LICENSE

Patch File:

diff -r ab4db38ed085 src/jdk.scripting.nashorn/share/legal/joni.md
--- a/src/jdk.scripting.nashorn/share/legal/joni.md Fri Nov 01 
16:16:05 2019 +0100
+++ b/src/jdk.scripting.nashorn/share/legal/joni.md Mon Nov 04 
09:57:41 2019 +

@@ -1,9 +1,9 @@
-## JRuby Joni v1.1.9
+## JRuby Joni v2.1.16

-### JRuby License
+### MIT License
 

-Jruby 2012
+Copyright (c) 2017 JRuby Team

 Permission is hereby granted, free of charge, to any person obtaining a
 copy of this software and associated documentation files (the 
"Software"),



Thanks,

Kiran

Re: RFR of JDK-8134599: TEST_BUG: java/rmi/transport/closeServerSocket/CloseServerSocket.java fails intermittently with Address already in use

[Seán Coffey]

Quite a while since I touched this one! The nature of the test is a bit 
flaky given

that we're assuming no other process will use the chosen port. I'm not sure
if a concrete solution is possible. One other option is to have this 
test run
in non-concurrent mode by editing test/jdk/TEST.ROOT --> 
exclusiveAccess.dirs=


The while loop approach may help but won't protect against cases where a
long running process has attached to the port number we're interested in.

regards,
Sean.

On 15/10/2019 03:20, Weijun Wang wrote:

+SeanC

The wait might unnecessarily increase the test time. Maybe you can do something 
like this:

int timeout = 10;
while (timeout > 0) {
   sleep(one second);
   verifyPortFree && return;
   timeout--;
}
throw new Exception(still not freed);

And Sean, back in JDK-8016728 you said "Let's extend it to 1000ms and see how test 
behaves". So what do you think?

Thanks,
Max


On Oct 15, 2019, at 10:03 AM, Hamlin Li  wrote:

Hi,

The test is failing more frequently, could some help to review it?

Thank you

-Hamlin

On 2019/9/4 11:11 AM, Hamlin Li wrote:

some background & comment: in most of failures, the "test.timeout.factor" is 
10.0 or 8.0, so in the test code this factor should be considered in rmi operations such 
unexporting an object.

Thank you

-Hamlin

On 2019/9/4 11:01 AM, Hamlin Li wrote:

Hi,

Would you please review the following patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8134599

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


Thank you

-Hamlin

Re: RFR: JDK-8231770 - Test java/util/zip/FlaterTest.java fails with -Xcheck:jni

[Seán Coffey]

Looks good Kiran. I'll sponsor this for you.

regards,
Sean.

On 08/10/2019 17:17, Kiran Ravikumar wrote:

Hi Guys,

I am a new hire with the Oracle Java Platform Group and will be 
working mainly on JDK Update releases. Below is my first contribution. 
Looking forward to contribute more to the community.


Please review the fix:

Bug :https://bugs.openjdk.java.net/browse/JDK-8231770
Webrev :https://cr.openjdk.java.net/~coffeys/webrev.8231770/webrev/ 

Explanation : Minor correction to the native code that had wrong 
arguments. Updated the corresponding test to run with -Xcheck:jni to 
prevent further occurrences of similar issue.



Thanks,
Kiran

RFR: 8231124: Missing closedir call with JDK-8223490

[Seán Coffey]

A minor issue that was introduced via my recent JDK-8223490 fix. One 
which I noticed while backporting the edits..


https://bugs.openjdk.java.net/browse/JDK-8231124

proposed patch:
diff --git a/src/java.base/unix/native/libjava/TimeZone_md.c 
b/src/java.base/unix/native/libjava/TimeZone_md.c

--- a/src/java.base/unix/native/libjava/TimeZone_md.c
+++ b/src/java.base/unix/native/libjava/TimeZone_md.c
@@ -130,11 +130,6 @@
 char *tz = NULL;
 int res;

-    dirp = opendir(dir);
-    if (dirp == NULL) {
-    return NULL;
-    }
-
 if (strcmp(dir, ZONEINFO_DIR) == 0) {
 /* fast path for 1st iteration */
 for (unsigned int i = 0; i < sizeof (popularZones) / sizeof 
(popularZones[0]); i++) {

@@ -151,6 +146,11 @@
 }
 }

+    dirp = opendir(dir);
+    if (dirp == NULL) {
+    return NULL;
+    }
+
 while ((dp = readdir(dirp)) != NULL) {
 /*
  * Skip '.' and '..' (and possibly other .* files)


==

regards,
Sean.

Re: RFR: 8223490: Optimize search algorithm for determining default time zone

[Seán Coffey]

Thanks Roger,

I've reverted the header file changes then and added this to the .c file:

@@ -42,6 +42,8 @@
 #include "jvm.h"
 #include "TimeZone_md.h"

+static char *isFileIdentical(char* buf, size_t size, char *pathname);
+
 #define SKIP_SPACE(p)   while (*p == ' ' || *p == '\t') p++;

 #define RESTARTABLE(_cmd, _result) do { \

Regards,
Sean.

On 13/09/19 13:01, Roger Riggs wrote:

Hi Sean,

The declaration can be up in the _md.c file to satify the compiler 
about forward declarations.


$.02, Roger

On 9/13/19 4:34 AM, Seán Coffey wrote:
Thanks for the review Roger. I run into compiler issues if I don't 
declare the new function in the header file.


/ws/jdk-jdk/open/src/java.base/unix/native/libjava/TimeZone_md.c:143:18: 
error: implicit declaration of function ‘isFileIdentical’ 
[-Werror=implicit-function-declaration]

 tz = isFileIdentical(buf, size, pathname);

If I try to declare it earlier in the main code,  I also run into 
issues since this new function calls other implicitly declared 
functions.


regards,
Sean.

On 12/09/2019 18:47, Roger Riggs wrote:

Hi Sean,

In addition to Naoto's comments.

The change to TimeZone_md.h should not be needed.
A 'static' declaration doesn't need to be visible outside its source 
file.


Roger


On 9/12/19 12:42 PM, naoto.s...@oracle.com wrote:

Hi Seán,

I like your approach to provide the fast path to determine the 
system time zone. One general question is, is UTC/GMT the right set 
of fast path candidates? Should we add some more common ones?


Other comments to the code:

TimeZone_md.c

- Should fast path search come after "dir" validation, i.e., line 
146-148?

- Line 126: "statbuf" can be removed.
- Line 134: 'i' is not size_of_something, so 'int' type should 
suffice (and its initialization is done in the for-loop).
- Line 138: the fast path search should "continue" with the next 
name, instead of "break".

- Line 142, 182: I'd wrap this line with parens for the if statement.
- Line 232-242: "pathname" is an argument to this function, so 
freeing it inside the function seems odd. Also, no need to reset 
dbuf/fd since they are no longer reused in the loop.


Naoto

On 9/11/19 3:50 AM, Seán Coffey wrote:
The current algorithm for determining the default timezone on 
(non-AIX) unix systems goes something like :


1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the 
name pointed to
4. else if /etc/localtime is a binary, find the first identical 
time zone binary file in /usr/share/zoneinfo/


Step 4 is a bit crude in that the zoneinfo directory can contain 
over 1,800 files on today's systems. I'd like to change the logic 
so that common timezones are first checked for buffer matching 
before a full directory traversal is performed. It should be a 
performance gain and it should also lead to more consistent 
results for reasons outlined in the bug report.


https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 
<http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223490/webrev/>

Re: RFR: 8223490: Optimize search algorithm for determining default time zone

[Seán Coffey]

Thanks for the review Roger. I run into compiler issues if I don't 
declare the new function in the header file.


/ws/jdk-jdk/open/src/java.base/unix/native/libjava/TimeZone_md.c:143:18: 
error: implicit declaration of function ‘isFileIdentical’ 
[-Werror=implicit-function-declaration]

 tz = isFileIdentical(buf, size, pathname);

If I try to declare it earlier in the main code,  I also run into issues 
since this new function calls other implicitly declared functions.


regards,
Sean.

On 12/09/2019 18:47, Roger Riggs wrote:

Hi Sean,

In addition to Naoto's comments.

The change to TimeZone_md.h should not be needed.
A 'static' declaration doesn't need to be visible outside its source 
file.


Roger


On 9/12/19 12:42 PM, naoto.s...@oracle.com wrote:

Hi Seán,

I like your approach to provide the fast path to determine the system 
time zone. One general question is, is UTC/GMT the right set of fast 
path candidates? Should we add some more common ones?


Other comments to the code:

TimeZone_md.c

- Should fast path search come after "dir" validation, i.e., line 
146-148?

- Line 126: "statbuf" can be removed.
- Line 134: 'i' is not size_of_something, so 'int' type should 
suffice (and its initialization is done in the for-loop).
- Line 138: the fast path search should "continue" with the next 
name, instead of "break".

- Line 142, 182: I'd wrap this line with parens for the if statement.
- Line 232-242: "pathname" is an argument to this function, so 
freeing it inside the function seems odd. Also, no need to reset 
dbuf/fd since they are no longer reused in the loop.


Naoto

On 9/11/19 3:50 AM, Seán Coffey wrote:
The current algorithm for determining the default timezone on 
(non-AIX) unix systems goes something like :


1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the 
name pointed to
4. else if /etc/localtime is a binary, find the first identical time 
zone binary file in /usr/share/zoneinfo/


Step 4 is a bit crude in that the zoneinfo directory can contain 
over 1,800 files on today's systems. I'd like to change the logic so 
that common timezones are first checked for buffer matching before a 
full directory traversal is performed. It should be a performance 
gain and it should also lead to more consistent results for reasons 
outlined in the bug report.


https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 
<http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223490/webrev/>

Re: RFR: 8223490: Optimize search algorithm for determining default time zone

[Seán Coffey]

Thanks for the review Naoto. The edits certainly did need some tidying 
up. Comments inline.


On 12/09/2019 17:42, naoto.s...@oracle.com wrote:

Hi Seán,

I like your approach to provide the fast path to determine the system 
time zone. One general question is, is UTC/GMT the right set of fast 
path candidates? Should we add some more common ones?


I'm open to suggestions. I think these two are very common and good for 
starting with.



Other comments to the code:

TimeZone_md.c

- Should fast path search come after "dir" validation, i.e., line 
146-148?

- Line 126: "statbuf" can be removed.
- Line 134: 'i' is not size_of_something, so 'int' type should suffice 
(and its initialization is done in the for-loop).
- Line 138: the fast path search should "continue" with the next name, 
instead of "break".

- Line 142, 182: I'd wrap this line with parens for the if statement.

All above corrected.
- Line 232-242: "pathname" is an argument to this function, so freeing 
it inside the function seems odd. Also, no need to reset dbuf/fd since 
they are no longer reused in the loop.


I thought it was a useful approach given that it's the last function to 
use 'pathname'. However, it's not in keeping with normal design I guess. 
I've reverted and now free pathname at other call sites instead.


new webrev at http://cr.openjdk.java.net/~coffeys/webrev.8223490.v2/webrev/

regards,
Sean.


Naoto

On 9/11/19 3:50 AM, Seán Coffey wrote:
The current algorithm for determining the default timezone on 
(non-AIX) unix systems goes something like :


1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the name 
pointed to
4. else if /etc/localtime is a binary, find the first identical time 
zone binary file in /usr/share/zoneinfo/


Step 4 is a bit crude in that the zoneinfo directory can contain over 
1,800 files on today's systems. I'd like to change the logic so that 
common timezones are first checked for buffer matching before a full 
directory traversal is performed. It should be a performance gain and 
it should also lead to more consistent results for reasons outlined 
in the bug report.


https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 
<http://cr.openjdk.java.net/%7Ecoffeys/webrev.8223490/webrev/>

RFR: 8223490: Optimize search algorithm for determining default time zone

[Seán Coffey]

The current algorithm for determining the default timezone on (non-AIX) 
unix systems goes something like :


1. If TZ environment variable is defined, use it
2. else if /etc/timezone exists, use the value contained within it
3. else if /etc/localtime exists and is a symbolic link, use the name 
pointed to
4. else if /etc/localtime is a binary, find the first identical time 
zone binary file in /usr/share/zoneinfo/


Step 4 is a bit crude in that the zoneinfo directory can contain over 
1,800 files on today's systems. I'd like to change the logic so that 
common timezones are first checked for buffer matching before a full 
directory traversal is performed. It should be a performance gain and it 
should also lead to more consistent results for reasons outlined in the 
bug report.


https://bugs.openjdk.java.net/browse/JDK-8223490
webrev: http://cr.openjdk.java.net/~coffeys/webrev.8223490/webrev/ 



--
Regards,
Sean.

Re: [13u]: RFR: 8228469: (tz) Upgrade time-zone data to tzdata2019b

[Seán Coffey]

Looks good Ramanand.

regards,
Sean.

On 06/08/2019 06:57, Ramanand Patil wrote:

Hi all,
Please review the patch for jdk13u backport of tzdata2019b integration into jdk:
Webrev: http://cr.openjdk.java.net/~rpatil/8228469/13u/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8228469

The patch is not clean because: It uses "rearguard" format tzdata from IANA 
(instead of vanguard format), as the enhancement bug[1] is not fixed in jdk13u.
The patch has passed all the related testing including JCK.

Note: As per the mail from tzdata maintainers[2] there is a possibility that 
Brazil might not abolish DST.


[1] https://bugs.openjdk.java.net/browse/JDK-8212970
[2] https://mm.icann.org/pipermail/tz/2019-July/028344.html


Regards,
Ramanand.

RFR: 8213561: ZipFile/MultiThreadedReadTest.java timed out in tier1

[Seán Coffey]

Reports that this test is failing intermittently over past few months. 
It's a rare occurrence but I'd like to take steps to correct it.


I've removed the dependence on randomness in the bug.
I've fixed up the zip file creation logic to produce a real zip file
I've renamed the file to a unique file name per test run (and recorded 
how long it takes to delete it)

Test now logs any exception encountered during zip entry read

I've verified that the test still fails and passes as expected.

https://bugs.openjdk.java.net/browse/JDK-8213561
http://cr.openjdk.java.net/~coffeys/webrev.8213561/webrev/

--
Regards,
Sean.

Re: RFR: JDK8U JDK-8202088, JDK-8207152, JDK-8211398, JDK-8180469, JDK-8206120, JDK-8218915, JDK-8217710

[Seán Coffey]

Looks good.

regards,
Sean.

On 21/02/2019 08:54, Deepak Kejriwal wrote:

Hi Naoto,

Corrected the exception message. Please find below updated version of webrev:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_and_Currency_changes_8u/webrev.02/


Thanks for review.

Regards,
Deepak

-Original Message-
From: Naoto Sato
Sent: Wednesday, February 20, 2019 11:07 PM
To: Deepak Kejriwal ; Sean Coffey 
; core-libs-dev ; 
jdk8u-...@openjdk.java.net
Subject: Re: RFR: JDK8U JDK-8202088, JDK-8207152, JDK-8211398, JDK-8180469, 
JDK-8206120, JDK-8218915, JDK-8217710

Hi Deepak,

The same comment for 11u can be applied here too:

  > - Line 163,198: Exception messages are incorrect. they are for 
isJavaIdentifierStart().

Naoto

On 2/20/19 8:57 AM, Deepak Kejriwal wrote:

Thanks Naoto San and Sean for review. I have incorporate all the
comments. Please find below updated version of webrev :-

http://cr.openjdk.java.net/~rpatil/JapaneseEra_and_Currency_changes_8u
/webrev.01/

Regards,
Deepak

-Original Message-
From: Naoto Sato
Sent: Tuesday, February 19, 2019 10:40 PM
To: Deepak Kejriwal ; core-libs-dev
; jdk8u-...@openjdk.java.net
Subject: Re: RFR: JDK8U JDK-8202088, JDK-8207152, JDK-8211398,
JDK-8180469, JDK-8206120, JDK-8218915, JDK-8217710

Hi Deepak,

Almost all the comments for the 11u changes [1] applies here, except the "newCodePoint" comment. 
For this one, I'd suggest renaming "newCodePoints" to "UNASSIGNED_CODEPOINTS_IN_6_2"

Naoto

[1]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/05
8610.html

On 2/19/19 5:55 AM, Deepak Kejriwal wrote:

Hi All,

Please review the backport of the following bug fixes to jdk8u-dev:

HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8202088"JDK-8202088: 
Japanese new era implementation.
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8207152"JDK-8207152: 
Placeholder for Japanese new era should be two characters.
HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 :
Square character support for the Japanese new era HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8180469"JDK-8180469 : Wrong
short form text for supplemental Japanese era HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add
test cases for lenient Japanese era parsing HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 :
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new
code points HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8217710"JDK-8217710 : Add 5
currency code points to Java SE 8uX

Webrev:
http://cr.openjdk.java.net/~rpatil/JapaneseEra_and_Currency_changes_8
u
/webrev.00/

These code changes are made possible thanks to specification changes already 
pushed:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/c35f231af17a
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/00475cd329f7

Regards,
Deepak

Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

[Seán Coffey]

Thanks. Looks good to me.

regards,
Sean.

On 21/02/2019 09:10, Deepak Kejriwal wrote:


Hi Sean,

The webrev I shared was not correct. I have corrected the webrev.02 
now. Please check now:-


http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/

Regards,

Deepak

*From:*Seán Coffey
*Sent:* Thursday, February 21, 2019 2:11 PM
*To:* Deepak Kejriwal ; Naoto Sato 
; core-libs-dev 
; jdk-updates-...@openjdk.java.net

*Subject:* Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

Deepak,

this exception message in new test still needs correction:

  166 "Character.isLetter(int) failed for codepoint 
"

  167 + Integer.toHexString(cp));

As an aside, there's probably no need for such specific exception 
messages in a test case. It's error prone (but you've come this far now)


regards,
Sean.

On 21/02/2019 08:26, Deepak Kejriwal wrote:

Hi Naoto,

Corrected the exception message. Please find below updated version of 
webrev:-

http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/

Thanks for review.

Regards,

Deepak

-Original Message-

From: Naoto Sato

Sent: Wednesday, February 20, 2019 11:06 PM

To: Deepak Kejriwal  <mailto:deepak.kejri...@oracle.com>; Sean 
Coffey  <mailto:sean.cof...@oracle.com>; 
core-libs-dev  
<mailto:core-libs-dev@openjdk.java.net>;jdk-updates-...@openjdk.java.net  
<mailto:jdk-updates-...@openjdk.java.net>

Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

Hi Deepak,

I see the following comment is not addressed yet:

  > - Line 163,198: Exception messages are incorrect. they are for 
isJavaIdentifierStart().

Naoto

On 2/20/19 8:53 AM, Deepak Kejriwal wrote:

Thanks Naoto San and Sean for review. I have incorporate all the

comments. Please find below updated version of webrev :-

http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/

Regards,

Deepak

-Original Message-

From: Naoto Sato

Sent: Tuesday, February 19, 2019 10:23 PM

To: Deepak Kejriwal  
<mailto:deepak.kejri...@oracle.com>; core-libs-dev

  
<mailto:core-libs-dev@openjdk.java.net>;jdk-updates-...@openjdk.java.net  
<mailto:jdk-updates-...@openjdk.java.net>

Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

Hi Deepak,

Here are my comments to the webrev (other than what Sean pointed out):

TestIsJavaIdentifierMethods.java

- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> 
"isJavaLetterOrDigit".

- Line 34: "newCodePoint" does not represent the era character, as "new"

is subjective. It will become moot in the year 2020. How about 
"JAPANESE_ERA_CODEPOINT"?

- Line 67,68,(...all the comments): Reflect the above change to the 
comments.

- Line 103: "All Unicode chars (0x..0x)" does not sound correct.

It may be "All Unicode code points in the BMP (0x..0x), and 
remove extra period at the end. This applies to other method descriptions.

- Line 104: The test case returns "void", what does this "Expected 
results" mean?

- Line 140-142,174-176: The condition statement in the document is 
different from JDK11's javadoc. In the API doc, it is (in case of int):

  isLetter(codePoint) returns true

  getType(codePoint) returns LETTER_NUMBER

  the referenced character is a currency symbol (such as '$')

  the referenced character is a connecting punctuation character 
(such as '_').

- Line 163,198: Exception messages are incorrect. they are for 
isJavaIdentifierStart().

Naoto

On 2/19/19 6:15 AM, Deepak Kejriwal wrote:

Correcting typo for release.




From: Deepak Kejriwal  
<mailto:deepak.kejri...@oracle.com>

Sent: Tuesday, February 19, 2019 7:42 PM

To: 'core-libs-dev'  
<mailto:core-libs-dev@openjdk.java.net>;

'jdk-updates-...@openjdk.java.net  
<mailto:jdk-updates-...@openjdk.java.net>'  
<mailto:jdk-updates-...@openjdk.java.net>

Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915




Hi All,

Please review the backport of the following bug fixes to jdk11u-dev:

HYPERLINK

"https://bugs.openjdk.java.net/browse/JDK-8206120;  
<https://bugs.openjdk.java.net/browse/JDK-8206120>JDK-8206120 : Add

test cases for lenient Japanese era parsing HYPERLINK

"https://bugs.openjdk.java.net/browse/JDK-8211398;  
<

Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

[Seán Coffey]

Deepak,

this exception message in new test still needs correction:


  166 "Character.isLetter(int) failed for codepoint "
  167 + Integer.toHexString(cp));
As an aside, there's probably no need for such specific exception 
messages in a test case. It's error prone (but you've come this far now)


regards,
Sean.

On 21/02/2019 08:26, Deepak Kejriwal wrote:

Hi Naoto,

Corrected the exception message. Please find below updated version of webrev:-
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.02/

Thanks for review.

Regards,
Deepak

-Original Message-
From: Naoto Sato
Sent: Wednesday, February 20, 2019 11:06 PM
To: Deepak Kejriwal ; Sean Coffey 
; core-libs-dev ; 
jdk-updates-...@openjdk.java.net
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

Hi Deepak,

I see the following comment is not addressed yet:

  > - Line 163,198: Exception messages are incorrect. they are for 
isJavaIdentifierStart().

Naoto

On 2/20/19 8:53 AM, Deepak Kejriwal wrote:

Thanks Naoto San and Sean for review. I have incorporate all the
comments. Please find below updated version of webrev :-

http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.01/

Regards,
Deepak

-Original Message-
From: Naoto Sato
Sent: Tuesday, February 19, 2019 10:23 PM
To: Deepak Kejriwal ; core-libs-dev
; jdk-updates-...@openjdk.java.net
Subject: Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

Hi Deepak,

Here are my comments to the webrev (other than what Sean pointed out):

TestIsJavaIdentifierMethods.java

- @summary: "testIsJavaLetter" -> "isJavaLetter", "testIsJavaLetterOrDigit" -> 
"isJavaLetterOrDigit".

- Line 34: "newCodePoint" does not represent the era character, as "new"
is subjective. It will become moot in the year 2020. How about 
"JAPANESE_ERA_CODEPOINT"?

- Line 67,68,(...all the comments): Reflect the above change to the comments.

- Line 103: "All Unicode chars (0x..0x)" does not sound correct.
It may be "All Unicode code points in the BMP (0x..0x), and remove 
extra period at the end. This applies to other method descriptions.

- Line 104: The test case returns "void", what does this "Expected results" 
mean?

- Line 140-142,174-176: The condition statement in the document is different 
from JDK11's javadoc. In the API doc, it is (in case of int):

  isLetter(codePoint) returns true
  getType(codePoint) returns LETTER_NUMBER
  the referenced character is a currency symbol (such as '$')
  the referenced character is a connecting punctuation character (such as 
'_').

- Line 163,198: Exception messages are incorrect. they are for 
isJavaIdentifierStart().

Naoto


On 2/19/19 6:15 AM, Deepak Kejriwal wrote:

Correcting typo for release.




From: Deepak Kejriwal 
Sent: Tuesday, February 19, 2019 7:42 PM
To: 'core-libs-dev' ;
'jdk-updates-...@openjdk.java.net' 
Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915




Hi All,

Please review the backport of the following bug fixes to jdk11u-dev:

HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add
test cases for lenient Japanese era parsing HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 :
Square character support for the Japanese new era HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 :
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new
code points

Webrev:
http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/

These code changes are made possible thanks to specification change already 
pushed:
http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace

Regards,
Deepak

Re: RFR: JDK8U JDK-8202088, JDK-8207152, JDK-8211398, JDK-8180469, JDK-8206120, JDK-8218915, JDK-8217710

[Seán Coffey]

Looks fine to me.

some minor comments on formatting :

space after "//" style comments in your new tests :

e.g


//List of new code points are not present in Unicode 6.2.
   39  add(0x20BB); //NORDIC MARK SIGN
   40  add(0x20BC); //MANAT SIGN
   41  add(0x20BD); //RUBLE SIGN
   42  add(0x20BE); //LARI SIGN
   43  add(0x20BF); //BITCOIN SIGN
   44  add(0x32FF); //SQUARE ERA NAME 
NEWERA



   77 //Since Character.isJavaIdentifierPart(int) strictly conforms 
to
   78 //character information from version 6.2 of the Unicode 
Standard,
   79 //check if code point is new code point. If the code point is 
new
   80 //code point, value of variable expected is considered false.

this looks like a typo in one of your new tests :


  268 public static void testIsJavaLetterOrDigit() {
  269 for (int i = 0; i <= Character.MAX_VALUE; ++i) {
  270 char ch = (char) i;
  271 boolean expected = false;
  272 //Since Character.isIdentifierIgnorable(char) strictly 
conforms to

regards,
Sean.

On 19/02/2019 13:55, Deepak Kejriwal wrote:

Hi All,

Please review the backport of the following bug fixes to jdk8u-dev:

HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8202088"JDK-8202088: 
Japanese new era implementation.
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8207152"JDK-8207152: 
Placeholder for Japanese new era should be two characters.
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : 
Square character support for the Japanese new era
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8180469"JDK-8180469 : Wrong 
short form text for supplemental Japanese era
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add 
test cases for lenient Japanese era parsing
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : 
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8217710"JDK-8217710 : Add 5 
currency code points to Java SE 8uX

Webrev:  
http://cr.openjdk.java.net/~rpatil/JapaneseEra_and_Currency_changes_8u/webrev.00/

These code changes are made possible thanks to specification changes already 
pushed:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/c35f231af17a
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/00475cd329f7

Regards,
Deepak

  

  

  

Re: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

[Seán Coffey]

Deepak,

changes look fine to me. Some minor comments on formatting :

make a space after all  "//" comments

e.g.
+ //isJavaIdentifierStart strictly conforms to code points assigned
+ //in Unicode 10.0. Since code point {32FF} is not from Unicode 
10.0,

+ //return false.

  33 //New code point(Japanese Era Square character) not present in 
Unicode 10.0

  34 private static final int newCodePoint = 0x32FF;

  65 //Since Character.isJavaIdentifierPart(int) strictly 
conforms to
  66 //character information from version 10.0 of the 
Unicode Standard,
  67 //check if code point is new code point. If the code 
point is new
  68 //code point, value of variable expected is considered 
false.



  typo in comment :
  make/data/characterdata/CharacterData00.java.template

    boolean isJavaIdentifierPart(int ch) {
+ //isJavaIdentifierStart strictly conforms to code points assigned
+ //in Unicode 10.0. Since code point {32FF} is not from Unicode 
10.0,


regards,
Sean.

On 19/02/2019 14:15, Deepak Kejriwal wrote:

Correcting typo for release.

  


From: Deepak Kejriwal 
Sent: Tuesday, February 19, 2019 7:42 PM
To: 'core-libs-dev' ; 
'jdk-updates-...@openjdk.java.net' 
Subject: RFR: JDK11U JDK-8206120, JDK-8211398, JDK-8218915

  


Hi All,

Please review the backport of the following bug fixes to jdk11u-dev:

HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8206120"JDK-8206120 : Add 
test cases for lenient Japanese era parsing
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8211398"JDK-8211398 : 
Square character support for the Japanese new era
HYPERLINK "https://bugs.openjdk.java.net/browse/JDK-8218915"JDK-8218915 : 
Change isJavaIdentifierStart and isJavaIdentifierPart to handle new code points

Webrev:  http://cr.openjdk.java.net/~rpatil/JapaneseEra_changes_11u/webrev.00/

These code changes are made possible thanks to specification change already 
pushed:
http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/c1e1669edace

Regards,
Deepak

  

Re: FW: RFR: JDK8U Backport of JDK-8212941: Support new Japanese era in java.time.chrono.JapaneseEra

[Seán Coffey]

Thanks for the reviews. Here's the final webrev which I hope to sponsor 
for Deepak once I have the necessary approvals.


http://cr.openjdk.java.net/~coffeys/webrev.8212941.jdk8u/webrev/

Regards,
Sean.

On 28/01/19 16:06, Naoto Sato wrote:

+1

Naoto

On 1/28/19 3:18 AM, Chris Hegarty wrote:


On 27/01/2019 11:54, Deepak Kejriwal wrote:

...
Hi All,


JBS report: https://bugs.openjdk.java.net/browse/JDK-8212941

Webrev: http://cr.openjdk.java.net/~rpatil/8212941/jdk8u/webrev.00/


This looks good Deepak. Reviewed.

One minor comment. "The of(int) and valueOf(String) methods ...", these
references could be javadoc links.

Re: RFR: 8182992 Typo in DatagramPacket constructor API doc

[Seán Coffey]

Looks fine to me. We can update the bug summary to make it more general 
to affected classes.


I can help sponsor this change if required.

regards,
Sean.

On 03/01/2019 16:51, Roger Calnan wrote:

I was looking at the fix for:

https://bugs.openjdk.java.net/browse/JDK-8182992 Typo in DatagramPacket 
constructor API doc

and searched around for other similar issues.  I found:

4 in networking
2 in SQL
2 in client libs

all of which are cases of missing spaces.  I will ask for review of the the 
client ones on the
client alias but was hoping to review the rest as one, as they are all very 
similar.  Let me
know if the reviews should be separated out,

Thanks,

Roger


https://bugs.openjdk.java.net/browse/JDK-8182992 
 Typo in DatagramPacket 
constructor API doc
diff -r 3d0f6ef91216 src/java.base/share/classes/java/net/DatagramPacket.java
--- a/src/java.base/share/classes/java/net/DatagramPacket.java  Wed Jan 02 
13:37:55 2019 -0500
+++ b/src/java.base/share/classes/java/net/DatagramPacket.java  Wed Jan 02 
13:19:38 2019 -0800
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1995, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2019, 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
@@ -102,7 +102,7 @@
  
  /**

   * Constructs a datagram packet for sending packets of length
- * {@code length} with offset {@code ioffset}to the
+ * {@code length} with offset {@code offset} to the
   * specified port number on the specified host. The
   * {@code length} argument must be less than or equal to
   * {@code buf.length}.
@@ -125,7 +125,7 @@
  
  /**

   * Constructs a datagram packet for sending packets of length
- * {@code length} with offset {@code ioffset}to the
+ * {@code length} with offset {@code offset} to the
   * specified port number on the specified host. The
   * {@code length} argument must be less than or equal to
   * {@code buf.length}.

JDK-8215912 Various Typos in java.net  Method Documentation
https://bugs.openjdk.java.net/browse/JDK-8215912 


https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#isReachable(java.net.NetworkInterface,int,int)
 

https://hg.openjdk.java.net/jdk/jdk/file/abe21b82ff7c/src/java.base/share/classes/java/net/InetAddress.java#l534
 


diff -r 3d0f6ef91216 src/java.base/share/classes/java/net/InetAddress.java
--- a/src/java.base/share/classes/java/net/InetAddress.java Wed Jan 02 
13:37:55 2019 -0500
+++ b/src/java.base/share/classes/java/net/InetAddress.java Wed Jan 02 
13:19:38 2019 -0800
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1995, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2019, 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
@@ -531,7 +531,7 @@
   * @param   timeout the time, in milliseconds, before the call aborts
   * @throws  IllegalArgumentException if either {@code timeout}
   *  or {@code ttl} are negative.
- * @return a {@code boolean}indicating if the address is reachable.
+ * @return a {@code boolean} indicating if the address is reachable.
   * @throws IOException if a network error occurs
   * @since 1.5
   */

https://hg.openjdk.java.net/jdk/jdk/file/abe21b82ff7c/src/java.base/share/classes/java/net/Socket.java#l1105
 

https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/Socket.html#getOOBInline()
 


diff -r 3d0f6ef91216 src/java.base/share/classes/java/net/Socket.java
--- a/src/java.base/share/classes/java/net/Socket.java  Wed Jan 02 13:37:55 
2019 -0500
+++ b/src/java.base/share/classes/java/net/Socket.java  Wed Jan 02 13:19:38 
2019 -0800
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 1995, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2019, 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
@@ -1102,7 +1102,7 @@
   * Tests if {@link SocketOptions#SO_OOBINLINE 

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

[Seán Coffey]

I've renamed the 'peerCertificateId' variable and label in 
TLSHandshakeEvent to 'certificateId'. This allows the event data to be 
co-displayed in JMC views which share the same type of data 
(@CertificateId). I've uploaded an example screenshot [1]


I've also made an adjustment to the TestModuleEvents test to disregard 
the jdk.proxy1 module for test purposes.


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

[1] 
http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certificateId.png/Screenshot%20from%202018-11-16%2015%3a28%3a59.png


Regards,
Sean.

On 14/11/18 21:09, Seán Coffey wrote:

Hi Sean,

comments inline..

On 13/11/2018 18:53, Sean Mullan wrote:

Looking good, a couple of comments/questions:

* src/java.base/share/classes/java/security/Security.java

The isJdkSecurityProperty method could return false positives, for 
example there may be a non-JDK property starting with "security.". I 
was thinking it would be better to put all the JDK property names in 
a HashSet which is populated by the static initialize() method, and 
only if event logging is enabled. Then setProperty can just check if 
the property name is in this set.
after further discussion, we've decided to log all properties operated 
on via Security.setProperty(String, String). It think it makes sense. 
The contents of a JFR recording should always be treated as sensitive. 
Keep in mind also that these new JFR Events will be off by default.


* src/java.base/share/classes/sun/security/x509/X509CertImpl.java
Good points here. I've taken on board your suggestion to move this 
code logic to the sun/security/provider/X509Factory.java class as per 
your later email suggestion.



45 l.addExpected("SunJSSE Test Serivce");

Is that a typo? "Serivce"

That's a typo in the test certificate details. We should fix it up via 
another bug record.



* test/jdk/jdk/security/logging/TestX509ValidationLog.java

54: remove blank line

* test/lib/jdk/test/lib/security/SSLSocketTest.java

Why is this file included? It looks like a duplicate of 
SSLSocketTemplate.
Yes - it's almost identical to SSLSocketTemplate except that 
SSLSocketTemplate doesn't belong to a package. I had a hard time 
incorporating its use into the jtreg tests via the @lib syntax. I 
think it's best if we open another JBS issue to migrate other uses of 
SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate


I've cleaned up the various typo/syntax issues that you've highlighted 
also.

http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html

regards,
Sean.


The rest of my comments are mostly minor stuff, and nits:

* 
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java


245 if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {

add space before "(xve"

* src/java.base/share/classes/sun/security/ssl/Finished.java

1097 }

indent

1098 if(event.shouldCommit()) {

add space before "(event"

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

update copyright

* src/java.base/share/classes/jdk/internal/event/EventHelper.java

35  * A helper class to have events logged to a JDK Event Logger

Add '.' at end of sentence.

* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java

38: remove blank line

* 
src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java


30  * used in X509 cert path validation

Add '.' at end of sentence.

* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java

47: remove blank line

* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java


--Sean


On 11/6/18 3:46 AM, Seán Coffey wrote:
With JDK-8203629 now pushed, I've re-based my patch on latest 
jdk/jdk code.


Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
   - we can consider a new event to record non-JDK security events 
if demand comes about

   - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 



* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

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

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEv

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

[Seán Coffey]

Thanks for the comments Weijun.

As per other review thread, I'm now recording all properties set via 
Security.setProperty(String, String).


regards,
Sean.


On 14/11/2018 01:11, Weijun Wang wrote:

Confused. Aren't all Security properties security-related? This is not about 
normal system properties.

And the method name in the latest webrev is "isSecurityProperty" without the 
"JDK" word. I assume this means you don't care about the difference between SE properties 
and JDK properties.

--Max

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

[Seán Coffey]

Hi Sean,

comments inline..

On 13/11/2018 18:53, Sean Mullan wrote:

Looking good, a couple of comments/questions:

* src/java.base/share/classes/java/security/Security.java

The isJdkSecurityProperty method could return false positives, for 
example there may be a non-JDK property starting with "security.". I 
was thinking it would be better to put all the JDK property names in a 
HashSet which is populated by the static initialize() method, and only 
if event logging is enabled. Then setProperty can just check if the 
property name is in this set.
after further discussion, we've decided to log all properties operated 
on via Security.setProperty(String, String). It think it makes sense. 
The contents of a JFR recording should always be treated as sensitive. 
Keep in mind also that these new JFR Events will be off by default.


* src/java.base/share/classes/sun/security/x509/X509CertImpl.java
Good points here. I've taken on board your suggestion to move this code 
logic to the sun/security/provider/X509Factory.java class as per your 
later email suggestion.



45 l.addExpected("SunJSSE Test Serivce");

Is that a typo? "Serivce"

That's a typo in the test certificate details. We should fix it up via 
another bug record.



* test/jdk/jdk/security/logging/TestX509ValidationLog.java

54: remove blank line

* test/lib/jdk/test/lib/security/SSLSocketTest.java

Why is this file included? It looks like a duplicate of 
SSLSocketTemplate.
Yes - it's almost identical to SSLSocketTemplate except that 
SSLSocketTemplate doesn't belong to a package. I had a hard time 
incorporating its use into the jtreg tests via the @lib syntax. I think 
it's best if we open another JBS issue to migrate other uses of 
SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate


I've cleaned up the various typo/syntax issues that you've highlighted also.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html

regards,
Sean.


The rest of my comments are mostly minor stuff, and nits:

* 
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java


245 if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {

add space before "(xve"

* src/java.base/share/classes/sun/security/ssl/Finished.java

1097 }

indent

1098 if(event.shouldCommit()) {

add space before "(event"

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

update copyright

* src/java.base/share/classes/jdk/internal/event/EventHelper.java

35  * A helper class to have events logged to a JDK Event Logger

Add '.' at end of sentence.

* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java

38: remove blank line

* src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java

30  * used in X509 cert path validation

Add '.' at end of sentence.

* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java

47: remove blank line

* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java


--Sean


On 11/6/18 3:46 AM, Seán Coffey wrote:
With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk 
code.


Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
   - we can consider a new event to record non-JDK security events if 
demand comes about

   - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 



* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

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

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


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

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & 
logger commit code into the class where it's used works too well 
after all.


In the code you suggested, there's an assumption that calls such as 
EventHelp

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

[Seán Coffey]

Erik,

comments inline..
On 08/11/18 15:12, Erik Gahlin wrote:

Hi Sean,

I think we could still call the event 
"jdk.SecurityPropertyModification", but add a @Description that 
explains that events are only emitted for the JDK due to security 
concerns. If we at a later stage want to include user events, we could 
add those and remove the @Description, possibly with a setting where 
you can specify scope, or perhaps a new event all together.

sounds fine. I've made those changes.


Perhaps we could find a better name for the field 
validationEventNumber? It sounds like we have an event number, which 
is not really the case. We have a counter for the validation id.

How about 'validationCounter' ?


I noticed that you use hashCode as an id. I assume this is to simplify 
the implementation? That is probably OK, the risk of a collision is 
perhaps low, but could you make the field a long, so we in the future 
could add something more unique (Flight Recorder uses compressed 
integers, so a long uses the same amount of disk space as an int). 
Could you also rename the field and the annotation, perhaps 
certificationId could work, so we don't leak how the id was implemented.
Yes - originally, I was using the certificate serial number but that may 
not always be unique (esp. for test generated certs). I've changed the 
variable name to 'certificateId' and made it a long. Renamed the 
annotation also.


I could not find the file: 
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java

whoops - forgot to add it to mercurial tracking. there now :
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v8/webrev/

regards,
Sean.

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

[Seán Coffey]

With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code.

Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
  - we can consider a new event to record non-JDK security events if 
demand comes about

  - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

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

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


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

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be 
the case here, it's something we can't always assume. It's called 
once for the JFR operation and once for the Logger operation. That 
pattern multiplies itself further in other Events. i.e. set up and 
assign the variables for a JFR event without knowing whether they'll 
be needed again for the Logger call. We could work around it by 
setting up some local variables and re-using them in the Logger code 
but then, we're back to where we were. The current EventHelper design 
eliminates this problem and also helps to abstract the 
recording/logging functionality away from the functionality of the 
class itself.


It also becomes a problem where we record events in multiple 
different classes (e.g. SecurityPropertyEvent). While we could leave 
the code in EventHelper for this case, we then have a mixed design 
pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized some 
variable assignments in X509Impl.commitEvent(..)


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

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

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

[Seán Coffey]

I'd like to re-boot this review. I've been working with Erik off thread 
who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the @Relational 
JFR annotation.
* Keep calls for JFR event operations local to call site to optimize jvm 
compilation


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

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be the 
case here, it's something we can't always assume. It's called once for 
the JFR operation and once for the Logger operation. That pattern 
multiplies itself further in other Events. i.e. set up and assign the 
variables for a JFR event without knowing whether they'll be needed 
again for the Logger call. We could work around it by setting up some 
local variables and re-using them in the Logger code but then, we're 
back to where we were. The current EventHelper design eliminates this 
problem and also helps to abstract the recording/logging functionality 
away from the functionality of the class itself.


It also becomes a problem where we record events in multiple different 
classes (e.g. SecurityPropertyEvent). While we could leave the code in 
EventHelper for this case, we then have a mixed design pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized some 
variable assignments in X509Impl.commitEvent(..)


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

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

Re: How to detect that a VM was started with --enable-preview ?

[Seán Coffey]

would `jcmd  VM.info | grep jvm_args:` help ? or `jcmd  
VM.command_line`


regards,
Sean.

On 09/10/2018 16:20, Remi Forax wrote:

Hi all,
it seems that there is no simple way* to detect if a VM is started with 
--enable-preview or not ?

Rémi
* apart using JMX to sniff the VM command line with 
ManagementFactory.getRuntimeMXBean().getInputArguments()

Re: RFR[12] JDK-8211107: LDAPS communication failure with jdk 1.8.0_181

[Seán Coffey]

Correction looks good to me.

regards,
Sean.


On 02/10/2018 02:09, Prasadrao Koppula wrote:

Hi,

  


Could you please review this patch. This patch fixes the regression caused by 
LDAP fixes in JDK11, JDK8u181, JDK7u191

  


Webrev: http://cr.openjdk.java.net/~pkoppula/8211107/webrev.01/

Issue: https://bugs.openjdk.java.net/browse/JDK-8211107

  

  


Thanks,

Prasad.K

  

Re: RFR: JDK-8197398 (zipfs) OutOfMemoryError when talking contents of empty JAR file

[Seán Coffey]

This RFR should be removed from core-libs-dev. nio-dev is the correct 
mailing list.


The test needs to be fixed before that happens (and possibly the code also)

regards,
Sean.


On 25/09/2018 06:53, Deepak Kejriwal wrote:

Hi,

Gentle reminder for review below fix.

Regards,
Deepak

-Original Message-
From: Deepak Kejriwal
Sent: Friday, September 21, 2018 3:26 PM
To: core-libs-dev 
Subject: RFR: JDK-8197398 (zipfs) OutOfMemoryError when talking contents of 
empty JAR file

Hi all,

  


Please help review the changes for JDK-8199776 to JDK8u:-

  


Master Bug: https://bugs.openjdk.java.net/browse/JDK-8199776

Webrev: http://cr.openjdk.java.net/~rpatil/8199776/webrev.00/

  


Background:-

We have bug JDK-8197398 fixed in jdk 12 where Files.walkFileTree walk indefinitely while 
processing JAR file with "/" as a directory inside.

Since code for zipfs in jdk8dev different compared to jdk 12, the fix is not 
exactly same.

  


Proposed fix:

While iterating over the zip entries added a check to see if entry name is equal to "/" (absolute 
path). If the entry name is equal to "/" we don't add it to "inodes" map.

  


Regards,

Deepak

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

[Seán Coffey]

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be the 
case here, it's something we can't always assume. It's called once for 
the JFR operation and once for the Logger operation. That pattern 
multiplies itself further in other Events. i.e. set up and assign the 
variables for a JFR event without knowing whether they'll be needed 
again for the Logger call. We could work around it by setting up some 
local variables and re-using them in the Logger code but then, we're 
back to where we were. The current EventHelper design eliminates this 
problem and also helps to abstract the recording/logging functionality 
away from the functionality of the class itself.


It also becomes a problem where we record events in multiple different 
classes (e.g. SecurityPropertyEvent). While we could leave the code in 
EventHelper for this case, we then have a mixed design pattern.


Are you ok with leaving as is for now? A future wish might be one where 
JFR would handle Logger via its own framework/API in a future JDK release.


I've removed the variable names using underscore. Also optimized some 
variable assignments in X509Impl.commitEvent(..)


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

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

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

[Seán Coffey]

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

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

[Seán Coffey]

As per request from Erik, I separated the tests out into individual ones 
to test the JFR and Logger functionality. I introduced a new separate 
test for the CertificateChainEvent event also. Originally this was 
wrapped into the TLSHandshakeEvent test.


Thanks to Erik for extra refactoring and modifications carried out to 
clean up the code up further.


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

This enhancement has a dependency on  JDK-8203629

Regards,
Sean.

On 02/07/18 09:49, Erik Gahlin wrote:



On 29 Jun 2018, at 17:34, Seán Coffey  wrote:

I've introduced a new test helper class in the jdk/test/lib/jfr directory to 
help with the dual test nature of the new tests. It's helped alot with test 
code duplication.


My thinking was to put things like the certificates in a separate file, i.e 
TestCertificates, and then have a logging test and a JFR test reuse it.

One rationale for adding logging was to use it if JFR is not present. By 
putting the tests together, it becomes impossible to compile and test logging 
without having JFR.


Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. The output is displayed 
in units like "KiB" - not the normal when examining key lengths used in X509Certificates. 
i.e a 2048 bit key gets displayed as "2 KiB" - I'd prefer to keep the 2048 display 
version.

We should not let the JMC GUI decide how units are specified. There will be 
other GUIs and this is the first event that uses bits, so I don’t think it is 
formatted that way because it was considered superior.

Erik


new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/

Regards,
Sean.

On 28/06/18 17:59, Seán Coffey wrote:

Comments inline.


On 28/06/2018 17:20, Erik Gahlin wrote:

It's sufficient if an event object escapes to another method (regardless if JFR 
is enabled or not).

Some more feedback:

Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber

all above done.

Rename field algId to AlgorithmicId or AlgorithmicID

maybe "algorithm" works here also ?

Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the 
context?
Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e  @Category({"Java Development Kit", 
"Security"})

done.

Make classes final.

done. I had thought that the JFR mechanism required non-final classes.

What is the unit of the key in X509Certificate event? Bits? If that is the 
case, use @DataAmount(DataAmount.BITS)

Yes - it's essentially the bit length of the keys used. Let me look into that 
annotation usage.

@Label("Serial numbers forming chain of trust") should not be a sentence. How about 
"Serial Numbers"?

I think the tests are hard to read when two things are tested at the same time. 
It is also likely that if a test fail due to logging issues, it will be 
assigned to JFR because of the test name, even thought the issues is not JFR 
related.

I think we're always going to have some ownership issues when tests serve a 
dual purpose. I still think it makes sense to keep the test logic in one place. 
Any failures in these tests will most likely be owned by security team. (moving 
the tests to security directory is also an option)

If you want to reuse code between tests, I would put it in testlibrary.

Let me check if there's any common patterns that could be added to the 
testlibary.

Thanks,
Sean.

Erik


Thanks for the update Erik. By default I'm proposing that the new JFR Events 
and Logger be disabled. As a result the event class shouldn't escape. If 
performance metrics highlight an issue, we should revisit.

regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, 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.

The JIT 

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

[Seán Coffey]

I've introduced a new test helper class in the jdk/test/lib/jfr 
directory to help with the dual test nature of the new tests. It's 
helped alot with test code duplication.


Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's 
fits. The output is displayed in units like "KiB" - not the normal when 
examining key lengths used in X509Certificates. i.e a 2048 bit key gets 
displayed as "2 KiB" - I'd prefer to keep the 2048 display version.


new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/

Regards,
Sean.

On 28/06/18 17:59, Seán Coffey wrote:

Comments inline.


On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method 
(regardless if JFR is enabled or not).


Some more feedback:

Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber

all above done.

Rename field algId to AlgorithmicId or AlgorithmicID

maybe "algorithm" works here also ?

Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense 
in the context?

Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e  @Category({"Java Development Kit", 
"Security"})

done.

Make classes final.

done. I had thought that the JFR mechanism required non-final classes.
What is the unit of the key in X509Certificate event? Bits? If that 
is the case, use @DataAmount(DataAmount.BITS)
Yes - it's essentially the bit length of the keys used. Let me look 
into that annotation usage.
@Label("Serial numbers forming chain of trust") should not be a 
sentence. How about "Serial Numbers"?


I think the tests are hard to read when two things are tested at the 
same time. It is also likely that if a test fail due to logging 
issues, it will be assigned to JFR because of the test name, even 
thought the issues is not JFR related.
I think we're always going to have some ownership issues when tests 
serve a dual purpose. I still think it makes sense to keep the test 
logic in one place. Any failures in these tests will most likely be 
owned by security team. (moving the tests to security directory is 
also an option)


If you want to reuse code between tests, I would put it in testlibrary.
Let me check if there's any common patterns that could be added to the 
testlibary.


Thanks,
Sean.


Erik

Thanks for the update Erik. By default I'm proposing that the new 
JFR Events and Logger be disabled. As a result the event class 
shouldn't escape. If performance metrics highlight an issue, we 
should revisit.


regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, 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.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT 
should be able to remove the allocation.  This will be best from a 
performance point of view, and also in my opinion from a 
maintenance and readability perspective. Others may disagree.


Erik



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.jf

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

[Seán Coffey]

Comments inline.


On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method 
(regardless if JFR is enabled or not).


Some more feedback:

Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber

all above done.

Rename field algId to AlgorithmicId or AlgorithmicID

maybe "algorithm" works here also ?

Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense 
in the context?

Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e  @Category({"Java Development Kit", 
"Security"})

done.

Make classes final.

done. I had thought that the JFR mechanism required non-final classes.
What is the unit of the key in X509Certificate event? Bits? If that is 
the case, use @DataAmount(DataAmount.BITS)
Yes - it's essentially the bit length of the keys used. Let me look into 
that annotation usage.
@Label("Serial numbers forming chain of trust") should not be a 
sentence. How about "Serial Numbers"?


I think the tests are hard to read when two things are tested at the 
same time. It is also likely that if a test fail due to logging 
issues, it will be assigned to JFR because of the test name, even 
thought the issues is not JFR related.
I think we're always going to have some ownership issues when tests 
serve a dual purpose. I still think it makes sense to keep the test 
logic in one place. Any failures in these tests will most likely be 
owned by security team. (moving the tests to security directory is also 
an option)


If you want to reuse code between tests, I would put it in testlibrary.
Let me check if there's any common patterns that could be added to the 
testlibary.


Thanks,
Sean.


Erik

Thanks for the update Erik. By default I'm proposing that the new JFR 
Events and Logger be disabled. As a result the event class shouldn't 
escape. If performance metrics highlight an issue, we should revisit.


regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, 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.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT should 
be able to remove the allocation.  This will be best from a 
performance point of view, and also in my opinion from a maintenance 
and readability perspective. Others may disagree.


Erik



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)


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

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

[Seán Coffey]

Thanks for the update Erik. By default I'm proposing that the new JFR 
Events and Logger be disabled. As a result the event class shouldn't 
escape. If performance metrics highlight an issue, we should revisit.


regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, 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.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT should 
be able to remove the allocation.  This will be best from a 
performance point of view, and also in my opinion from a maintenance 
and readability perspective. Others may disagree.


Erik



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)


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 

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

[Seán Coffey]

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("Details of Security Property")

I would either remove the annotation,

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

[Seán Coffey]

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.


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)


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" 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.

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

[Seán Coffey]

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();
 r.start();

 CertificateFactory cf = 
CertificateFactory.getInstance("X.509");
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 // Make sure only one event per certificate
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 r.stop();

 List events = Events.fromRecording(r);
 Asserts.assertEquals(events

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

[Seán Coffey]

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();
 r.start();

 CertificateFactory cf = CertificateFactory.getInstance("X.509");
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 // Make sure only one event per certificate
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 r.stop();

 List events = Events.fromRecording(r);
 Asserts.assertEquals(events.size(), 2, "Expected two X.509 
Certificate events");


 assertEvent(events, "1000", "SHA256withRSA",
    "CN=SSLCertificate, O=SomeCompany",
    "CN=Intermediate CA Cert, O=SomeCompany",
 "RSA", 2048);
 assertEvent(events, "1000", "SHA256withRSA",
    "CN=SSLCertificate, O=SomeCompany",
    "CN=Intermediate CA Cert, O=SomeCompany",
 "RSA", 2048);
    }

    private static void assertEvent(List events, String 
certID, String algId, String subject,

    String issuer, String keyType, int length) throws Exception {

    for (RecordedEvent e : events) {
    if (e.getString("serialNumber").equals(certID)) {
    Events.assertField(e, "algId").equal(algId);
    ...
    return;
    }
    }
    System.out.println(events);
    throw new Exception("Could not find event with Cert ID: " + 
certID);

    }
}

The reworked 

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

[Seán Coffey]

Following on from the recent JDK-8203629 code review, I'd like to 
propose enhancements on how we can record events in security libs. The 
introduction of the JFR libraries can give us much better ways of 
examining JDK actions. For the initial phase, I'm looking to enhance 
some key security library events in JDK 11 so that they can be either 
recorded to JFR, logged to a traditional logger, or both.


Examples of how useful JFR recordings could be can be seen here :

http://cr.openjdk.java.net/~coffeys/event_snaps/X509Event_1.png
http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_1.png
http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_2.png
http://cr.openjdk.java.net/~coffeys/event_snaps/TLSEvent_1.png

securityProp_2.png gives an example of how the JFR recording can be 
queried to quickly locate events of interest (in this case, code setting 
the jdk.tls.* Security properties). I still need to clean up the 
TLSEvents testcase to improve test coverage and hope to do that in 
coming days.


JBS record :
 * https://bugs.openjdk.java.net/browse/JDK-8148188

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

--
Regards,
Sean.

Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Seán Coffey]

Thanks for implementing this enhancement Erik. I think it'll prove to be 
popular given the functionality available in JFR recordings. I'd agree 
with Alan's comment that the Event class could be documented better to 
indicate its purpose.


I'm already using this enhancement to implement some new events in the 
JDK security libraries. I hope to get a review started real soon on that 
front.


Regards,
Sean.

On 19/06/18 03:06, Erik Gahlin wrote:

Hi,

Could I have a review of an enhancement that will make it possible to 
add JFR events to java.base, and possibly other modules in the JDK, 
without a compile time dependency on jdk.jfr.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629

Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0

Testing:
Tests in test/jdk/jdk/jfr

The functionality is a prerequisite for an upcoming enhancement that 
will add JFR events to the security libraries.


In short, jdk.jfr.Event (located in jdk.jfr) extends 
jdk.internal.event.Event (located in java.base). Metadata for events, 
such as labels and descriptions, are added using a mirror event class 
located in jdk.jfr module. If the fields of the mirror class and the 
event class don't match an InternalError is thrown.


To illustrate how the mechanism can be used, see the following example 
(not meant to be checked in).


http://cr.openjdk.java.net/~egahlin/8203629.example

Since the implementation of jdk.internal.event.Event is empty, the JIT 
will typically eliminate all traces of JFR functionality unless Flight 
Recorder is enabled.


Thanks
Erik

Re: [RFR] 8204539: improve error messages in matchJavaTZ [windows]

[Seán Coffey]

Looks good. Thanks for adding this improvement.

Regards,
Sean.

On 08/06/18 12:28, Baesken, Matthias wrote:

Hi Goetz/Christoph, thanks for  the reviews  .
However of course Sean is absolutely  correct about the  null character message 
output.
Updated the null character related  message output :

http://cr.openjdk.java.net/~mbaesken/webrevs/8204539.2/

Best regards, Matthias



-Original Message-
From: Lindenmaier, Goetz
Sent: Freitag, 8. Juni 2018 09:34
To: Baesken, Matthias ; Langer, Christoph
; core-libs-dev@openjdk.java.net;
'sean.cof...@oracle.com' 
Subject: RE: [RFR] 8204539: improve error messages in matchJavaTZ
[windows]

Hi Matthias,

thanks for adding this information, looks good!

Best regards,
   Goety.


-Original Message-
From: Baesken, Matthias
Sent: Freitag, 8. Juni 2018 08:51
To: Langer, Christoph ; core-libs-
d...@openjdk.java.net; 'sean.cof...@oracle.com'

Cc: Lindenmaier, Goetz 
Subject: RE: [RFR] 8204539: improve error messages in matchJavaTZ
[windows]

Hi,  looks like I uploaded a wrong version of the patch.

Updated webrev is here  :



http://cr.openjdk.java.net/~mbaesken/webrevs/8204539.1/



I followed the advice of Sean and  removed  the  mapFileName   from the
output because it is usually a static name .

Updated webrev was going through our internal build/tests .





Best regards, Matthias





From: Langer, Christoph
Sent: Donnerstag, 7. Juni 2018 13:52
To: Baesken, Matthias ; core-libs-
d...@openjdk.java.net
Cc: Lindenmaier, Goetz 
Subject: RE: [RFR] 8204539: improve error messages in matchJavaTZ
[windows]



Hi Matthias,



in line 527, where the actual output is done, I think you would need to
replace variable 'message' with 'outputMessage', otherwise I guess it

won't

compile??



Also, mapFileName can't be used at this place, because it has already been
free'ed there.



But in general the additions make sense and will make it easier to find

issues

in the tzmappings file.



Best regards

Christoph



From: Baesken, Matthias
Sent: Donnerstag, 7. Juni 2018 13:35
To: core-libs-dev@openjdk.java.net 
Cc: Langer, Christoph mailto:christoph.lan...@sap.com> >; Lindenmaier, Goetz
mailto:goetz.lindenma...@sap.com> >
Subject: [RFR] 8204539: improve error messages in matchJavaTZ [windows]



Hi, could you please  review this small  change that improves  the error
messages in  matchJavaTZ .

A reason of the failure is added to the message , and also  the offset where
the error happened .





Bug :



https://bugs.openjdk.java.net/browse/JDK-8204539





Webrev :



http://cr.openjdk.java.net/~mbaesken/webrevs/8204539/





Thanks, Matthias

Re: [RFR] 8204539: improve error messages in matchJavaTZ [windows]

[Seán Coffey]

Not sure if this has been tested.

Don't you need to escape the printing of the \0 character ?

errorMessage = "illegal character \0 found";

regards,
Sean.


On 08/06/2018 08:14, Langer, Christoph wrote:


Hi Matthias,

this looks good. Reviewed from my end.

Best regards

Christoph

*From:*Baesken, Matthias
*Sent:* Freitag, 8. Juni 2018 08:51
*To:* Langer, Christoph ; 
core-libs-dev@openjdk.java.net; 'sean.cof...@oracle.com' 


*Cc:* Lindenmaier, Goetz 
*Subject:* RE: [RFR] 8204539: improve error messages in matchJavaTZ 
[windows]


Hi,  looks like I uploaded a wrong version of the patch.

Updated webrev is here  :

http://cr.openjdk.java.net/~mbaesken/webrevs/8204539.1/ 



I followed the advice of Sean and  removed  the  mapFileName   from 
the output because it is usually a static name .


Updated webrev was going through our internal build/tests .

Best regards, Matthias

*From:*Langer, Christoph
*Sent:* Donnerstag, 7. Juni 2018 13:52
*To:* Baesken, Matthias >; core-libs-dev@openjdk.java.net 

*Cc:* Lindenmaier, Goetz >
*Subject:* RE: [RFR] 8204539: improve error messages in matchJavaTZ 
[windows]


Hi Matthias,

in line 527, where the actual output is done, I think you would need 
to replace variable ‘message’ with ‘outputMessage’, otherwise I guess 
it won’t compile??


Also, mapFileName can’t be used at this place, because it has already 
been free’ed there.


But in general the additions make sense and will make it easier to 
find issues in the tzmappings file.


Best regards

Christoph

*From:*Baesken, Matthias
*Sent:* Donnerstag, 7. Juni 2018 13:35
*To:* core-libs-dev@openjdk.java.net 

*Cc:* Langer, Christoph >; Lindenmaier, Goetz 
mailto:goetz.lindenma...@sap.com>>

*Subject:* [RFR] 8204539: improve error messages in matchJavaTZ [windows]

Hi, could you please  review this small  change that improves  the 
error messages in matchJavaTZ .


A reason of the failure is added to the message , and also  the offset 
where the error happened .


Bug :

https://bugs.openjdk.java.net/browse/JDK-8204539

Webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8204539/ 



Thanks, Matthias

Re: [RFR] 8204539: improve error messages in matchJavaTZ [windows]

[Seán Coffey]

On 07/06/2018 13:03, Baesken, Matthias wrote:

Hi Christoph you are correct I compiled it on the wrong platforms , stupid 
mistake !
Will send an updated web rev .


   *   Also, mapFileName can't be used at this place, because it has already 
been free'ed there.

Yes, makes sense. Will move the free-calls .
Please make sure you run some manual tests also to ensure the message 
looks correct. Not sure if you really need mapFileName to be printed - 
it's generally a static file within java.home


regards,
Sean.


Best regards, Matthias

From: Langer, Christoph
Sent: Donnerstag, 7. Juni 2018 13:52
To: Baesken, Matthias ; core-libs-dev@openjdk.java.net
Cc: Lindenmaier, Goetz 
Subject: RE: [RFR] 8204539: improve error messages in matchJavaTZ [windows]

Hi Matthias,

in line 527, where the actual output is done, I think you would need to replace 
variable 'message' with 'outputMessage', otherwise I guess it won't compile??

Also, mapFileName can't be used at this place, because it has already been 
free'ed there.

But in general the additions make sense and will make it easier to find issues 
in the tzmappings file.

Best regards
Christoph

From: Baesken, Matthias
Sent: Donnerstag, 7. Juni 2018 13:35
To: core-libs-dev@openjdk.java.net
Cc: Langer, Christoph mailto:christoph.lan...@sap.com>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: [RFR] 8204539: improve error messages in matchJavaTZ [windows]

Hi, could you please  review this small  change that improves  the error 
messages in  matchJavaTZ .
A reason of the failure is added to the message , and also  the offset where 
the error happened .


Bug :

https://bugs.openjdk.java.net/browse/JDK-8204539


Webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8204539/


Thanks, Matthias

Re: [8u-dev] Request for Review and Approval to Backport 8203368: ObjectInputStream filterCheck method throws NullPointerException

[Seán Coffey]

Looks fine.

Approved for jdk8u-dev.

regards,
Sean.


On 31/05/2018 02:40, Ivan Gerasimov wrote:

Hello!

I'd like to backport the fix to JDK 8u-dev.

Both source patch and the regression test had to be adjusted slightly 
due to the code divergence, thus the review request.


Bug: https://bugs.openjdk.java.net/browse/JDK-8203368

Jdk 8u webrev: http://cr.openjdk.java.net/~igerasim/8203368/00/webrev/

Jdk 11 changeset: http://hg.openjdk.java.net/jdk/jdk/rev/697b49a04e19

Jdk 11 review: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/053318.html



Thanks in advance!

Re: RFR: 8201609 - Split test/jdk/:tier2 to enable better parallel execution

[Seán Coffey]

The Cipher tests were re-grouped via 
https://bugs.openjdk.java.net/browse/JDK-8132855


NIO re-grouped via https://bugs.openjdk.java.net/browse/JDK-8132854

All appears connected with exercising hotspot intrinsics in tier1 testing.

regards,
Sean.


On 17/04/2018 08:10, Alan Bateman wrote:



On 16/04/2018 21:20, Christian Tornqvist wrote:
Do you know what sun/nio/cs/ISO8859x.java and 
com/sun/crypto/provider/Cipher are special cased? Just wondering if 
there is a historical or current reason to not run those.



No idea.
It would be useful to find out why is was setup like this, if we can. 
The reason is that it will come up every time there is re-balancing of 
the tiers.


-Alan

RFR : 8196854 :TestFlushableGZIPOutputStream failing with IndexOutOfBoundsException

[Seán Coffey]

A jdk8u (and earlier) issue where some new/recent test code meant that 
IndexOutOfBoundsException could be thrown.


https://bugs.openjdk.java.net/browse/JDK-8196854
http://cr.openjdk.java.net/~coffeys/webrev.8196854/webrev/

The buf array was never required and I've verified that the original 
8189789 issue can still be reproduced with a buggy JDK.


--
Regards,
Sean.

Re: JDK-8166339,Code conversion working behavior was changed for x-IBM834

[Seán Coffey]

Looks fine to me Sherman.

regards,
Sean.


On 30/01/2018 22:59, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8166339.

issue: https://bugs.openjdk.java.net/browse/JDK-8166339
webrev: http://cr.openjdk.java.net/~sherman/8166339/webrev

This is a regression triggered by

issue: https://bugs.openjdk.java.net/browse/JDK-8008386
webrev: http://cr.openjdk.java.net/~sherman/8008386/webrev

which updated the doublebyte decoder implementation to handle unmappable
bytes more "appropriately" ( malformed (1) or unmappable(2) ).

However in case of Cp834, which is a doublebyte-only charset, the 
unmappable
bytes should always be treated as double-byte pair, therefor always 
unmappable(2).


Thanks
Sherman

Re: RFR[JDK10]: 8195837: (tz) Support tzdata2018c

[Seán Coffey]

The changes look fine to me Ramanand. The europe file contains some 
interesting comments about the rolled back changes that have been made 
for 2018c. A plan on how to resolve these pending changes can be 
followed up via JDK-8195595


Regards,
Sean.

On 29/01/18 08:49, Ramanand Patil wrote:

Hi all,
Please review the latest TZDATA integration (tzdata2018c) into JDK10.
Bug: https://bugs.openjdk.java.net/browse/JDK-8195837
Webrev: http://cr.openjdk.java.net/~rpatil/8195837/webrev.00/

All the TimeZone related tests are passed after integration.

Regards,
Ramanand.

Re: Oracle Java 8u161 regression in XML Schema Factory

[Seán Coffey]

I don't see them being related with data given below. Maybe there's a 
classpath configuration issue ?


If you feel there's a bug in the JDK libraries, please report it via 
https://bugs.java.com/


regards,
Sean.

On 26/01/2018 00:23, Krystal Mok wrote:

Hi guys,

A coworker of mine had hit this issue last night on 8u161 and it 
worked fine on 8u151:


ERROR: 
/home/myuser/.cache/bazel/_bazel_myuser/some_hash_code/external/jackson_datatype_joda_shaded/BUILD:5:1: 
Building 
external/jackson_datatype_joda_shaded/libjackson-datatype-joda-class.jar 
(35 source files) failed (Exit 1)
java.lang.InternalError: Cannot find requested resource bundle for 
locale en_US
        at 
com.sun.tools.javac.util.JavacMessages.getBundles(JavacMessages.java:128)
        at 
com.sun.tools.javac.util.JavacMessages.getLocalizedString(JavacMessages.java:147)
        at 
com.sun.tools.javac.util.JavacMessages.getLocalizedString(JavacMessages.java:140)

        at com.sun.tools.javac.util.Log.localize(Log.java:788)
        at com.sun.tools.javac.util.Log.printLines(Log.java:586)
        at 
com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:170)
        at 
com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:96)
        at 
com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:90)
        at 
com.google.devtools.build.buildjar.javac.BlazeJavacMain.compile(BlazeJavacMain.java:105)
        at 
com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder$1.invokeJavac(SimpleJavaLibraryBuilder.java:106)
        at 
com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.compileSources(ReducedClasspathJavaLibraryBuilder.java:53)
        at 
com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.compileJavaLibrary(SimpleJavaLibraryBuilder.java:109)
        at 
com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.run(SimpleJavaLibraryBuilder.java:117)
        at 
com.google.devtools.build.buildjar.BazelJavaBuilder.processRequest(BazelJavaBuilder.java:105)
        at 
com.google.devtools.build.buildjar.BazelJavaBuilder.runPersistentWorker(BazelJavaBuilder.java:67)
        at 
com.google.devtools.build.buildjar.BazelJavaBuilder.main(BazelJavaBuilder.java:45)
Caused by: java.util.MissingResourceException: Can't find bundle for 
base name com.google.errorprone.errors, locale en_US
        at 
java.util.ResourceBundle.throwMissingResourceException(ResourceBundle.java:1573)
        at 
java.util.ResourceBundle.getBundleImpl(ResourceBundle.java:1396)

        at java.util.ResourceBundle.getBundle(ResourceBundle.java:854)
        at 
com.sun.tools.javac.util.JavacMessages.lambda$add$0(JavacMessages.java:106)
        at 
com.sun.tools.javac.util.JavacMessages.getBundles(JavacMessages.java:125)

        ... 15 more

Resource bundle loading issue...could that be related to this XML 
issue here?


Thanks,
Kris

On Thu, Jan 25, 2018 at 8:41 AM, Seán Coffey <sean.cof...@oracle.com 
<mailto:sean.cof...@oracle.com>> wrote:


On 25/01/2018 11:58, Bernd wrote:

Hello,

some of our unit tests (using PowerMock and xmlunit) fail with
8u161 (and
u162) but work with 8u152.

I cant reproduce the fault in a stand-alone program so it
seems to be
related to classloader/reflection magic of those tools, sorry.

Is this a regression of 8159240
<http://bugs.java.com/view_bug.do?bug_id=JDK-8159240
<http://bugs.java.com/view_bug.do?bug_id=JDK-8159240>> (not
public?)

Fixes in the CPU releases are kept private - hence the above bug
is not public. The changesets do become public once the release is
made public though. See :
http://hg.openjdk.java.net/jdk8u/jdk8u/jaxws/rev/06086cb6c349
<http://hg.openjdk.java.net/jdk8u/jdk8u/jaxws/rev/06086cb6c349>

I don't think it's a factor for what you're seeing.

Classes nearer to those below were touched via JDK-8186080:
Transform XML interfaces
http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/cb84156d54b2
<http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/cb84156d54b2>
http://hg.openjdk.java.net/jdk8u/jdk8u/jaxp/rev/08a44c164993
<http://hg.openjdk.java.net/jdk8u/jdk8u/jaxp/rev/08a44c164993>

This may be connected with some tools trying to redefine the
classes perhaps. Needs more investigating. Perhaps the
XMLSchemaLoader changes are a factor ?

regards,
Sean.


Here is the stacktrace anyway:

com.sun.org.apache.xerces.internal.impl.dv.DVFactoryException:
Schema
factory class
com.sun.org.apache.xerces.internal.impl.dv.xs.SchemaDVFactoryImpl
does not
extend from SchemaDVFactory.
     at

com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.getInstance(SchemaDVFactory.java:75)
     at

com.sun.org.apache.xerces.internal.impl.dv.SchemaDVF

Re: Oracle Java 8u161 regression in XML Schema Factory

[Seán Coffey]

On 25/01/2018 11:58, Bernd wrote:


Hello,

some of our unit tests (using PowerMock and xmlunit) fail with 8u161 (and
u162) but work with 8u152.

I cant reproduce the fault in a stand-alone program so it seems to be
related to classloader/reflection magic of those tools, sorry.

Is this a regression of 8159240
 (not public?)
Fixes in the CPU releases are kept private - hence the above bug is not 
public. The changesets do become public once the release is made public 
though. See : http://hg.openjdk.java.net/jdk8u/jdk8u/jaxws/rev/06086cb6c349


I don't think it's a factor for what you're seeing.

Classes nearer to those below were touched via JDK-8186080: Transform 
XML interfaces

http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/cb84156d54b2
http://hg.openjdk.java.net/jdk8u/jdk8u/jaxp/rev/08a44c164993

This may be connected with some tools trying to redefine the classes 
perhaps. Needs more investigating. Perhaps the XMLSchemaLoader changes 
are a factor ?


regards,
Sean.


Here is the stacktrace anyway:

com.sun.org.apache.xerces.internal.impl.dv.DVFactoryException: Schema
factory class
com.sun.org.apache.xerces.internal.impl.dv.xs.SchemaDVFactoryImpl does not
extend from SchemaDVFactory.
 at
com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.getInstance(SchemaDVFactory.java:75)
 at
com.sun.org.apache.xerces.internal.impl.dv.SchemaDVFactory.getInstance(SchemaDVFactory.java:57)
 at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.reset(XMLSchemaLoader.java:1024)
 at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.loadGrammar(XMLSchemaLoader.java:556)
 at
com.sun.org.apache.xerces.internal.impl.xs.XMLSchemaLoader.loadGrammar(XMLSchemaLoader.java:535)
 at
com.sun.org.apache.xerces.internal.jaxp.validation.XMLSchemaFactory.newSchema(XMLSchemaFactory.java:254)
 at javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.java:638)
 at javax.xml.validation.SchemaFactory.newSchema(SchemaFactory.java:654)
 at
com.seeburger.api.test.helpers.BuilderTestHelper.getCRSchemaValidator(BuilderTestHelper.java:57)
 at
com.seeburger.api.test.helpers.BuilderTestHelper.validateAndCompare(BuilderTestHelper.java:73)
 at
com.seeburger.api.test.KSMBuilderTest.testDeletePGP(KSMBuilderTest.java:196)
 at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
 at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
 at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
 at java.lang.reflect.Method.invoke(Method.java:498)
 at org.junit.internal.runners.TestMethod.invoke(TestMethod.java:68)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:310)
 at org.junit.internal.runners.MethodRoadie$2.run(MethodRoadie.java:89)
 at
org.junit.internal.runners.MethodRoadie.runBeforesThenTestThenAfters(MethodRoadie.java:97)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.executeTest(PowerMockJUnit44RunnerDelegateImpl.java:294)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTestInSuper(PowerMockJUnit47RunnerDelegateImpl.java:127)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit47RunnerDelegateImpl$PowerMockJUnit47MethodRunner.executeTest(PowerMockJUnit47RunnerDelegateImpl.java:82)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$PowerMockJUnit44MethodRunner.runBeforesThenTestThenAfters(PowerMockJUnit44RunnerDelegateImpl.java:282)
 at org.junit.internal.runners.MethodRoadie.runTest(MethodRoadie.java:87)
 at org.junit.internal.runners.MethodRoadie.run(MethodRoadie.java:50)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.invokeTestMethod(PowerMockJUnit44RunnerDelegateImpl.java:207)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.runMethods(PowerMockJUnit44RunnerDelegateImpl.java:146)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl$1.run(PowerMockJUnit44RunnerDelegateImpl.java:120)
 at
org.junit.internal.runners.ClassRoadie.runUnprotected(ClassRoadie.java:34)
 at
org.junit.internal.runners.ClassRoadie.runProtected(ClassRoadie.java:44)
 at
org.powermock.modules.junit4.internal.impl.PowerMockJUnit44RunnerDelegateImpl.run(PowerMockJUnit44RunnerDelegateImpl.java:122)
 at
org.powermock.modules.junit4.common.internal.impl.JUnit4TestSuiteChunkerImpl.run(JUnit4TestSuiteChunkerImpl.java:106)
 at
org.powermock.modules.junit4.common.internal.impl.AbstractCommonPowerMockRunner.run(AbstractCommonPowerMockRunner.java:53)
 at
org.powermock.modules.junit4.PowerMockRunner.run(PowerMockRunner.java:59)
   

Re: Adding SocketChannel toString to connection exception messages

[Seán Coffey]

As someone who works with alot of log files, I'd like to chime in and 
support Steven's end goal. Looking at a "Connection refused" error in 
the middle of a log file that possibly extends to millions of lines is 
near useless. In the era of cloud compute, diagnosing network issues is 
sure to become a more common task.


While we may not be able to put the sensitive information in an 
exception message, I think we could put it behind a (new?) system 
property which might be able to log this information. Logs contain all 
sorts of sensitive data. Take javax.net.debug=ssl output for example.


Regards,
Sean.

On 22/12/17 09:57, Chris Hegarty wrote:

On 22/12/17 01:27, David Holmes wrote:

cc'ing net-dev as that might be the more appropriate list.

On 22/12/2017 10:59 AM, Steven Schlansker wrote:


On Dec 21, 2017, at 4:35 PM, David Holmes  
wrote:


On 22/12/2017 10:29 AM, Steven Schlansker wrote:
On Dec 21, 2017, at 11:11 AM, Steven Schlansker 
 wrote:


What if ConnectException included the attempted hostname / IP / 
port SocketAddress?
java.net.ConnectException: Connection to 
'foo.mycorp.com[10.x.x.x]:12345' refused
Much more useful!  This could also be extended to various other 
socket exceptions.


I believe there are concerns with too much information that can be 
considered "sensitive" (like host names and IP addresses) appearing 
in error messages due to them ending up in log files and bug reports.


Unfortunately that's exactly the information that is crucial to 
someone trying to diagnose issues...


And to someone trying to discern private information about a network.


Could it be an opt-in policy?  Perhaps by a system property?


Well, you don't know where a log file or exception will end up.
Most likely on stackoverflow.

The expectation is that such information should be added by layers 
higher in the call chain, rather than the JDK libraries assuming it 
is okay to do.


Agreed. It may be some work, but if the issue is significant
enough to the user then it may be worth their effort, as well
as affording the opportunity to opt-out at any particular
point in the code.

Currently the alternative I'm faced with is going through every 
piece of user code and library that *might*
throw this exception and wrapping it to add this critical diagnostic 
information.  For an application that uses
java.net heavily, you can imagine how that is a tall task and 
possibly even not realistically achievable...


(Is there a written policy regarding this somewhere, or is it up to 
the personal feelings of the contributors?)


This is covered by the secure coding guidelines, under 'Confidential 
information':


http://www.oracle.com/technetwork/java/seccodeguide-139067.html#2

"Guideline 2-1 / CONFIDENTIAL-1: Purge sensitive information from 
exceptions"


Exactly.

I know for a fact that we'd have to scrub this information from test 
failures when putting the info into a bug report.


If net-dev folk can't expand further on this then I suggest filing a 
CSR request so that the CSR group can consider it. But I think this 
will be a no-go (I'm a member of the CSR group).


I have to agree with David here. I don't think that such a request
could be supported.

-Chris.

Re: RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit

[Seán Coffey]

Looks fine to me.

regards,
Sean.


On 18/12/2017 11:41, Ramanand Patil wrote:

Hi all,
Please review the fix for JDK8u Backport of 
https://bugs.openjdk.java.net/browse/JDK-8153955
Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266
Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/

Summary(also added to backport bug description):
The fix from JDK9 cannot be backported as is into the jdk8u and earlier update 
releases, since it contains JDK API spec changes.
In JDK9 the fix is added by altering the FileHandler spec, which introduces a new 
configurable property "java.util.logging.FileHandler.maxLocks" to 
java.util.logging.FileHandler, defined in .../conf/logging.properties.
The solution proposed for update releases is:
To introduce an internal JDK implementation specific property- 
"jdk.internal.FileHandlerLogging.maxLocks" which will be used to 
control/override FileHandler's MAX_LOCKS value. The default value of the maxLocks (100) 
will be retained if this new System property is not set. The new property is read only 
once during FileHandler class initialization.


Regards,
Ramanand.

Re: RFR JDK-8191918: tomcat gzip-compressed response bodies appear to be broken in update 151

[Seán Coffey]

Looks good to me.

Regards,
Sean.

On 02/12/17 01:04, Xueming Shen wrote:

On 12/1/17, 3:40 PM, Paul Sandoz wrote:



On 30 Nov 2017, at 14:46, Xueming Shen  wrote:

Hi,

Please help review the change for  JDK-8191918:

issue: https://bugs.openjdk.java.net/browse/JDK-8191918
webrev: http://cr.openjdk.java.net/~sherman/8191918/webrev


InflateIn_DeflateOut.java
—

  174 GZIPInputStream gzis = new GZIPInputStream(byteInStream);
  175 ByteArrayOutputStream baos = new ByteArrayOutputStream();
  176 int numRead;
  177 byte[] b = new byte[4 * 1024];
  178 try {
  179 while ((numRead = gzis.read(buf))>= 0) {
  180 baos.write(b, 0, numRead);
  181 }
  182 } finally {
  183 baos.close();
  184 }

Can you use gzis.transferTo(baos)?

Paul.




Thanks Paul!

webrev has been updated as suggested.

http://cr.openjdk.java.net/~sherman/8191918/webrev

-Sherman

Re: RFR: JDK8U Backport of 8154017: Shutdown hooks are racing against shutdown sequence, if System.exit()-calling thread is interrupted

[Seán Coffey]

Looks fine.

Regards,
Sean.

On 27/11/17 08:17, Muneer Kolarkunnu wrote:

Hi All,
  
Please review this webrev for jdk8u backport.

Webrev: http://cr.openjdk.java.net/~akolarkunnu/8191289/webrev.00/
  
Main Bug: https://bugs.openjdk.java.net/browse/JDK-8154017

JDK10 review thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041952.html
JDK10 changeset: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/ceeea7370651
  
Change w.r.t JDK9 changeset: Changed only throws Exception to throws Throwable in test case,  as jdk.testlibrary.ProcessTools.executeProcess() throws Throwable in JDK8.

public static void main(String[] args) throws Throwable {
  
Testing: Tested new regression test and all core-libs regression tests on all platforms - there are no new failures.
  
Regards,

Muneer

Re: 8189789: tomcat gzip-compressed response bodies appear to be broken in update 151

[Seán Coffey]

On 22/11/2017 15:46, Hohensee, Paul wrote:

If the fix is from the zlib project, then ignore the following, since it’s 
their code.

In Deflater.c: the existing significant code duplication between the arms of 
the if-else gives one pause. E.g., in the new file, compare lines 148/183 and 
155/190. Might be bugs lurking at 183 and 190.
There might be some room for optimizing alright. The different length 
variables are measuring the available input and output length values. 
Different action is taken if we encounter exceptions when setting up 
memory for related operations. Maybe we can bring more consistent length 
checks to both blocks. I'd have to check why we check for native 
exception handling in one block and not the other.


In deflate.c: the old code had “s->last_flush = Z_NO_FLUSH”, the new has “-2”. 
Should “-2” be symbolic instead?

Sherman is following the code from the main zlib repo for that change [1]

regards,
Sean.

[1] https://github.com/madler/zlib/issues/275


Thanks,

Paul

On 11/22/17, 2:00 AM, "core-libs-dev on behalf of Seán Coffey" 
<core-libs-dev-boun...@openjdk.java.net on behalf of sean.cof...@oracle.com> wrote:

 Looking to fix a recent regression that appeared in 8u151. Thanks goes
 to Sherman for the src patch. Paul Hohensee has also helped by provided
 some test code. I've been able to modify an existing test to mimic the
 behaviour seen with FlushableGZIPOutputStream which is found in Tomcat 7.x.
 
 Sherman is working on a patch for JDK 10. For unix OSes, JDK 10 operates

 with the zlib version found on the OS.
 
 bug : https://bugs.openjdk.java.net/browse/JDK-8189789

 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8189789.jdk8u/webrev/
 
 regards,

 Sean.
 
 

RFR: 8189789: tomcat gzip-compressed response bodies appear to be broken in update 151

[Seán Coffey]

Looking to fix a recent regression that appeared in 8u151. Thanks goes 
to Sherman for the src patch. Paul Hohensee has also helped by provided 
some test code. I've been able to modify an existing test to mimic the 
behaviour seen with FlushableGZIPOutputStream which is found in Tomcat 7.x.


Sherman is working on a patch for JDK 10. For unix OSes, JDK 10 operates 
with the zlib version found on the OS.


bug : https://bugs.openjdk.java.net/browse/JDK-8189789
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8189789.jdk8u/webrev/

regards,
Sean.

Re: Incorrect validation of DST in java.util.SimpleTimeZone

[Seán Coffey]

I think the OpenJDK mailing lists accept attachments if in patch format. 
If it's a simple short patch, it's acceptable to paste it into email body.


Easiest solution is to use webrev[1]. If you can't upload this to 
cr.openjdk.java.net - then one of your colleagues may be able to help.


[1] http://openjdk.java.net/guide/webrevHelp.html

Regards,
Sean.

On 10/11/17 12:18, Venkateswara R Chintala wrote:
Looks like the patch attached earlier is not visible. As this is my 
first contribution, please let me know how I can send the patch for 
review.



On 10/11/17 5:37 PM, Venkateswara R Chintala wrote:

Hi,

In a multi-threaded environment, when java.util.SimpleTimeZone object 
is used to create a default timezone, there can be a race condition 
between the methods java.util.Timezone.getDefault() and 
java.util.Timezone.getDefaultRef() which can result in inconsistency 
of cache that is used to validate a particular time/date in DST.


When a thread is cloning a default timezone object (SimpleTimeZone) 
and at the same time if a different thread modifies the time/year 
values, then the cache values (cacheYear, cacheStart, cacheEnd) can 
become inconsistent which leads to incorrect DST determination.


We considered two approaches to fix the issue.

1)Synchronize access to cloning default timezone object when cache is 
being modified.


2)Invalidate the cache while returning the clone.

We preferred the second option as synchronization is more expensive.

We have attached the patch and jtreg testcase. Please review.

Re: [8u-dev][JAXB] Request for review and approval: 8159240: XSOM parser incorrectly processes type names with whitespaces

[Seán Coffey]

This looks fine Aleksei.  Approved for jdk8u-dev.

The JDK-8159240 record is currently closed as a duplicate. I'd suggest 
you reopen that master record for your 8u-dev port and mark it 9-na.


regards,
Sean.

On 27/09/2017 22:28, Aleks Efimov wrote:

Hi,

Can I, please, ask for review and approval of JDK-8159240 backport to 
JDK8? This bug was resolved in JDK9 as part of JAXWS-RI sync changeset 
(JDK-8164479) therefore separate review for the partially backported 
changes is needed: http://cr.openjdk.java.net/~aefimov/8159240/8/00/


File changes partially backported to JDK8 from JDK9 sync changeset:
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l37.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l38.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l39.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l40.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l41.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l42.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l43.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l44.1
http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052#l45.1

Testing:
    New regression test and all jaxb/ws related JCK/JTREG tests shows 
no failures.


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

JAXWS-RI sync bug:
    https://bugs.openjdk.java.net/browse/JDK-8164479

JDK9 JAXWS-RI sync changeset:
    http://hg.openjdk.java.net/jdk9/dev/jaxws/rev/26c9b9c51052

JDK9 JAXWS-RI sync review threads:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-September/043724.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/043920.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-November/044513.html 



JAXWS-RI changes for XSOM parser:
https://github.com/javaee/jaxb-v2/commit/cc9c09a71d739989d1e250d9db8c1e011215abfd#diff-796464168dc0014203b00c9728d41de8 



With Best Regards,
Aleksei

Re: RFR: JDK8U Backport of 8185346: Relax RMI Registry Serial Filter to allow arrays of any type

[Seán Coffey]

Looks good.

Regards,
Sean.

On 17/08/17 20:06, Ramanand Patil wrote:

Hi All,

Please review this webrev for jdk8u backport.
Webrev: http://cr.openjdk.java.net/~rpatil/8185346/jdk8u-dev/webrev.00/

Main Bug: https://bugs.openjdk.java.net/browse/JDK-8185346
JDK10 review thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-August/048738.html
JDK10 changeset: http://hg.openjdk.java.net/jdk10/jdk10/jdk/rev/6256e94781f5

The usage of shared secrets is removed as compared to the original fix since 
the ObjectInputFilter is accessible in the jdk.internal.misc package.


Regards,
Ramanand.

Re: RFR 8185346 : Relax RMI Registry Serial Filter to allow arrays of any type

[Seán Coffey]

Looks fine to me Roger. Thanks for handling.

Should the testcase get an @bug tag ?

regards,
Sean.


On 03/08/2017 15:25, Roger Riggs wrote:
Please review a relaxation of the serial filter checks on the RMI 
Registry.


Arrays of any type are serializable and do not pose a risk except 
based on the array size,
which is checked by the maxarray limit. Objects inserted in the array 
are checked Individually.


The filter is modified to check only the array size against the limit 
and otherwise allow arrays of any type.
Both the built-in filter and the pattern based override are updated to 
allow arrays.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-registry-allow-arrays-8185346/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8185346

Thanks, Roger

Re: 8179527:(8182743?) Ineffective use of volatile hurts performance of Charset.atBugLevel()

[Seán Coffey]

Ogata,

minor comments from me.

* The bug ID referenced in mail/webrev links is wrong. It should be 
8182743 ?
* The copyright change in Charset-X-Coder.java.template is the wrong 
format. You can simply replace 2013 with 2017.


Regards,
Sean.

On 29/06/17 19:49, Kazunori Ogata wrote:

Hi Christoph,

I updated webrev: http://cr.openjdk.java.net/~horii/8179527/webrev.02/

This one includes changes in tests.  I removed all @run and @build
directives in the tests because those after removing "@run main/othervm
-Dsun.nio.cs.bugLevel=1.4 EmptyCharsetName" are the same as the default
ones.  I checked the modified tests passed.

I also fixed the copyright lines.


Regards,
Ogata


"Langer, Christoph"  wrote on 2017/06/28
21:04:36:


From: "Langer, Christoph" 
To: Kazunori Ogata 
Cc: Alan Bateman , Claes Redestad
, core-libs-dev , "nio-...@openjdk.java.net" , "ppc-aix-port-...@openjdk.java.net"


d...@openjdk.java.net>
Date: 2017/06/28 21:04
Subject: RE: 8179527: Ineffective use of volatile hurts performance of
Charset.atBugLevel()

Hi Ogata,


remove the second run with -Dsun.nio.cs.bugLevel=1.4

How can I do this?  Is it sufficient to remove the following line at

the

beginning of the file?: "@run main/othervm -Dsun.nio.cs.bugLevel=1.4
EmptyCharsetName"

Yes, this line should be removed. Currently there are two @run

directives

which cause running the testcase twice. Once in normal mode and once

with

bugLevel set to 1.4. So, if "sun.nio.cs.bugLevel" ought to be removed

then

the second iteration of the test is obsolete. And then one should

probably

remove the whole "compat" handling in the test.

Best regards
Christoph

Re: Bug in File.getLastModified()

[Seán Coffey]

Hi Brian,

I think it's worth fixing unless there are objections. I see Stuart's 
comment about compatibility and wonder if we've any examples of such 
applications.


I just put together a patch [1] for this. I'm still figuring out how 
nanoseconds get recorded for macosx. stat64.st_mtimespec.tv_nsec seems 
to return 0 for me. We'd have to check the ppc/s390 ports also.


If successful, I might start an RFR for it. I think I found an issue 
with how Files.getLastModifiedTime handles a timestamp of Long.MAX_VALUE 
also. I might follow that up as a NIO issue. I don't think macosx 
returns millisecond resolution for the nio Files case either.


regards,
Sean.

[1]

diff --git a/src/solaris/native/java/io/UnixFileSystem_md.c 
b/src/solaris/native/java/io/UnixFileSystem_md.c

--- a/src/solaris/native/java/io/UnixFileSystem_md.c
+++ b/src/solaris/native/java/io/UnixFileSystem_md.c
@@ -208,7 +208,12 @@
 WITH_FIELD_PLATFORM_STRING(env, file, ids.path, path) {
 struct stat64 sb;
 if (stat64(path, ) == 0) {
-rv = 1000 * (jlong)sb.st_mtime;
+rv = (jlong)sb.st_mtime * 1000;
+#ifndef MACOSX
+rv += (jlong)sb.st_mtim.tv_nsec / 100;
+#else
+rv += (jlong)sb.st_mtimespec.tv_nsec / 100;
+#endif
 }
 } END_PLATFORM_STRING(env, path);
 return rv;
@@ -393,8 +398,11 @@

 /* Preserve access time */
 tv[0].tv_sec = sb.st_atime;
-tv[0].tv_usec = 0;
-
+#ifndef MACOSX
+tv[0].tv_usec = sb.st_atim.tv_nsec / 1000;
+#else
+tv[0].tv_usec = sb.st_atimespec.tv_nsec / 1000;
+#endif
 /* Change last-modified time */
 tv[1].tv_sec = time / 1000;
 tv[1].tv_usec = (time % 1000) * 1000;
diff --git a/test/java/io/File/GetLastModified.java 
b/test/java/io/File/GetLastModified.java

new file mode 100644
--- /dev/null
+++ b/test/java/io/File/GetLastModified.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2017, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License 
version

+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * @test
+ * @bug 8177809
+ * @summary File.lastModified() is losing milliseconds (always ends in 000)
+ */
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+
+public class GetLastModified {
+private static final long[] LM = { 1490606336718L,
+100L, 30L /*, Long.MAX_VALUE */ };
+
+public static void main(String[] args) throws IOException {
+boolean fail = false;
+File f = new File("GetLastModified.txt");
+f.createNewFile();
+
+for (int i = 0; i < LM.length; i++) {
+f.setLastModified(LM[i]);
+System.out.println("==");
+System.out.printf("Test f.lastModified [%s]: %b\n",
+f.lastModified(), f.lastModified() == LM[i]);
+long filesMillis = 
Files.getLastModifiedTime(f.toPath()).toMillis();

+System.out.printf("Test Files.getLastModifiedTime [%s]: %b\n",
+filesMillis, filesMillis == LM[i]);
+if (f.lastModified() != LM[i] || filesMillis != LM[i]) {
+fail = true;
+}
+}
+f.delete();
+if (fail) {
+throw new RuntimeException("Unexpected time stamps");
+}
+}
+}

On 31/03/17 16:03, Brian Burkhalter wrote:

Hi Ricardo,

Thanks for reading the specification verbiage closely. I think you 
have a point. I’d like to read what others think about this.


Regards,

Brian

On Mar 31, 2017, at 1:35 AM, Ricardo Almeida > wrote:



Just to add another though...

I was just double-reading the documentation and it says:

"All platforms support file-modification times to the nearest second,
but some provide more precision. The argument will be truncated to fit
the supported precision."

So, if the platform supports it, the argument is NOT truncated... 
Next we have:


"the next invocation of the lastModified() method will return the
(possibly 

Re: Bug in File.getLastModified()

[Seán Coffey]

Ricardo,

I see that JDK-8177809[1] has been logged. The newer NIO APIs can report 
back with higher time precision and it's probably best to use those. 
JDK-6939260 is related to your issue also.


Returning a ms value not equal to the ms value used in the 
setLastModified call does seem strange.


regards,
Sean.

[1] https://bugs.openjdk.java.net/browse/JDK-8177809

On 30/03/2017 02:15, David Holmes wrote:

Hi Ricardo,

This isn't a build issue. Redirecting to core-libs-dev. Please don't 
reply to build-dev.


Thanks,
David

On 30/03/2017 12:40 AM, Ricardo Almeida wrote:

Hi,

I could not raise a bug in https://bugs.openjdk.java.net/ so I hope it
is ok to say it here, hoping someone can take a look at it...

File.getLastModified() always returns with second precision, losing
the miliseconds (i.e. a number ending in 000). I tried using
Files.getLastModifiedTime and it seems to work correctly as the output
of the following test app shows:

Test f.lastModified [1490606336000]: false
Test Files.getLastModifiedTime [1490606336718]: true

The code:

package com.espatial.test;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;

public class FileTest {
private static final long LM = 1490606336718L;

public static void main(String[] args) throws IOException {
File f = new File("test.txt");
f.createNewFile();

f.setLastModified(LM);

System.out.printf("Test f.lastModified [%s]: %b\n",
f.lastModified(), f.lastModified() == LM);
System.out.printf("Test Files.getLastModifiedTime [%s]: %b\n",
Files.getLastModifiedTime(f.toPath()).toMillis(),
(Files.getLastModifiedTime(f.toPath()).toMillis() == LM));

f.delete();
}
}

Thanks you,
Regards,
Ricardo Almeida

Re: [8u-dev] (XS) RFR 8169056: StringIndexOutOfBoundsException in Pattern.compile with CANON_EQ flag

[Seán Coffey]

Looks good.

regards,
Sean.


On 23/03/2017 00:26, Ivan Gerasimov wrote:

Hello!

This is an 8u-dev only fix.
(In 9 the bug was fixed with a massive update [1]).

The fix is essentially one-liner: just check that the index remains 
within the bounds after update.

Regression test was updated to cover the case.

Would you please help review at your convenience?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8169056
WEBREV: http://cr.openjdk.java.net/~igerasim/8169056/00/webrev/

[1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/d0c319c32334

Re: RFR: JDK-8173094, 8173751, 8173752, 8173755, 8173802

[Seán Coffey]

Updated the subject line. Looks fine.

Minor nit with FilterOutputStream.java

  * write method of its underlying output stream with 
the single

  * argument b.

You're gone over 80 chars in length here. You can bring a word or two 
down to the last line. Do you want me to push this change for you?


Regards,
Sean.

On 10/02/17 19:18, Abhijit Roy wrote:


Hi Sean,

As per your suggestion, I have corrected those points. Please find the 
link below for further review.


http://cr.openjdk.java.net/~rpatil/ababroy/8173094/webrev.01/ 
<http://cr.openjdk.java.net/%7Erpatil/ababroy/8173094/webrev.01/>


Thanks,
Abhijit



On 2/10/2017 10:37 PM, Seán Coffey wrote:

Hi Abhijit,

you'll need to correct the Copyright year format in your edits. It 
needs to be "Copyright (c) , , Oracle..." format.


For the ZipFile change, you need to use lower case 's' in @since.

Looks good otherwise.

regards,
Sean.

On 10/02/17 10:51, Abhijit Roy wrote:

Hi all,

Please review the java doc fix for the below Bug:

Bug: https://bugs.openjdk.java.net/browse/JDK-8173094

Description: Error in API documentation for SwingWorker

Webrev-http://cr.openjdk.java.net/~rpatil/ababroy/8173094/webrev.00/


I have addressed some other doc issues which I included into the 
same webrev, please find those links below.



Bug: https://bugs.openjdk.java.net/browse/JDK-8173751

Description: Syntax error in ZipFile.getComment() method


Bug: https://bugs.openjdk.java.net/browse/JDK-8173752

Description: Syntax error in ZipEntry.setCompressedSize(long) method 
documentation



Bug: https://bugs.openjdk.java.net/browse/JDK-8173755

Description: The word input should be replaced by output in 
FilterOutputStream.write(byte[],int,int) method documentation



Bug: https://bugs.openjdk.java.net/browse/JDK-8173802

Description: Incorrect argument name in 
java.io.FilterInputStream.read(byte[]) method documentation




Regards,

Abhijit