Re: RFR(XXS): 8205916: [test] Fix jdk/tools/launcher/RunpathTest to handle both, RPATH and RUNPATH

[Volker Simonis]

Hi Martin, Erik,

thanks for the reviews!

Regards,
Volker


On Wed, Jun 27, 2018 at 5:53 PM, Erik Joelsson  wrote:
> Looks ok to me.
>
> /Erik
>
>
>
> On 2018-06-27 03:26, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have a review for the following tiny test fix (I'm
>> actually not sure which would be the appropriate mailing list for this
>> fix so please redirect if necessary):
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8205916/
>> https://bugs.openjdk.java.net/browse/JDK-8205916
>>
>> The test currently only checks for RPATH in the dynamic section of the
>> launcher, but some linkers / Linux distributions (notably SLES) use
>> RUNPATH instead.
>>
>> Following are the gory details:
>>
>> The test jdk/tools/launcher/RunpathTest.java checks that the java
>> launcher on Linux and Solaris has the correct RPATH path baked into
>> the executable.
>>
>> Unfortunately, the situation with RPATH is a little weird:
>>
>>- in order to bake a runtime path into a dynamically linked library
>> or executable one has to use the "-rpath " linker option (from
>> the C/C++ compiler this is usually available as "-Wl,-rpath,").
>>- depending on the dynamic linker version and Linux distribution,
>> the "-rpath" linker option adds either a "RPATH" entry (Ubuntu 16.04,
>> RHEL 7.4) or a "RUNPATH" entry (SLES 12.1, SLES 15) or both entries
>> together (SLES 11.3) to the dynamic section of the shared
>> library/executable.
>>- the semantics of "RPATH" and "RUNPATH" are slightly different:
>> "RPATH" is evaluated at runtime before LD_LIBRARY_PATH (if "RUNPATH"
>> isn't present) while "RUNPATH" is evaluated after LD_LIBRARY_PATH
>> (i.e. RUNPATH can be overridden at runtime by setting
>> LD_LIBRARY_PATH).
>>- "RPATH" is considered obsolete and should be replaced by "RUNPATH"
>> according to the man-page of "ld.so (8)".
>>- the linker option "--enable-new-dtags"/"--disable-new-dtags" (or
>> the corresponding compiler flags
>> "-Wl,--enable-new-dtags"/"-Wl,--disable-new-dtags") can be used to
>> enforce the generation of "RUNPATH"/"RPATH" respectively (except for
>> systems like SLES 11.3 where "--enable-new-dtags" generates both
>> "RPATH" and "RUNPATH" while "--disable-new-dtags" only generates
>> "RPATH").
>>
>> But this issue is not about fixing the build so to cut a long story
>> short - the test RunpathTest.java should be able to handle both
>> runtime path variants equally well. This can be easily achieved by
>> extending the match pattern from ".*RPATH.*\\$ORIGIN/../lib.*" to
>> ".*R(UN)?PATH.*\\$ORIGIN/../lib.*"
>>
>> Thank you and best regards,
>> Volker
>
>

Re: Draft JEP proposal: JDK-8200758: Packaging Tool

[Remi Forax]

Hi Kevin,
Just a small request,
can you call it something like jbundler and not jpackager, because usually in 
build tools the packager step is the step that create the jars, not the one 
that bundle the whole application ?

regards,
Rémi

- Mail original -
> De: "Kevin Rushforth" 
> À: "core-libs-dev" 
> Envoyé: Jeudi 31 Mai 2018 02:10:48
> Objet: Draft JEP proposal: JDK-8200758: Packaging Tool

> I would like to propose the following Draft JEP [1] for discussion.
> 
> JDK-8200758: Packaging Tool
> 
> This is intended as a JDK-scope replacement for the existing
> javapackager tool that ships with Oracle JDK 10 (and earlier Oracle JDK
> releases), and was delivered as part of the JavaFX build. The
> javapackager tool has been removed from Oracle JDK 11 along with the
> removal of JavaFX.
> 
> Comments on this JEP are welcome. It is currently not targeted for a
> specific release, but we are aiming for JDK 12.
> 
> -- Kevin
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8200758

Re: [PATCH] AbstractStringBuilder.append()

[Martin Buchholz]

I'm fairly sure the append(StringBuilder) overloads were left out
intentionally.

On Wed, Jun 27, 2018 at 8:57 PM, Isaac Levy  wrote:

> AbstractStringBuilder already has append(). This patch
> adds append().
>
> The new method improves parity between the two classes. In addition,
> combining StringBuilders is presumably common. append()
> has a couple insteadof checks, which this new method skips.
>
> -Isaac
>
>
>
>
> diff --git a/src/java.base/share/classes/java/lang/
> AbstractStringBuilder.java
> b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> index 2ef3e53256..1fe89bb92a 100644
> --- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> +++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
> @@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
> Appendable, CharSequence {
>  return this.append((AbstractStringBuilder)sb);
>  }
>
> +// Documentation in subclasses because of synchro difference
> +public AbstractStringBuilder append(StringBuilder sb) {
> +return this.append((AbstractStringBuilder)sb);
> +}
> +
>  /**
>   * @since 1.8
>   */
> diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
> b/src/java.base/share/classes/java/lang/StringBuffer.java
> index e597a8112e..613ba90c25 100644
> --- a/src/java.base/share/classes/java/lang/StringBuffer.java
> +++ b/src/java.base/share/classes/java/lang/StringBuffer.java
> @@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
>  return this;
>  }
>
> +/**
> + * Appends the specified {@code StringBuilder} to this sequence.
> + * 
> + * The characters of the {@code StringBuilder} argument are appended,
> + * in order, to the contents of this {@code StringBuffer}, increasing
> the
> + * length of this {@code StringBuffer} by the length of the argument.
> + * If {@code sb} is {@code null}, then the four characters
> + * {@code "null"} are appended to this {@code StringBuffer}.
> + * 
> + * Let n be the length of the old character sequence, the one
> + * contained in the {@code StringBuffer} just prior to execution of
> the
> + * {@code append} method. Then the character at index k in
> + * the new character sequence is equal to the character at index
> k
> + * in the old character sequence, if k is less than n;
> + * otherwise, it is equal to the character at index k-n in the
> + * argument {@code sb}.
> + * 
> + *
> + * @param   sb   the {@code StringBuilder} to append.
> + * @return  a reference to this object.
> + */
> +public synchronized StringBuffer append(StringBuilder sb) {
> +toStringCache = null;
> +super.append(sb);
> +return this;
> +}
> +
>  /**
>   * Appends the specified {@code StringBuffer} to this sequence.
>   * 
> diff --git a/src/java.base/share/classes/java/lang/StringBuilder.java
> b/src/java.base/share/classes/java/lang/StringBuilder.java
> index 40da2083c2..5ddd4fb5f9 100644
> --- a/src/java.base/share/classes/java/lang/StringBuilder.java
> +++ b/src/java.base/share/classes/java/lang/StringBuilder.java
> @@ -199,6 +199,30 @@ public final class StringBuilder
>  return this;
>  }
>
> +/**
> + * Appends the specified {@code StringBuilder} to this sequence.
> + * 
> + * The characters of the {@code StringBuilder} argument are appended,
> + * in order, to this sequence, increasing the
> + * length of this sequence by the length of the argument.
> + * If {@code sb} is {@code null}, then the four characters
> + * {@code "null"} are appended to this sequence.
> + * 
> + * Let n be the length of this character sequence just prior to
> + * execution of the {@code append} method. Then the character at index
> + * k in the new character sequence is equal to the character at
> + * index k in the old character sequence, if k is less
> than
> + * n; otherwise, it is equal to the character at index
> k-n
> + * in the argument {@code sb}.
> + *
> + * @param   sb   the {@code StringBuilder} to append.
> + * @return  a reference to this object.
> + */
> +public StringBuilder append(StringBuilder sb) {
> +super.append(sb);
> +return this;
> +}
> +
>  @Override
>  public StringBuilder append(CharSequence s) {
>  super.append(s);
>

[PATCH] AbstractStringBuilder.append()

[Isaac Levy]

AbstractStringBuilder already has append(). This patch
adds append().

The new method improves parity between the two classes. In addition,
combining StringBuilders is presumably common. append()
has a couple insteadof checks, which this new method skips.

-Isaac




diff --git a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
index 2ef3e53256..1fe89bb92a 100644
--- a/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
+++ b/src/java.base/share/classes/java/lang/AbstractStringBuilder.java
@@ -543,6 +543,11 @@ abstract class AbstractStringBuilder implements
Appendable, CharSequence {
 return this.append((AbstractStringBuilder)sb);
 }

+// Documentation in subclasses because of synchro difference
+public AbstractStringBuilder append(StringBuilder sb) {
+return this.append((AbstractStringBuilder)sb);
+}
+
 /**
  * @since 1.8
  */
diff --git a/src/java.base/share/classes/java/lang/StringBuffer.java
b/src/java.base/share/classes/java/lang/StringBuffer.java
index e597a8112e..613ba90c25 100644
--- a/src/java.base/share/classes/java/lang/StringBuffer.java
+++ b/src/java.base/share/classes/java/lang/StringBuffer.java
@@ -313,6 +313,33 @@ import jdk.internal.HotSpotIntrinsicCandidate;
 return this;
 }

+/**
+ * Appends the specified {@code StringBuilder} to this sequence.
+ * 
+ * The characters of the {@code StringBuilder} argument are appended,
+ * in order, to the contents of this {@code StringBuffer}, increasing the
+ * length of this {@code StringBuffer} by the length of the argument.
+ * If {@code sb} is {@code null}, then the four characters
+ * {@code "null"} are appended to this {@code StringBuffer}.
+ * 
+ * Let n be the length of the old character sequence, the one
+ * contained in the {@code StringBuffer} just prior to execution of the
+ * {@code append} method. Then the character at index k in
+ * the new character sequence is equal to the character at index k
+ * in the old character sequence, if k is less than n;
+ * otherwise, it is equal to the character at index k-n in the
+ * argument {@code sb}.
+ * 
+ *
+ * @param   sb   the {@code StringBuilder} to append.
+ * @return  a reference to this object.
+ */
+public synchronized StringBuffer append(StringBuilder sb) {
+toStringCache = null;
+super.append(sb);
+return this;
+}
+
 /**
  * Appends the specified {@code StringBuffer} to this sequence.
  * 
diff --git a/src/java.base/share/classes/java/lang/StringBuilder.java
b/src/java.base/share/classes/java/lang/StringBuilder.java
index 40da2083c2..5ddd4fb5f9 100644
--- a/src/java.base/share/classes/java/lang/StringBuilder.java
+++ b/src/java.base/share/classes/java/lang/StringBuilder.java
@@ -199,6 +199,30 @@ public final class StringBuilder
 return this;
 }

+/**
+ * Appends the specified {@code StringBuilder} to this sequence.
+ * 
+ * The characters of the {@code StringBuilder} argument are appended,
+ * in order, to this sequence, increasing the
+ * length of this sequence by the length of the argument.
+ * If {@code sb} is {@code null}, then the four characters
+ * {@code "null"} are appended to this sequence.
+ * 
+ * Let n be the length of this character sequence just prior to
+ * execution of the {@code append} method. Then the character at index
+ * k in the new character sequence is equal to the character at
+ * index k in the old character sequence, if k is less than
+ * n; otherwise, it is equal to the character at index k-n
+ * in the argument {@code sb}.
+ *
+ * @param   sb   the {@code StringBuilder} to append.
+ * @return  a reference to this object.
+ */
+public StringBuilder append(StringBuilder sb) {
+super.append(sb);
+return this;
+}
+
 @Override
 public StringBuilder append(CharSequence s) {
 super.append(s);

[JDK 11] RFR 8187069: The case auto failed with the "java.lang.ClassNotFoundException: IPv6NameserverPlatformParsingTest" exception

[Chris Yin]

Please review below one line change for manual test case 
com/sun/jndi/dns/Test6991580.java to build test class automatically which will 
be used in manual steps, thanks

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


Review change as below:

diff -r 2d3e99a72541 test/jdk/com/sun/jndi/dns/Test6991580.java
--- a/test/jdk/com/sun/jndi/dns/Test6991580.javaWed Jun 27 17:02:41 
2018 -0700
+++ b/test/jdk/com/sun/jndi/dns/Test6991580.javaThu Jun 28 09:50:37 
2018 +0800
@@ -37,6 +37,7 @@
  * @summary IPv6 Nameservers in resolv.conf throws NumberFormatException
  * @modules java.desktop
  *  jdk.naming.dns/com.sun.jndi.dns
+ * @build IPv6NameserverPlatformParsingTest
  * @run main/manual Test6991580
  */

Regards,
Chris

Re: Draft JEP proposal: JDK-8200758: Packaging Tool

[Kevin Rushforth]

We're aiming to get this into JDK 12 early enough so that an EA build 
would be available around the time JDK 11 ships. That will allow you to 
take a jlinked image with JDK 11 and package it up using (the EA) jpackager.


We will create a development branch in the JDK sandbox [1] some time in 
the next week or so so you can follow the development.


Also, thank you to those who have provided feedback. I'll reply to 
feedback soon and then incorporate it into an updated JEP.


-- Kevin


On 6/27/2018 3:09 AM, Buchberger, Joerg wrote:

Thanks for the info! And thanks for the efforts.  [no irony, no sarcasm - I 
really mean it]

But, to sum up my comprehension...

anyone who placed their bets on javapackager, starting with last LTS Java 8
will be left in the rain with followup LTS Java 11, because their ain't neither 
javapackager (anymore), nor jpackager (yet).

Is this correct?

So, strategic choice boils down to either throw away all work done based on 
javapackager so far and the associated distribution concepts (reworking 
everything from scratch)
or neglect Java 11 completely, thus placing all bets on jpackager really coming 
w/ Java 12 or even waiting for Java 14 as next LTS thereafter.

Bam(!), I think, I first need a tiny shot now ;-) and let that info sink in ...

Cheers
Jörg


On 5/31/2018 0:10 AM, Rushforth, Kevin wrote:

I would like to propose the following Draft JEP [1] for discussion.

JDK-8200758: Packaging Tool

This is intended as a JDK-scope replacement for the existing
javapackager tool that ships with Oracle JDK 10 (and earlier Oracle JDK
releases), and was delivered as part of the JavaFX build. The
javapackager tool has been removed from Oracle JDK 11 along with the
removal of JavaFX.

Comments on this JEP are welcome. It is currently not targeted for a
specific release, but we are aiming for JDK 12.

-- Kevin

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

Re: RFR: 8205438: Re-enable shebang tests in test/jdk/tools/launchers/SourceMode.java

[James Laskey]

We went through the same exercise with jjs. Tough coming up with shebang tests 
that worked on all platforms. Shebang args vs command line args was a 
nightmare. 

Sent from my iPhone

> On Jun 27, 2018, at 6:02 PM, mandy chung  wrote:
> 
> Looks good.  It's amazing to find out 3 different behavior on these 3 
> platforms.
> 
> Mandy
> 
>> On 6/27/18 1:37 PM, Jonathan Gibbons wrote:
>> Please review a test fix to re-enable shebang tests in 
>> test/jdk/tools/launchers/SourceMode.java .
>> The test cases for invoking the source launcher via the shebang mechanism 
>> have been disabled because they were adversely affected by the very long 
>> pathnames on our internal test infastructure, causing the test cases to 
>> overflow the system limit of 128 characters for the first line of a shebang 
>> file
>> The fix is to run jlink to create a small image that can be addressed by a 
>> short relative path.
>> Some of the test cases on macOS and Solaris remain explicitly disabled 
>> because of the inconsistencies in the system-level support for shebang 
>> execution.
>> Finally, the use of hard-coded version numbers in the test is changed to 
>> avoid having to add this test to the list of tests that need to be modified 
>> at the beginning of each release.
>> -- Jon
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8205438
>> Webrev: http://cr.openjdk.java.net/~jjg/8205438/webrev.00/
> 
> 

Re: RFR: 8205438: Re-enable shebang tests in test/jdk/tools/launchers/SourceMode.java

[mandy chung]

Looks good.  It's amazing to find out 3 different behavior on these 3 
platforms.


Mandy

On 6/27/18 1:37 PM, Jonathan Gibbons wrote:
Please review a test fix to re-enable shebang tests in 
test/jdk/tools/launchers/SourceMode.java .


The test cases for invoking the source launcher via the shebang 
mechanism have been disabled because they were adversely affected by the 
very long pathnames on our internal test infastructure, causing the test 
cases to overflow the system limit of 128 characters for the first line 
of a shebang file


The fix is to run jlink to create a small image that can be addressed by 
a short relative path.


Some of the test cases on macOS and Solaris remain explicitly disabled 
because of the inconsistencies in the system-level support for shebang 
execution.


Finally, the use of hard-coded version numbers in the test is changed to 
avoid having to add this test to the list of tests that need to be 
modified at the beginning of each release.


-- Jon

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

RFR: 8205438: Re-enable shebang tests in test/jdk/tools/launchers/SourceMode.java

[Jonathan Gibbons]

Please review a test fix to re-enable shebang tests in 
test/jdk/tools/launchers/SourceMode.java .


The test cases for invoking the source launcher via the shebang 
mechanism have been disabled because they were adversely affected by the 
very long pathnames on our internal test infastructure, causing the test 
cases to overflow the system limit of 128 characters for the first line 
of a shebang file


The fix is to run jlink to create a small image that can be addressed by 
a short relative path.


Some of the test cases on macOS and Solaris remain explicitly disabled 
because of the inconsistencies in the system-level support for shebang 
execution.


Finally, the use of hard-coded version numbers in the test is changed to 
avoid having to add this test to the list of tests that need to be 
modified at the beginning of each release.


-- Jon

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

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

[Xuelei Fan]

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


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


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

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

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

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

No more comment.

Thanks,
Xuelei

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

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

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

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


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


Thanks,
Xuelei

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

Erik,

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


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


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

regards,
Sean.


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

Many thanks for the review comments Erik. Replies inline.


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

Hi Sean,

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


Some initial comments:

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

Makes sense. I'll remove that parameter.


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


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

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

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

This following annotation adds little value

@Description("Details of Security Property")

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


@Category("Java Application")

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

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


@Label("X509Cert")

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


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

I think it would be good to separate testing of JFR 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

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

[Erik Gahlin]

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 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 s

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

[Xuelei Fan]

KeyUtil.java

167  public static String getKeyAlgorithm(Key key) {

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


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



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

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

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

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

Got it.



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


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


Thanks,
Xuelei


regards,
Sean.



Thanks,
Xuelei

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

Erik,

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


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


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

regards,
Sean.


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

Many thanks for the review comments Erik. Replies inline.


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

Hi Sean,

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


Some initial comments:

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

Makes sense. I'll remove that parameter.


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


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

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

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

This following annotation adds little value

@Description("Details of Security Property")

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


@Category("Java Application")

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

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


@Label("X509Cert")

The label should be human readable name, with spaces, title cased 
etc. I would recommend "X.509 Certificate". In general, avoid 
abbreviations like "certs" 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

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.


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 j

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

[Xuelei Fan]

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

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

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


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


Thanks,
Xuelei

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

Erik,

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


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


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

regards,
Sean.


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

Many thanks for the review comments Erik. Replies inline.


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

Hi Sean,

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


Some initial comments:

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

Makes sense. I'll remove that parameter.


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


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

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

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

This following annotation adds little value

@Description("Details of Security Property")

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


@Category("Java Application")

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

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


@Label("X509Cert")

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


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

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


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


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


regards,
Sean.


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

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

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

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


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

Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file

[Joe Wang]

Thanks all!  Glad to be able to get this done right before the deadline :-)

-Joe

On 6/27/18, 12:44 AM, Alan Bateman wrote:

On 26/06/2018 20:49, Joe Wang wrote:

:

Removed the null check. The internal impl and test guarantees it's 
not null indeed:

http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/

Looks good.

-Alan

Re: RFR(XXS): 8205916: [test] Fix jdk/tools/launcher/RunpathTest to handle both, RPATH and RUNPATH

[Erik Joelsson]

Looks ok to me.

/Erik


On 2018-06-27 03:26, Volker Simonis wrote:

Hi,

can I please have a review for the following tiny test fix (I'm
actually not sure which would be the appropriate mailing list for this
fix so please redirect if necessary):

http://cr.openjdk.java.net/~simonis/webrevs/2018/8205916/
https://bugs.openjdk.java.net/browse/JDK-8205916

The test currently only checks for RPATH in the dynamic section of the
launcher, but some linkers / Linux distributions (notably SLES) use
RUNPATH instead.

Following are the gory details:

The test jdk/tools/launcher/RunpathTest.java checks that the java
launcher on Linux and Solaris has the correct RPATH path baked into
the executable.

Unfortunately, the situation with RPATH is a little weird:

   - in order to bake a runtime path into a dynamically linked library
or executable one has to use the "-rpath " linker option (from
the C/C++ compiler this is usually available as "-Wl,-rpath,").
   - depending on the dynamic linker version and Linux distribution,
the "-rpath" linker option adds either a "RPATH" entry (Ubuntu 16.04,
RHEL 7.4) or a "RUNPATH" entry (SLES 12.1, SLES 15) or both entries
together (SLES 11.3) to the dynamic section of the shared
library/executable.
   - the semantics of "RPATH" and "RUNPATH" are slightly different:
"RPATH" is evaluated at runtime before LD_LIBRARY_PATH (if "RUNPATH"
isn't present) while "RUNPATH" is evaluated after LD_LIBRARY_PATH
(i.e. RUNPATH can be overridden at runtime by setting
LD_LIBRARY_PATH).
   - "RPATH" is considered obsolete and should be replaced by "RUNPATH"
according to the man-page of "ld.so (8)".
   - the linker option "--enable-new-dtags"/"--disable-new-dtags" (or
the corresponding compiler flags
"-Wl,--enable-new-dtags"/"-Wl,--disable-new-dtags") can be used to
enforce the generation of "RUNPATH"/"RPATH" respectively (except for
systems like SLES 11.3 where "--enable-new-dtags" generates both
"RPATH" and "RUNPATH" while "--disable-new-dtags" only generates
"RPATH").

But this issue is not about fixing the build so to cut a long story
short - the test RunpathTest.java should be able to handle both
runtime path variants equally well. This can be easily achieved by
extending the match pattern from ".*RPATH.*\\$ORIGIN/../lib.*" to
".*R(UN)?PATH.*\\$ORIGIN/../lib.*"

Thank you and best regards,
Volker

Re: RFR JDK-8200243: System error message is decoded as invalid encoding in Windows.

[Xueming Shen]

On 6/27/18, 12:39 AM, Alan Bateman wrote:

On 27/06/2018 04:14, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8200243

issue: https://bugs.openjdk.java.net/browse/JDK-8200243
webrev: http://cr.openjdk.java.net/~sherman/8200243/webrev

This is a regression. The root cause is one of the change we put in 
jdk9 for

JDK-805 [1], which is to remove those unused vm interfaces.

The webrev [1] indicates that the changeset added a platform 
dependent version of
GetLastErrorString(),  which uses utf8, to replace the original jvm 
version of

JVM_GetLastErrorString() (in jni_util.c).

However the encoding of the char* interface between jvm and jdk is 
always
interpreted/specified as "native encoding". So if the platform's 
default encoding
is NOT utf-8, it never uses  utf8. This is what we have on Windows 
platform. The
fix is to use/copy-paste the "original" vm implementation in hotspot 
repo for

JVM_GetLastErrorString() (from os_windows.cpp).
This looks okay, just not immediately clear if this links to 
FormatMessageA or FormatMessageW.


Without an explicit _UNICODE and UNICODE switch it should always be "A" 
version. If we have more

time maybe I should go through the circle to simply use the "A" explicitly.

-Sherman

Re: RFR(XXS): 8205916: [test] Fix jdk/tools/launcher/RunpathTest to handle both, RPATH and RUNPATH

[Martin Buchholz]

Looks good to me!

On Wed, Jun 27, 2018 at 3:26 AM, Volker Simonis 
wrote:

> Hi,
>
> can I please have a review for the following tiny test fix (I'm
> actually not sure which would be the appropriate mailing list for this
> fix so please redirect if necessary):
>
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8205916/
> https://bugs.openjdk.java.net/browse/JDK-8205916
>
> The test currently only checks for RPATH in the dynamic section of the
> launcher, but some linkers / Linux distributions (notably SLES) use
> RUNPATH instead.
>
> Following are the gory details:
>
> The test jdk/tools/launcher/RunpathTest.java checks that the java
> launcher on Linux and Solaris has the correct RPATH path baked into
> the executable.
>
> Unfortunately, the situation with RPATH is a little weird:
>
>   - in order to bake a runtime path into a dynamically linked library
> or executable one has to use the "-rpath " linker option (from
> the C/C++ compiler this is usually available as "-Wl,-rpath,").
>   - depending on the dynamic linker version and Linux distribution,
> the "-rpath" linker option adds either a "RPATH" entry (Ubuntu 16.04,
> RHEL 7.4) or a "RUNPATH" entry (SLES 12.1, SLES 15) or both entries
> together (SLES 11.3) to the dynamic section of the shared
> library/executable.
>   - the semantics of "RPATH" and "RUNPATH" are slightly different:
> "RPATH" is evaluated at runtime before LD_LIBRARY_PATH (if "RUNPATH"
> isn't present) while "RUNPATH" is evaluated after LD_LIBRARY_PATH
> (i.e. RUNPATH can be overridden at runtime by setting
> LD_LIBRARY_PATH).
>   - "RPATH" is considered obsolete and should be replaced by "RUNPATH"
> according to the man-page of "ld.so (8)".
>   - the linker option "--enable-new-dtags"/"--disable-new-dtags" (or
> the corresponding compiler flags
> "-Wl,--enable-new-dtags"/"-Wl,--disable-new-dtags") can be used to
> enforce the generation of "RUNPATH"/"RPATH" respectively (except for
> systems like SLES 11.3 where "--enable-new-dtags" generates both
> "RPATH" and "RUNPATH" while "--disable-new-dtags" only generates
> "RPATH").
>
> But this issue is not about fixing the build so to cut a long story
> short - the test RunpathTest.java should be able to handle both
> runtime path variants equally well. This can be easily achieved by
> extending the match pattern from ".*RPATH.*\\$ORIGIN/../lib.*" to
> ".*R(UN)?PATH.*\\$ORIGIN/../lib.*"
>
> Thank you and best regards,
> Volker
>

Re: RFR JDK-8066709 Make some JDK system properties read only

[Roger Riggs]

Hi Sean,

Updated webrev:
http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/index.html

I added the suggested text and updated the class level warning to match.

Thanks, Roger


On 6/27/2018 6:58 AM, Sean Mullan wrote:
I think it is worth putting a stronger warning in each of the methods 
(and not just the class description) of StaticProperty that additional 
care should be taken when using these methods since there is no 
SecurityManager check. For example:



"{@link SecurityManager#checkPropertyAccess} is NOT checked
in this method. The caller of this method should take care to ensure 
that the returned property is not made accessible to untrusted code."


--Sean

On 6/26/18 10:10 PM, Roger Riggs wrote:

Hi,

Updated webrev:

http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/index.html 



Applied changes from prior comments and droped a change no longer 
needed due

to the TLS 1.3 removal of ClientKeyExchangeService.java.

The CSR has been approved without possibly confusing @implNote in 
System.getProperties

about caching of specific properties, including java.home, etc.

Thanks for any additional comments.

Roger



On 6/19/18 11:52 AM, Brent Christian wrote:

On 6/19/18 8:08 AM, Roger Riggs wrote:


* src/java.base/share/classes/java/lang/System.java :

Should the @implNote with the list of cached properties be added 
everywhere the @apiNote is being added ?  Right now the @implNote 
is only added to getProperties().


The repetition was getting tiresome and the base of all the 
xxxProperties methods is getProperties.
  Joe suggested having one copy of the full information  and 
referring to that from the individual @apiNotes.


Fair enough.


* src/java.base/share/classes/jdk/internal/util/StaticProperty.java :

  45 private StaticProperty() {
  46
  47 }

Maybe put this all on one line?


Will do


Thanks,
-Brent

Re: RFR JDK-8066709 Make some JDK system properties read only

[Roger Riggs]

I rechecked, almost all were within doPriv and the others have new 
explicit SM checks.


Thanks, Roger

On 6/27/2018 1:54 AM, mandy chung wrote:
Looks good to me.  The part that I'm unsure is whether you caught all 
the callers that are outside doPrivileged to StaticProperty.* will 
need to do its property permission check.  This requires API 
inspection and testing that I assume you covered that.


Mandy

On 6/26/18 7:10 PM, Roger Riggs wrote:

Hi,

Updated webrev:

http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/index.html 



Applied changes from prior comments and droped a change no longer 
needed due

to the TLS 1.3 removal of ClientKeyExchangeService.java.

The CSR has been approved without possibly confusing @implNote in 
System.getProperties

about caching of specific properties, including java.home, etc.

Thanks for any additional comments.

Roger



On 6/19/18 11:52 AM, Brent Christian wrote:

On 6/19/18 8:08 AM, Roger Riggs wrote:


* src/java.base/share/classes/java/lang/System.java :

Should the @implNote with the list of cached properties be added 
everywhere the @apiNote is being added ?  Right now the @implNote 
is only added to getProperties().


The repetition was getting tiresome and the base of all the 
xxxProperties methods is getProperties.
  Joe suggested having one copy of the full information  and 
referring to that from the individual @apiNotes.


Fair enough.


* src/java.base/share/classes/jdk/internal/util/StaticProperty.java :

  45 private StaticProperty() {
  46
  47 }

Maybe put this all on one line?


Will do


Thanks,
-Brent

Re: RFR JDK-8066709 Make some JDK system properties read only

[Sean Mullan]

I think it is worth putting a stronger warning in each of the methods 
(and not just the class description) of StaticProperty that additional 
care should be taken when using these methods since there is no 
SecurityManager check. For example:



"{@link SecurityManager#checkPropertyAccess} is NOT checked
in this method. The caller of this method should take care to ensure 
that the returned property is not made accessible to untrusted code."


--Sean

On 6/26/18 10:10 PM, Roger Riggs wrote:

Hi,

Updated webrev:

http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/index.html 



Applied changes from prior comments and droped a change no longer needed 
due

to the TLS 1.3 removal of ClientKeyExchangeService.java.

The CSR has been approved without possibly confusing @implNote in 
System.getProperties

about caching of specific properties, including java.home, etc.

Thanks for any additional comments.

Roger



On 6/19/18 11:52 AM, Brent Christian wrote:

On 6/19/18 8:08 AM, Roger Riggs wrote:


* src/java.base/share/classes/java/lang/System.java :

Should the @implNote with the list of cached properties be added 
everywhere the @apiNote is being added ?  Right now the @implNote is 
only added to getProperties().


The repetition was getting tiresome and the base of all the 
xxxProperties methods is getProperties.
  Joe suggested having one copy of the full information  and 
referring to that from the individual @apiNotes.


Fair enough.


* src/java.base/share/classes/jdk/internal/util/StaticProperty.java :

  45 private StaticProperty() {
  46
  47 }

Maybe put this all on one line?


Will do


Thanks,
-Brent

RFR(XXS): 8205916: [test] Fix jdk/tools/launcher/RunpathTest to handle both, RPATH and RUNPATH

[Volker Simonis]

Hi,

can I please have a review for the following tiny test fix (I'm
actually not sure which would be the appropriate mailing list for this
fix so please redirect if necessary):

http://cr.openjdk.java.net/~simonis/webrevs/2018/8205916/
https://bugs.openjdk.java.net/browse/JDK-8205916

The test currently only checks for RPATH in the dynamic section of the
launcher, but some linkers / Linux distributions (notably SLES) use
RUNPATH instead.

Following are the gory details:

The test jdk/tools/launcher/RunpathTest.java checks that the java
launcher on Linux and Solaris has the correct RPATH path baked into
the executable.

Unfortunately, the situation with RPATH is a little weird:

  - in order to bake a runtime path into a dynamically linked library
or executable one has to use the "-rpath " linker option (from
the C/C++ compiler this is usually available as "-Wl,-rpath,").
  - depending on the dynamic linker version and Linux distribution,
the "-rpath" linker option adds either a "RPATH" entry (Ubuntu 16.04,
RHEL 7.4) or a "RUNPATH" entry (SLES 12.1, SLES 15) or both entries
together (SLES 11.3) to the dynamic section of the shared
library/executable.
  - the semantics of "RPATH" and "RUNPATH" are slightly different:
"RPATH" is evaluated at runtime before LD_LIBRARY_PATH (if "RUNPATH"
isn't present) while "RUNPATH" is evaluated after LD_LIBRARY_PATH
(i.e. RUNPATH can be overridden at runtime by setting
LD_LIBRARY_PATH).
  - "RPATH" is considered obsolete and should be replaced by "RUNPATH"
according to the man-page of "ld.so (8)".
  - the linker option "--enable-new-dtags"/"--disable-new-dtags" (or
the corresponding compiler flags
"-Wl,--enable-new-dtags"/"-Wl,--disable-new-dtags") can be used to
enforce the generation of "RUNPATH"/"RPATH" respectively (except for
systems like SLES 11.3 where "--enable-new-dtags" generates both
"RPATH" and "RUNPATH" while "--disable-new-dtags" only generates
"RPATH").

But this issue is not about fixing the build so to cut a long story
short - the test RunpathTest.java should be able to handle both
runtime path variants equally well. This can be easily achieved by
extending the match pattern from ".*RPATH.*\\$ORIGIN/../lib.*" to
".*R(UN)?PATH.*\\$ORIGIN/../lib.*"

Thank you and best regards,
Volker

Re: Draft JEP proposal: JDK-8200758: Packaging Tool

[Buchberger, Joerg]

Thanks for the info! And thanks for the efforts.  [no irony, no sarcasm - I 
really mean it]

But, to sum up my comprehension...

anyone who placed their bets on javapackager, starting with last LTS Java 8
will be left in the rain with followup LTS Java 11, because their ain't neither 
javapackager (anymore), nor jpackager (yet).

Is this correct?

So, strategic choice boils down to either throw away all work done based on 
javapackager so far and the associated distribution concepts (reworking 
everything from scratch)
or neglect Java 11 completely, thus placing all bets on jpackager really coming 
w/ Java 12 or even waiting for Java 14 as next LTS thereafter.

Bam(!), I think, I first need a tiny shot now ;-) and let that info sink in ...

Cheers
Jörg


On 5/31/2018 0:10 AM, Rushforth, Kevin wrote:
> I would like to propose the following Draft JEP [1] for discussion.
>
> JDK-8200758: Packaging Tool
>
> This is intended as a JDK-scope replacement for the existing 
> javapackager tool that ships with Oracle JDK 10 (and earlier Oracle JDK 
> releases), and was delivered as part of the JavaFX build. The 
> javapackager tool has been removed from Oracle JDK 11 along with the 
> removal of JavaFX.
>
> Comments on this JEP are welcome. It is currently not targeted for a 
> specific release, but we are aiming for JDK 12.
>
> -- Kevin
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8200758
>

Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file

[Alan Bateman]

On 26/06/2018 20:49, Joe Wang wrote:

:

Removed the null check. The internal impl and test guarantees it's not 
null indeed:

http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/

Looks good.

-Alan

Re: RFR JDK-8200243: System error message is decoded as invalid encoding in Windows.

[Alan Bateman]

On 27/06/2018 04:14, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8200243

issue: https://bugs.openjdk.java.net/browse/JDK-8200243
webrev: http://cr.openjdk.java.net/~sherman/8200243/webrev

This is a regression. The root cause is one of the change we put in 
jdk9 for

JDK-805 [1], which is to remove those unused vm interfaces.

The webrev [1] indicates that the changeset added a platform dependent 
version of
GetLastErrorString(),  which uses utf8, to replace the original jvm 
version of

JVM_GetLastErrorString() (in jni_util.c).

However the encoding of the char* interface between jvm and jdk is always
interpreted/specified as "native encoding". So if the platform's 
default encoding
is NOT utf-8, it never uses  utf8. This is what we have on Windows 
platform. The
fix is to use/copy-paste the "original" vm implementation in hotspot 
repo for

JVM_GetLastErrorString() (from os_windows.cpp).
This looks okay, just not immediately clear if this links to 
FormatMessageA or FormatMessageW.


-Alan

Re: RFR JDK-8066709 Make some JDK system properties read only

[Alan Bateman]

On 27/06/2018 03:10, Roger Riggs wrote:

Hi,

Updated webrev:

http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/index.html 



Applied changes from prior comments and droped a change no longer 
needed due

to the TLS 1.3 removal of ClientKeyExchangeService.java.

The CSR has been approved without possibly confusing @implNote in 
System.getProperties

about caching of specific properties, including java.home, etc.
This looks okay and is a good first step to eliminating the issues 
caused by code changing these properties in a running VM.


-Alan