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

[Weijun Wang]

> On May 27, 2020, at 1:46 AM, Alexey Bakhtin  wrote:
> 
> Hello Max,
> 
> Thank you review.
> If I understand correctly tls-server-end-point channel binding data is a hash 
> of server certificate. Different SASL protocols could send cbData 
> differently, with different prefix format.

Not sure if I understand, what do "Different SASL protocols" mean? Here 
LdapClient is the application and therefore the "protocol". Are you worried 
that another SASL mechanism might choose a different prefix?

I really find the TLS word in a SASL mechanism strange. It belongs to a 
different layer.

--Max

> This is a reason I create TLSChannelBinding class and calculate hash from the 
> LdapClient and add “tls-server-end-point:” prefix in the JGSS/Kerberos.



> 
> Alexey
> 
>> On 26 May 2020, at 17:50, Weijun Wang  wrote:
>> 
>> I have a question on GssKrb5Client.java:
>> 
>> Do you think it's a good idea to let the SASL mechanism understand what TLS 
>> binding is? Instead of passing the whole TlsChannelBinding object through a 
>> SASL property, is it possible to only pass the actual cbData? After all, the 
>> name "com.sun.security.sasl.channelbinding" suggests it's just a general 
>> ChannelBinding which is independent with any application level info.
>> 
>> From my reading of the code change, it looks like this cbData can be 
>> calculated on the LDAP side.
>> 
>> Thanks,
>> Max
>> 
>>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
>>> 
>>> Hello,
>>> 
>>> Could you please review the following patch:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
>>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>>> 
>>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
>>> tls-server-end-point channel binding
>>> (based on the TLS server certificate). The channel binding data is 
>>> calculated as following :
>>> • The client calculates a hash of the TLS server certificate.
>>> The hash algorithm is selected on the base of the certificate 
>>> signature algorithm.
>>> Also, the client should use SHA-256 algorithm, in case of the 
>>> certificate signature algorithm is SHA1 or MD5 based
>>> • The channel binding information is the same as defined in rfc4121 [1] 
>>> with small corrections:
>>> • initiator and acceptor addresses should be set to NULL
>>> • initiator and acceptor address types should be zero.
>>>It contradicts to the “Using Channel Bindings in GSS-API” 
>>> document [2] that say that
>>>the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>>>instead of GSS_C_AF_UNSPEC=0x00.
>>> 
>>> This patch introduces changes in the LDAP, SASL and JGSS modules
>>> to generate channel binding data automatically if requested by an 
>>> application.
>>> This patch reuses existing org.ietf.jgss.ChannelBinding class 
>>> implementation but changes
>>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
>>> CHANNEL_BINDING_AF_UNSPEC.
>>> The patch introduces new environment property 
>>> “com.sun.jndi.ldap.tls.cbtype” that indicates
>>> Channel Binding type that should be used in the LDAPS connection over 
>>> JGSS/Kerberos.
>>> Right now "tls-server-end-point" Channel Binding type is supported only.
>>> The client extracts server certificate from the underlying TLS connection 
>>> and creates
>>> Channel Binding data for JGSS/Kerberos authentication if application 
>>> indicates
>>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
>>> Client application should also specify existing 
>>> "com.sun.jndi.ldap.connect.timeout” property
>>> to force and wait TLS handshake completion before JGSS/Kerberos 
>>> authentication data is generated.
>>> 
>>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
>>> 
>>> [2] -
>>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
>>> 
>>> Initial discussion of this issue is available at security-dev list:
>>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html
>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html
> 

RE: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API (Incubator): AArch64 backend changes

[Yang Zhang]

> But to my earlier question. please: can the new instructions be moved into 
> jdk head first, and then merged into the Panama branch, or not?

The new instructions can be classified as:
1. Instructions that can be matched with NEON instructions directly.
MulVB and SqrtVF have been merged into jdk master already. The patch of AbsV is 
in review [1].

2. Instructions that Jdk master has middle end support for, but they cannot be 
matched with NEON instructions directly.
Such as AddReductionVL, MulReductionVL, And/Or/XorReductionV
These new instructions can be moved into jdk master first, but for 
auto-vectorization, the performance might not get improved.
May I have a new patch for these? 

3. Panama/Vector API specific  instructions
Such as Load/StoreVector ( 16 bits), VectorReinterpret, VectorMaskCmp, 
MaxV/MinV, VectorBlend etc. 
These instructions cannot be moved into jdk master first because there isn't 
middle-end support.

Regards
Yang

[1] 
https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-May/008861.html

-Original Message-
From: Andrew Haley  
Sent: Tuesday, May 26, 2020 4:25 PM
To: Yang Zhang ; Paul Sandoz 
Cc: hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net; aarch64-port-...@openjdk.java.net; nd 

Subject: Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API 
(Incubator): AArch64 backend changes

On 25/05/2020 09:26, Yang Zhang wrote:
> In jdk master, what we need to do is that writing m4 file for existing 
> vector instructions and placed them to a new file aarch64_neon.ad.
> If no question, I will do it right away.

I'm not entirely sure that such a change is necessary now. In particular, 
reorganizing the existing vector instructions is IMO excessive, but I admit 
that it might be an improvement.

But to my earlier question. please: can the new instructions be moved into jdk 
head first, and then merged into the Panama branch, or not?
It'd help if this was possible.

--
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd.  https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR: JDK-8245831: Unify code parsing version strings on Mac and Windows

[alexander . matveev]

Hi Alexey,

Looks good.

Thanks,
Alexander

On 5/26/20 12:26 PM, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

Get rid of duplicated code parsing version strings. Move the code 
parsing version strings to dedicated classes with unit test coverage. 
Also remove Mac specific identifier setting in app's config file from 
the shared code.


- Alexey

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

[2] http://cr.openjdk.java.net/~asemenyuk/8245831/webrev.00

Re: RFR: JDK-8245202: Convert existing jpackage tests to newer form.

[alexander . matveev]

Looks good.

Thanks,
Alexander

On 5/26/20 1:43 PM, Andy Herrick wrote:

revised at [3] to combine JavaOptionsTest and JavaOptionsModular tests

[3] - http://cr.openjdk.java.net/~herrick/8245202/webrev.02/

/Andy

On 5/26/2020 8:08 AM, Andy Herrick wrote:

Please review the fix to issue [1] at [2].

The change removes all the old form helper tests and converts the 6 
remaining tests that used the old helpers into 4 new tests using the 
new helpers.


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

[2] - http://cr.openjdk.java.net/~herrick/8245202/webrev.01/

/Andy

Re: Possible optimization in StringLatin1.regionMatchesCI

[Claes Redestad]

So to try and clarify:

if (Character.toLowerCase(u1) == Character.toLowerCase(u2))

... can never happen today in the context of the StringLatin1 version
of regionMatchesCI (I did a quick check), and a test that exhaustively
tests this property holds should ensure any future unicode updates
doesn't trip us (unlikely -- but not theoretically impossible).

I think we can go ahead with this.

/Claes

On 2020-05-26 18:27, Martin Buchholz wrote:

On Tue, May 26, 2020 at 4:07 AM Christoph Dreis
 wrote:


Hi Martin,

> Not a review, but:

Compare with the variant of this code in StringUTF16.
StringLatin1 only ever needs to support the first 256 chars in Unicode


Does it really? That makes me wonder even more about the additional lowercase 
check.


which can never change, unlike StringUTF16,


What do you mean by "can never change"?


When we discover sentient life on Titan, their script needs to get
added to Unicode.  But the first 256 chars are already fully
allocated; the Titans will be given empty space elsewhere.  Hopefully
Unicode won't be clogged by a million emojis at that point.

There's a real fear of eszett capitalization changing. After centuries
of debate the German Sprachbund will finally decide to (wisely!)
abolish eszett, but Liechtenstein will be the only holdout insisting
that eszett be capitalized to
https://en.wikipedia.org/wiki/Capital_%E1%BA%9E

Fortunately the code we are reviewing here is Locale-independent, and
so is hopefully immune to the future politics of Liechtenstein.

Re: RFR: JDK-8245202: Convert existing jpackage tests to newer form.

[Alexey Semenyuk]

Looks good.

- Alexey

On 5/26/2020 4:43 PM, Andy Herrick wrote:

revised at [3] to combine JavaOptionsTest and JavaOptionsModular tests

[3] - http://cr.openjdk.java.net/~herrick/8245202/webrev.02/

/Andy

On 5/26/2020 8:08 AM, Andy Herrick wrote:

Please review the fix to issue [1] at [2].

The change removes all the old form helper tests and converts the 6 
remaining tests that used the old helpers into 4 new tests using the 
new helpers.


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

[2] - http://cr.openjdk.java.net/~herrick/8245202/webrev.01/

/Andy

Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers

[Claes Redestad]

On 2020-05-26 20:56, fo...@univ-mlv.fr wrote:

- Mail original -

De: "Claes Redestad" 
À: "Paul Sandoz" 
Cc: "Remi Forax" , "core-libs-dev" 

Envoyé: Mardi 26 Mai 2020 20:31:51
Objet: Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers



On 2020-05-26 20:02, Paul Sandoz wrote:

I prefer the first revision from a simplicity perspective.

Are there any measurable benefits in the subsequent revision? It requires some
careful reading, where the logic in the prepender is duplicated in the layering
of the computeIfAbsent functions. Which, if needed, is fine.


Right, it depends on test. On the ObjStringCombos test there's no
real gain - on a case where there are more interleaving constants
it should have more of an effect.

I'll go ahead with the first version and we can think through if there
are ways to simplify this for a follow-up, maybe along with a fix to
remove prefix constants altogether. (the trick is to bind in the
constants without binding more things directly into the main MH
combinator tree - the complex prependers reduced the possible tree
shapes a lot)


I should have shut my big mouth,
i agree that the first patch is actually better.



Ok, I'll go ahead and push that version.

/Claes





Are the existing test sufficient to cover these cases? You referenced
ObjStringCombos and I wondering if that makes sense as a combo stress test?


Adding more stress tests that test all combinations at more arities
would be good. My bootstrap tests as written are a bit poor since I
don't do any verification apart from checking that they don't crash.
I'll file a standalone RFE to improve the coverage.

/Claes


Rémi





Paul.


On May 26, 2020, at 9:20 AM, Claes Redestad  wrote:

On 2020-05-26 00:48, fo...@univ-mlv.fr wrote:

Not sure in which sense you mean inlining? Few of the methods in the
bootstrap code are likely hot enough to see inlining by a JIT - and the
resulting MHs should be identical, just not constructed over and over
again.

I'm wondering if the code without the null tests was not simple enough to be
inlined by c1,
the MH creations tends to do a lot of checks that can be removed even by c1.


Time spent in prepender() does not seem to be a significant contributor
to bootstrap overhead - time spent in MHs.insertArguments is, though.

One thing that falls out naturally from this is that prefix can only be
non-null on the first prepender: "foo" + a + "bar" + b ... will bind
both "foo" and "bar" to the a-prepender, then b and subsequent
prependers will never see a prefix constant since it will be bound in
as a suffix of the preceding argument. Thus it makes sense to either
provide a caching mechanism for prependers whose prefix arguments are
pre-bound to null:

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

We might be able to special-case the first prepender so that we can
remove the prefix argument from prependers in general, which might make
the final MH a bit smaller and easier for the JIT to optimize.

Testing: tier1+2

/Claes

RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[Mandy Chung]

Lookup::defineHiddenClass currently throws IAE by ASM if the given bytes 
are of unsupported class file version.  The implementation should catch 
and throw UnsupportedClassVersionError instead.


webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/

This patch also includes a spec clarification of @throws IAE if the
the bytes has ACC_MODULE flag set to fix JDK-8245596.

thanks
Mandy

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Mark Kralj-Taylor]

David,
Thanks for taking this enhancement, and making it work on the older glibc
(pre 2.17) Linux platforms currently supported by openjdk.

I like that it is a small change to split the JVM startup check on
availability of Posix clock_gettime/getres() APIs and then if additionally
CLOCK_MONOTONIC is supported.

On Daniel's comment on the spec. Yes the nice part of this is that the
JavaDoc on Clock.systemUTC() is worded to allow higher clock resolution
than system.currentTimeMillis().

Please let me know if I can help any more on this.
Mark

On Tue, 26 May 2020 at 16:35, Daniel Fuchs  wrote:

> Hi David,
>
> This is not a review for the posix code.
>
> Your webrev looks good to me and corresponds to what I expected
> to see. I understand that not all operating systems / platforms
> are expected to have the nano second precision, so your test
> probably can't go much beyond what is currently being tested.
>
> Last time I upgraded the system clock to micro second precision [1]
> I had to write a CSR and release notes. That was the first time
> the clock went beyond millisecond precision however - and my
> expectation is that your proposed change should no longer
> require a CSR as potential nanosecond precision should
> now be covered by the spec.
>
> I had to modify a few other places as well (see [1]) - where precision
> greater than 1ms was not expected, but these modifications
> should cover your current changes too, so I don't think anything
> more is required. Some --test-repeat might be in order
> to verify things are stable but I don't expect any issue there :-)
>
> LGTM.
>
> best regards,
>
> -- daniel
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8068730
>
> On 26/05/2020 05:59, David Holmes wrote:
> > bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> > webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> >
> > This work was contributed by Mark Kralj-Taylor:
> >
> >
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
> >
> >
> > On the hotspot side we change the Linux implementations of
> > javaTimeMillis() and javaTimeSystemUTC() so that they use
> > clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> > our conditional use of this API I added a new guard
> >
> > os::Posix::supports_clock_gettime()
> >
> > and refined the existing supports_monotonic_clock() so that we can still
> > use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> > (hopefully very near future) we will simply assume these APIs always
> exist.
> >
> > On the core-libs side the existing test:
> >
> > test/jdk/java/time/test/java/time/TestClock_System.java
> >
> > is adjusted to track the precision in more detail.
> >
> > Finally Mark has added instantNowAsEpochNanos() to the benchmark
> > previously known as
> >
> > test/micro/org/openjdk/bench/java/lang/Systems.java
> >
> > which I've renamed to SystemTime.java as Mark suggested. I agree having
> > these three functions measured together makes sense.
> >
> > Testing:
> >- test/jdk/java/time/test/java/time/TestClock_System.java
> >- test/micro/org/openjdk/bench/java/lang/SystemTime.java
> >- Tiers 1-3
> >
> > Thanks,
> > David
>
>

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

[Michael Osipov]

Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:

Hello,

Could you please review the following patch:

JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/


Let's go through your changes statically:

* The JIRA issue title has a typo
* The word "cannot" is incorrectly spelled throughout all exception messages


+if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) 
{
+throw new UnsupportedOperationException("LdapCtx: " +
+TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not 
supported");



"LdapCtx: " is redundant because the stacktrace will contain the class
name already. A better message would be: "Channel binding type '%s' is
not supported". Not just the plain value.


+} else if 
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+if (connectTimeout == -1)
+throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + " 
property requires " +
+CONNECT_TIMEOUT + " property is set.");


* Same here with the message
* The IAE is wrong because passed value is correct, but leads to an
invalid state because connection timeout is -1. You need an
IllegalStateException here.

Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?


--- 
old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java  
2020-05-18 19:39:46.0 +0300
+++ 
new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java  
2020-05-18 19:39:46.0 +0300
@@ -531,9 +531,12 @@
 public void setChannelBinding(ChannelBinding channelBindings)
 throws GSSException {

-if (mechCtxt == null)
+if (mechCtxt == null) {
+if (this.channelBindings  != null) {
+throw new GSSException(GSSException.BAD_BINDINGS);
+}
 this.channelBindings = channelBindings;
-
+}
 }


I don't understand the purpose of this hunk. Is this safeguard to set
bindings only once?


 private static final int CHANNEL_BINDING_AF_INET = 2;
 private static final int CHANNEL_BINDING_AF_INET6 = 24;
 private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+private static final int CHANNEL_BINDING_AF_UNSPEC = 0;


This should sort from 0 to 255 and not at the end.


 private int getAddrType(InetAddress addr) {
-int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+int addressType = CHANNEL_BINDING_AF_UNSPEC;



   // initialize addrtype in CB first
-  cb->initiator_addrtype = GSS_C_AF_NULLADDR;
-  cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+  cb->initiator_addrtype = GSS_C_AF_UNSPEC;
+  cb->acceptor_addrtype = GSS_C_AF_UNSPEC;


This looks wrong to me -- as you already mentioned -- this violates RFC
2744, section 3.11, last sentence:

or omit addressing information, specifying
   GSS_C_AF_NULLADDR as the address-types.



   /* release initiator address */
-  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
+  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
+  cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
 resetGSSBuffer(&(cb->initiator_address));
   }
   /* release acceptor address */
-  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
+  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
+  cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
 resetGSSBuffer(&(cb->acceptor_address));
   }


Unspecified does not mean that it is null.


+final byte[] prefix = 
(TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
+byte[] cbData =  Arrays.copyOf(prefix,
+prefix.length + tlsCB.getData().length 
);
+System.arraycopy(tlsCB.getData(), 0, cbData,  
prefix.length, tlsCB.getData().length);


Since you are calling "tlsCB.getData()" multiple times, this should go
into a variable.



+secCtx.setChannelBinding(new

ChannelBinding(null, null, cbData));

Why not use new ChannelBinding(cbData)?


+String hashAlg = serverCertificate.getSigAlgName().
+replace("SHA", "SHA-").toUpperCase();
+int ind = hashAlg.indexOf("WITH");
+if (ind > 0) {
+hashAlg = hashAlg.substring(0, ind);
+if (hashAlg.equals("MD5") || hashAlg.equals("SHA-1")) {
+hashAlg = "SHA-256";
+}


I have several problems with that, some might be hypothetical:

* toUpperCase() should be qualified with Locale.ROOT or Locate.ENGLISH
* Looking at https://tools.ietf.org/html/rfc5280#section-4.1.1.2, then
at sun.security.x509.AlgorithmId.getName() it uses nameTable to
translate OIDs to readible names.

With indexOf("WITH") you are implying that the cert's sig alg 

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

[Michael Osipov]

Am 2020-05-24 um 01:38 schrieb Michael Osipov:

Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:
...
What about introducing a UnspecEmptyInetAddress() which gives the proper
AF type, but #getAddress() would return null? This should satisfy the
requirements, InitialToken as well as the RFC. this of course needs to
be properly passed to native providers too. GssKrb5Client would also
need to know that this channel binding is explicitly for Active
Directory and not some other, spec-compliant, SASL peer (property on
LdapCtx?)


Giving this more thought. I believe you have also found a bug in
InitialToken#getAddrType(InetAddress). It would return
CHANNEL_BINDING_AF_NULL_ADDR if addr is neither Inet6 nor Inet6, but is
not null which is wrong. But this is just a hypothetical case.

M

Re: RFR: JDK-8245202: Convert existing jpackage tests to newer form.

[Andy Herrick]

revised at [3] to combine JavaOptionsTest and JavaOptionsModular tests

[3] - http://cr.openjdk.java.net/~herrick/8245202/webrev.02/

/Andy

On 5/26/2020 8:08 AM, Andy Herrick wrote:

Please review the fix to issue [1] at [2].

The change removes all the old form helper tests and converts the 6 
remaining tests that used the old helpers into 4 new tests using the 
new helpers.


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

[2] - http://cr.openjdk.java.net/~herrick/8245202/webrev.01/

/Andy

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

[Valerie Peng]

I am also concerned about the changes in GSSLibStub.c about the default 
value being GSS_C_AF_UNSPECinstead of GSS_C_AF_NULLADDR (line 194-195).


Can you try and see if Window works with GSS_C_AF_NULLADDR? If yes, I'd 
prefer to not changing the default value for addresstype for the same 
reason which Michael has already stated.


Thanks,
Valerie

On 5/25/2020 8:33 AM, Alexey Bakhtin wrote:

Hello Michael, Thomas,

Thank you a lot for review and suggestions.
I’ve fixed most of the issues except of fundamental one
I need more time to evaluate suggested usage of UnspecEmptyInetAddress subtype.

Updated webrev is available at: 
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/

Also, please see my comments below.

Regards
Alexey


On 24 May 2020, at 02:38, Michael Osipov <1983-01...@gmx.net> wrote:

Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:

Hello,

Could you please review the following patch:

JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/

Let's go through your changes statically:

* The JIRA issue title has a typo

Thank you. Fixed in Jira

* The word "cannot" is incorrectly spelled throughout all exception messages

Fixed from “can not” to “cannot"

+if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) 
{
+throw new UnsupportedOperationException("LdapCtx: " +
+TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not 
supported");


"LdapCtx: " is redundant because the stacktrace will contain the class
name already. A better message would be: "Channel binding type '%s' is
not supported". Not just the plain value.

Exception message is corrected

+} else if 
(cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+if (connectTimeout == -1)
+throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + " 
property requires " +
+CONNECT_TIMEOUT + " property is set.");

* Same here with the message

Not sure, What’s wrong with the message ?

* The IAE is wrong because passed value is correct, but leads to an
invalid state because connection timeout is -1. You need an
IllegalStateException here.

Thank you. You are right again. Changed to IllegalStateException

Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?

This logic already existed here. It could be a reason for it and I don’t want 
change it without strong purpose.
The only changes here is to prevent double setting of channel binding data.


--- 
old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java  
2020-05-18 19:39:46.0 +0300
+++ 
new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java  
2020-05-18 19:39:46.0 +0300
@@ -531,9 +531,12 @@
 public void setChannelBinding(ChannelBinding channelBindings)
 throws GSSException {

-if (mechCtxt == null)
+if (mechCtxt == null) {
+if (this.channelBindings  != null) {
+throw new GSSException(GSSException.BAD_BINDINGS);
+}
 this.channelBindings = channelBindings;
-
+}
 }

I don't understand the purpose of this hunk. Is this safeguard to set
bindings only once?


 private static final int CHANNEL_BINDING_AF_INET = 2;
 private static final int CHANNEL_BINDING_AF_INET6 = 24;
 private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+private static final int CHANNEL_BINDING_AF_UNSPEC = 0;

This should sort from 0 to 255 and not at the end.

OK. Moved to the top.


 private int getAddrType(InetAddress addr) {
-int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+int addressType = CHANNEL_BINDING_AF_UNSPEC;
   // initialize addrtype in CB first
-  cb->initiator_addrtype = GSS_C_AF_NULLADDR;
-  cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+  cb->initiator_addrtype = GSS_C_AF_UNSPEC;
+  cb->acceptor_addrtype = GSS_C_AF_UNSPEC;

This looks wrong to me -- as you already mentioned -- this violates RFC
2744, section 3.11, last sentence:

or omit addressing information, specifying
   GSS_C_AF_NULLADDR as the address-types.
   /* release initiator address */
-  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
+  if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
+  cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
 resetGSSBuffer(&(cb->initiator_address));
   }
   /* release acceptor address */
-  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
+  if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
+  cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
 resetGSSBuffer(&(cb->acceptor_address));
   }

Unspecified does not mean that it is null.


+final byte[] prefix = 
(TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
+byte[] cbData =  

RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

[Stuart Marks]

Hi all,

Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.


I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:


 - java.rmi.activation package spec: add deprecation warning
 - java.rmi module spec: link to activation package spec
 - java.rmi.activation public classes and interfaces: deprecate
 - various other files: suppress warnings
 - Activation.java: have the rmid tool emit a deprecation warning
 - rmid.properties: localized warning message

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/

Bug ID:

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

JEP:

http://openjdk.java.net/jeps/385

Thanks,

s'marks

RFR: JDK-8245831: Unify code parsing version strings on Mac and Windows

[Alexey Semenyuk]

Please review fix [2] for jpackage bug [1].

Get rid of duplicated code parsing version strings. Move the code 
parsing version strings to dedicated classes with unit test coverage. 
Also remove Mac specific identifier setting in app's config file from 
the shared code.


- Alexey

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

[2] http://cr.openjdk.java.net/~asemenyuk/8245831/webrev.00

Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers

[forax]

- Mail original -
> De: "Claes Redestad" 
> À: "Paul Sandoz" 
> Cc: "Remi Forax" , "core-libs-dev" 
> 
> Envoyé: Mardi 26 Mai 2020 20:31:51
> Objet: Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory 
> prependers

> On 2020-05-26 20:02, Paul Sandoz wrote:
>> I prefer the first revision from a simplicity perspective.
>> 
>> Are there any measurable benefits in the subsequent revision? It requires 
>> some
>> careful reading, where the logic in the prepender is duplicated in the 
>> layering
>> of the computeIfAbsent functions. Which, if needed, is fine.
> 
> Right, it depends on test. On the ObjStringCombos test there's no
> real gain - on a case where there are more interleaving constants
> it should have more of an effect.
> 
> I'll go ahead with the first version and we can think through if there
> are ways to simplify this for a follow-up, maybe along with a fix to
> remove prefix constants altogether. (the trick is to bind in the
> constants without binding more things directly into the main MH
> combinator tree - the complex prependers reduced the possible tree
> shapes a lot)

I should have shut my big mouth,
i agree that the first patch is actually better.

> 
>> 
>> Are the existing test sufficient to cover these cases? You referenced
>> ObjStringCombos and I wondering if that makes sense as a combo stress test?
> 
> Adding more stress tests that test all combinations at more arities
> would be good. My bootstrap tests as written are a bit poor since I
> don't do any verification apart from checking that they don't crash.
> I'll file a standalone RFE to improve the coverage.
> 
> /Claes

Rémi

> 
>> 
>> Paul.
>> 
>>> On May 26, 2020, at 9:20 AM, Claes Redestad  
>>> wrote:
>>>
>>> On 2020-05-26 00:48, fo...@univ-mlv.fr wrote:
> Not sure in which sense you mean inlining? Few of the methods in the
> bootstrap code are likely hot enough to see inlining by a JIT - and the
> resulting MHs should be identical, just not constructed over and over
> again.
 I'm wondering if the code without the null tests was not simple enough to 
 be
 inlined by c1,
 the MH creations tends to do a lot of checks that can be removed even by 
 c1.
>>>
>>> Time spent in prepender() does not seem to be a significant contributor
>>> to bootstrap overhead - time spent in MHs.insertArguments is, though.
>>>
>>> One thing that falls out naturally from this is that prefix can only be
>>> non-null on the first prepender: "foo" + a + "bar" + b ... will bind
>>> both "foo" and "bar" to the a-prepender, then b and subsequent
>>> prependers will never see a prefix constant since it will be bound in
>>> as a suffix of the preceding argument. Thus it makes sense to either
>>> provide a caching mechanism for prependers whose prefix arguments are
>>> pre-bound to null:
>>>
>>> http://cr.openjdk.java.net/~redestad/8245756/open.01
>>>
>>> We might be able to special-case the first prepender so that we can
>>> remove the prefix argument from prependers in general, which might make
>>> the final MH a bit smaller and easier for the JIT to optimize.
>>>
>>> Testing: tier1+2
>>>
>>> /Claes

Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers

[Claes Redestad]

On 2020-05-26 20:02, Paul Sandoz wrote:

I prefer the first revision from a simplicity perspective.

Are there any measurable benefits in the subsequent revision? It requires some 
careful reading, where the logic in the prepender is duplicated in the layering 
of the computeIfAbsent functions. Which, if needed, is fine.


Right, it depends on test. On the ObjStringCombos test there's no
real gain - on a case where there are more interleaving constants
it should have more of an effect.

I'll go ahead with the first version and we can think through if there
are ways to simplify this for a follow-up, maybe along with a fix to
remove prefix constants altogether. (the trick is to bind in the 
constants without binding more things directly into the main MH

combinator tree - the complex prependers reduced the possible tree
shapes a lot)



Are the existing test sufficient to cover these cases? You referenced 
ObjStringCombos and I wondering if that makes sense as a combo stress test?


Adding more stress tests that test all combinations at more arities
would be good. My bootstrap tests as written are a bit poor since I
don't do any verification apart from checking that they don't crash.
I'll file a standalone RFE to improve the coverage.

/Claes



Paul.


On May 26, 2020, at 9:20 AM, Claes Redestad  wrote:

On 2020-05-26 00:48, fo...@univ-mlv.fr wrote:

Not sure in which sense you mean inlining? Few of the methods in the
bootstrap code are likely hot enough to see inlining by a JIT - and the
resulting MHs should be identical, just not constructed over and over
again.

I'm wondering if the code without the null tests was not simple enough to be 
inlined by c1,
the MH creations tends to do a lot of checks that can be removed even by c1.


Time spent in prepender() does not seem to be a significant contributor
to bootstrap overhead - time spent in MHs.insertArguments is, though.

One thing that falls out naturally from this is that prefix can only be
non-null on the first prepender: "foo" + a + "bar" + b ... will bind
both "foo" and "bar" to the a-prepender, then b and subsequent
prependers will never see a prefix constant since it will be bound in
as a suffix of the preceding argument. Thus it makes sense to either
provide a caching mechanism for prependers whose prefix arguments are
pre-bound to null:

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

We might be able to special-case the first prepender so that we can
remove the prefix argument from prependers in general, which might make
the final MH a bit smaller and easier for the JIT to optimize.

Testing: tier1+2

/Claes

Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers

[Paul Sandoz]

I prefer the first revision from a simplicity perspective. 

Are there any measurable benefits in the subsequent revision? It requires some 
careful reading, where the logic in the prepender is duplicated in the layering 
of the computeIfAbsent functions. Which, if needed, is fine.

Are the existing test sufficient to cover these cases? You referenced 
ObjStringCombos and I wondering if that makes sense as a combo stress test? 

Paul.

> On May 26, 2020, at 9:20 AM, Claes Redestad  wrote:
> 
> On 2020-05-26 00:48, fo...@univ-mlv.fr wrote:
>>> Not sure in which sense you mean inlining? Few of the methods in the
>>> bootstrap code are likely hot enough to see inlining by a JIT - and the
>>> resulting MHs should be identical, just not constructed over and over
>>> again.
>> I'm wondering if the code without the null tests was not simple enough to be 
>> inlined by c1,
>> the MH creations tends to do a lot of checks that can be removed even by c1.
> 
> Time spent in prepender() does not seem to be a significant contributor
> to bootstrap overhead - time spent in MHs.insertArguments is, though.
> 
> One thing that falls out naturally from this is that prefix can only be
> non-null on the first prepender: "foo" + a + "bar" + b ... will bind
> both "foo" and "bar" to the a-prepender, then b and subsequent
> prependers will never see a prefix constant since it will be bound in
> as a suffix of the preceding argument. Thus it makes sense to either
> provide a caching mechanism for prependers whose prefix arguments are
> pre-bound to null:
> 
> http://cr.openjdk.java.net/~redestad/8245756/open.01
> 
> We might be able to special-case the first prepender so that we can
> remove the prefix argument from prependers in general, which might make
> the final MH a bit smaller and easier for the JIT to optimize.
> 
> Testing: tier1+2
> 
> /Claes

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

[Bernd Eckenfels]

Not completely sure about which of the involved apIs have what possible 
extensions. Maybe we can somehow make two mechanisms one which is the 
compatible default and one would be the rfc compliant method. Then SASL can be 
configured and use different mechanism names with a new propert? That would 
help jgss for the other mechanisms for channel bindings (cbt) as well. Not sure 
how jgss and JSSE will talk to each other.. via SASL?


--
http://bernd.eckenfels.net

Von: core-libs-dev  im Auftrag von 
Alexey Bakhtin 
Gesendet: Tuesday, May 26, 2020 7:46:11 PM
An: Weijun Wang 
Cc: security-...@openjdk.java.net ; 
core-libs-dev@openjdk.java.net ; Michael Osipov 

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

Hello Max,

Thank you review.
If I understand correctly tls-server-end-point channel binding data is a hash 
of server certificate. Different SASL protocols could send cbData differently, 
with different prefix format. This is a reason I create TLSChannelBinding class 
and calculate hash from the LdapClient and add “tls-server-end-point:” prefix 
in the JGSS/Kerberos.

Alexey

> On 26 May 2020, at 17:50, Weijun Wang  wrote:
>
> I have a question on GssKrb5Client.java:
>
> Do you think it's a good idea to let the SASL mechanism understand what TLS 
> binding is? Instead of passing the whole TlsChannelBinding object through a 
> SASL property, is it possible to only pass the actual cbData? After all, the 
> name "com.sun.security.sasl.channelbinding" suggests it's just a general 
> ChannelBinding which is independent with any application level info.
>
> From my reading of the code change, it looks like this cbData can be 
> calculated on the LDAP side.
>
> Thanks,
> Max
>
>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
>>
>> Hello,
>>
>> Could you please review the following patch:
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>>
>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
>> tls-server-end-point channel binding
>> (based on the TLS server certificate). The channel binding data is 
>> calculated as following :
>>   • The client calculates a hash of the TLS server certificate.
>>  The hash algorithm is selected on the base of the certificate 
>> signature algorithm.
>>  Also, the client should use SHA-256 algorithm, in case of the 
>> certificate signature algorithm is SHA1 or MD5 based
>>   • The channel binding information is the same as defined in rfc4121 
>> [1] with small corrections:
>>   • initiator and acceptor addresses should be set to NULL
>>   • initiator and acceptor address types should be zero.
>> It contradicts to the “Using Channel Bindings in GSS-API” 
>> document [2] that say that
>> the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>> instead of GSS_C_AF_UNSPEC=0x00.
>>
>> This patch introduces changes in the LDAP, SASL and JGSS modules
>> to generate channel binding data automatically if requested by an 
>> application.
>> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation 
>> but changes
>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
>> CHANNEL_BINDING_AF_UNSPEC.
>> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” 
>> that indicates
>> Channel Binding type that should be used in the LDAPS connection over 
>> JGSS/Kerberos.
>> Right now "tls-server-end-point" Channel Binding type is supported only.
>> The client extracts server certificate from the underlying TLS connection 
>> and creates
>> Channel Binding data for JGSS/Kerberos authentication if application 
>> indicates
>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
>> Client application should also specify existing 
>> "com.sun.jndi.ldap.connect.timeout” property
>> to force and wait TLS handshake completion before JGSS/Kerberos 
>> authentication data is generated.
>>
>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
>>
>> [2] -
>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
>>
>> Initial discussion of this issue is available at security-dev list:
>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html

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

[Alexey Bakhtin]

Hello Max,

Thank you review.
If I understand correctly tls-server-end-point channel binding data is a hash 
of server certificate. Different SASL protocols could send cbData differently, 
with different prefix format. This is a reason I create TLSChannelBinding class 
and calculate hash from the LdapClient and add “tls-server-end-point:” prefix 
in the JGSS/Kerberos.

Alexey

> On 26 May 2020, at 17:50, Weijun Wang  wrote:
> 
> I have a question on GssKrb5Client.java:
> 
> Do you think it's a good idea to let the SASL mechanism understand what TLS 
> binding is? Instead of passing the whole TlsChannelBinding object through a 
> SASL property, is it possible to only pass the actual cbData? After all, the 
> name "com.sun.security.sasl.channelbinding" suggests it's just a general 
> ChannelBinding which is independent with any application level info.
> 
> From my reading of the code change, it looks like this cbData can be 
> calculated on the LDAP side.
> 
> Thanks,
> Max
> 
>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
>> 
>> Hello,
>> 
>> Could you please review the following patch:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>> 
>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
>> tls-server-end-point channel binding
>> (based on the TLS server certificate). The channel binding data is 
>> calculated as following :
>>  • The client calculates a hash of the TLS server certificate.
>>  The hash algorithm is selected on the base of the certificate 
>> signature algorithm.
>>  Also, the client should use SHA-256 algorithm, in case of the 
>> certificate signature algorithm is SHA1 or MD5 based
>>  • The channel binding information is the same as defined in rfc4121 [1] 
>> with small corrections:
>>  • initiator and acceptor addresses should be set to NULL
>>  • initiator and acceptor address types should be zero.
>> It contradicts to the “Using Channel Bindings in GSS-API” 
>> document [2] that say that
>> the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>> instead of GSS_C_AF_UNSPEC=0x00.
>> 
>> This patch introduces changes in the LDAP, SASL and JGSS modules
>> to generate channel binding data automatically if requested by an 
>> application.
>> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation 
>> but changes
>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
>> CHANNEL_BINDING_AF_UNSPEC.
>> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” 
>> that indicates
>> Channel Binding type that should be used in the LDAPS connection over 
>> JGSS/Kerberos.
>> Right now "tls-server-end-point" Channel Binding type is supported only.
>> The client extracts server certificate from the underlying TLS connection 
>> and creates
>> Channel Binding data for JGSS/Kerberos authentication if application 
>> indicates
>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
>> Client application should also specify existing 
>> "com.sun.jndi.ldap.connect.timeout” property
>> to force and wait TLS handshake completion before JGSS/Kerberos 
>> authentication data is generated.
>> 
>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
>> 
>> [2] -
>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
>> 
>> Initial discussion of this issue is available at security-dev list:
>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html

Re: RFR(S): 8245600: Clean up libjli

[Mikael Vidstedt]

> On May 22, 2020, at 4:27 AM, Alan Bateman  wrote:
> 
> 
> 
> On 22/05/2020 04:28, Mikael Vidstedt wrote:
>> Please review this change which cleans up the libjli related files a bit:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245600
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8245600/webrev.00/open/webrev/
>> 
> Looks good, don't have a strong opinion on the function name but if I were 
> doing this when I'd probably got for something like CurrentTimeMicros.

I liked it so much I changed it.

> Minor nit is that the JLI_TraceLauncher usages in java.c probably don't need 
> to be across two lines now.

Fixed.


I made the changes and pushed the change. Here’s the final webrev for 
completeness:

http://cr.openjdk.java.net/~mikael/webrevs/8245600/webrev.01/open/webrev/

Cheers,
Mikael

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

[Alexey Bakhtin]

Hello Aleks, Daniel,

Thank you for your comments.
I don’t like that the code is split into several modules too. The reason of 
doing it is I can not get TLS server certificate from the JGSS/Kerberos code. 
The only place Where I can fetch it is LdapClient.
If I understand your idea correctly, I can extract TLS server certificate in 
the LdapClient and put it into internal environment property as byte array 
without mention about Channel Binding. It will be done for every LDAPS 
connection.
In case of “com.sun.security.sasl.channelbinding=tls-server-end-point” property 
specified, GssKrb5Client extracts certificate from the internal environment 
property and use it to create TLS Channel Binding. In this case almost all 
Channel Binding code could be moved to GssKrb5Client. LdapClient still changed 
but not mention about Channel Binding. Changes in the LdapCtx could be omitted.

Regards
Alexey

> On 26 May 2020, at 17:55, Aleks Efimov  wrote:
> 
> Hi Alexey,
> 
> I agree with all Daniel's comments.
> 
> Few thoughts about moving the implementation down to SASL layers:
> Will it be possible to make this new code specific only for JGSS/Kerberos 
> authentication mechanism?
> Maybe investigate moving all the new code to GssKrb5Client SASL client 
> implementation (GssKrb5Client class, "GSSAPI" authentication mechanism name):
> - That may require to store the server certificate extracted from SSLSocket 
> into new context environment property
> - The code that processes and checks the String value of channel binding type 
> value could also be moved to GssKrb5Client or to TlsChannelBinding
> - Add TlsChannelBinding factory method that creates an object from the server 
> certificate and the string value of the environment property could also be 
> useful here
> 
> All of that will allow us to keep the fix specific to "JGSS/Kerberos" area 
> and will keep LdapCtx/LdapClient code changes minimal and clear of 
> "JGSS/Kerberos" details
> 
> Best Regards,
> Aleksei
> 
> On 26/05/2020 15:14, Daniel Fuchs wrote:
>> Hi Alexey,
>> 
>> This is not a review. A few high level comments however:
>> 
>> For that kind of change that introduce a new environment
>> property you will need to file a CSR, and probably provide
>> some release notes as well.
>> 
>> Your changes seem to trigger new IllegalStateException and
>> UnsupportedOperationExceptions which are undocumented.
>> I believe they should be replaced by NamingException which
>> is documented at the public API level.
>> 
>> Also it would be good to have a test that validates that
>> the proposed changes works as expected.
>> 
>> I will not comment on the security libs changes - I'm clearly
>> out of my depth there. It's a bit distasteful that the
>> LdapCtxt/LdapClient have to have knowledge of channel binding
>> and extract the certificates from the SSLSocket to pass them to
>> the lower layer. Ideally this should rather be handled by the
>> SASL layers?
>> 
>> best regards,
>> 
>> -- daniel
>> 
>> 
>> On 25/05/2020 16:33, Alexey Bakhtin wrote:
>>> Updated webrev is available 
>>> at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/
>> 

Re: Possible optimization in StringLatin1.regionMatchesCI

[Martin Buchholz]

On Tue, May 26, 2020 at 4:07 AM Christoph Dreis
 wrote:
>
> Hi Martin,
>
> > Not a review, but:
> > Compare with the variant of this code in StringUTF16.
> > StringLatin1 only ever needs to support the first 256 chars in Unicode
>
> Does it really? That makes me wonder even more about the additional lowercase 
> check.
>
> > which can never change, unlike StringUTF16,
>
> What do you mean by "can never change"?

When we discover sentient life on Titan, their script needs to get
added to Unicode.  But the first 256 chars are already fully
allocated; the Titans will be given empty space elsewhere.  Hopefully
Unicode won't be clogged by a million emojis at that point.

There's a real fear of eszett capitalization changing. After centuries
of debate the German Sprachbund will finally decide to (wisely!)
abolish eszett, but Liechtenstein will be the only holdout insisting
that eszett be capitalized to
https://en.wikipedia.org/wiki/Capital_%E1%BA%9E

Fortunately the code we are reviewing here is Locale-independent, and
so is hopefully immune to the future politics of Liechtenstein.

Re: RFR: 8245756: Reduce bootstrap cost of StringConcatFactory prependers

[Claes Redestad]

On 2020-05-26 00:48, fo...@univ-mlv.fr wrote:

Not sure in which sense you mean inlining? Few of the methods in the
bootstrap code are likely hot enough to see inlining by a JIT - and the
resulting MHs should be identical, just not constructed over and over
again.

I'm wondering if the code without the null tests was not simple enough to be 
inlined by c1,
the MH creations tends to do a lot of checks that can be removed even by c1.



Time spent in prepender() does not seem to be a significant contributor
to bootstrap overhead - time spent in MHs.insertArguments is, though.

One thing that falls out naturally from this is that prefix can only be
non-null on the first prepender: "foo" + a + "bar" + b ... will bind
both "foo" and "bar" to the a-prepender, then b and subsequent
prependers will never see a prefix constant since it will be bound in
as a suffix of the preceding argument. Thus it makes sense to either
provide a caching mechanism for prependers whose prefix arguments are
pre-bound to null:

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

We might be able to special-case the first prepender so that we can
remove the prefix argument from prependers in general, which might make
the final MH a bit smaller and easier for the JIT to optimize.

Testing: tier1+2

/Claes

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Daniel Fuchs]

Hi David,

This is not a review for the posix code.

Your webrev looks good to me and corresponds to what I expected
to see. I understand that not all operating systems / platforms
are expected to have the nano second precision, so your test
probably can't go much beyond what is currently being tested.

Last time I upgraded the system clock to micro second precision [1]
I had to write a CSR and release notes. That was the first time
the clock went beyond millisecond precision however - and my
expectation is that your proposed change should no longer
require a CSR as potential nanosecond precision should
now be covered by the spec.

I had to modify a few other places as well (see [1]) - where precision
greater than 1ms was not expected, but these modifications
should cover your current changes too, so I don't think anything
more is required. Some --test-repeat might be in order
to verify things are stable but I don't expect any issue there :-)

LGTM.

best regards,

-- daniel

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

On 26/05/2020 05:59, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

This work was contributed by Mark Kralj-Taylor:

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html 



On the hotspot side we change the Linux implementations of 
javaTimeMillis() and javaTimeSystemUTC() so that they use 
clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with 
our conditional use of this API I added a new guard


os::Posix::supports_clock_gettime()

and refined the existing supports_monotonic_clock() so that we can still 
use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future 
(hopefully very near future) we will simply assume these APIs always exist.


On the core-libs side the existing test:

test/jdk/java/time/test/java/time/TestClock_System.java

is adjusted to track the precision in more detail.

Finally Mark has added instantNowAsEpochNanos() to the benchmark 
previously known as


test/micro/org/openjdk/bench/java/lang/Systems.java

which I've renamed to SystemTime.java as Mark suggested. I agree having 
these three functions measured together makes sense.


Testing:
   - test/jdk/java/time/test/java/time/TestClock_System.java
   - test/micro/org/openjdk/bench/java/lang/SystemTime.java
   - Tiers 1-3

Thanks,
David

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

[Aleks Efimov]

Hi Alexey,

I agree with all Daniel's comments.

Few thoughts about moving the implementation down to SASL layers:
Will it be possible to make this new code specific only for 
JGSS/Kerberos authentication mechanism?
Maybe investigate moving all the new code to GssKrb5Client SASL client 
implementation (GssKrb5Client class, "GSSAPI" authentication mechanism 
name):
- That may require to store the server certificate extracted from 
SSLSocket into new context environment property
- The code that processes and checks the String value of channel binding 
type value could also be moved to GssKrb5Client or to TlsChannelBinding
- Add TlsChannelBinding factory method that creates an object from the 
server certificate and the string value of the environment property 
could also be useful here


All of that will allow us to keep the fix specific to "JGSS/Kerberos" 
area and will keep LdapCtx/LdapClient code changes minimal and clear of 
"JGSS/Kerberos" details


Best Regards,
Aleksei

On 26/05/2020 15:14, Daniel Fuchs wrote:

Hi Alexey,

This is not a review. A few high level comments however:

For that kind of change that introduce a new environment
property you will need to file a CSR, and probably provide
some release notes as well.

Your changes seem to trigger new IllegalStateException and
UnsupportedOperationExceptions which are undocumented.
I believe they should be replaced by NamingException which
is documented at the public API level.

Also it would be good to have a test that validates that
the proposed changes works as expected.

I will not comment on the security libs changes - I'm clearly
out of my depth there. It's a bit distasteful that the
LdapCtxt/LdapClient have to have knowledge of channel binding
and extract the certificates from the SSLSocket to pass them to
the lower layer. Ideally this should rather be handled by the
SASL layers?

best regards,

-- daniel


On 25/05/2020 16:33, Alexey Bakhtin wrote:
Updated webrev is available 
at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/

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

[Weijun Wang]

I have a question on GssKrb5Client.java:

Do you think it's a good idea to let the SASL mechanism understand what TLS 
binding is? Instead of passing the whole TlsChannelBinding object through a 
SASL property, is it possible to only pass the actual cbData? After all, the 
name "com.sun.security.sasl.channelbinding" suggests it's just a general 
ChannelBinding which is independent with any application level info.

From my reading of the code change, it looks like this cbData can be calculated 
on the LDAP side.

Thanks,
Max

> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
> 
> Hello,
> 
> Could you please review the following patch:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
> 
> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
> tls-server-end-point channel binding
> (based on the TLS server certificate). The channel binding data is calculated 
> as following :
>   • The client calculates a hash of the TLS server certificate.
>   The hash algorithm is selected on the base of the certificate 
> signature algorithm.
>   Also, the client should use SHA-256 algorithm, in case of the 
> certificate signature algorithm is SHA1 or MD5 based
>   • The channel binding information is the same as defined in rfc4121 [1] 
> with small corrections:
>   • initiator and acceptor addresses should be set to NULL
>   • initiator and acceptor address types should be zero.
>  It contradicts to the “Using Channel Bindings in GSS-API” 
> document [2] that say that
>  the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>  instead of GSS_C_AF_UNSPEC=0x00.
> 
> This patch introduces changes in the LDAP, SASL and JGSS modules
> to generate channel binding data automatically if requested by an application.
> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation 
> but changes
> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
> CHANNEL_BINDING_AF_UNSPEC.
> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” 
> that indicates
> Channel Binding type that should be used in the LDAPS connection over 
> JGSS/Kerberos.
> Right now "tls-server-end-point" Channel Binding type is supported only.
> The client extracts server certificate from the underlying TLS connection and 
> creates
> Channel Binding data for JGSS/Kerberos authentication if application indicates
> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
> Client application should also specify existing 
> "com.sun.jndi.ldap.connect.timeout” property
> to force and wait TLS handshake completion before JGSS/Kerberos 
> authentication data is generated.
> 
> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
> 
> [2] -
> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
> 
> Initial discussion of this issue is available at security-dev list:
> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html
> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html

Re: RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

[Maurizio Cimadamore]

Looks good.

For the diagnostic, longer term it would be nice to generalize those:

 # 0: token, 1: token
2168 compiler.err.expected2=\
2169 {0} or {1} expected
2170
2171 # 0: token, 1: token, 2: token
2172 compiler.err.expected3=\
2173 {0}, {1}, or {2} expected
2174
2175 # 0: token, 1: token, 2: token, 3: string
2176 compiler.err.expected4=\
2177 {0}, {1}, {2}, or {3} expected
2178


To work on more argument kinds (since you need the same thing).

I would at leas attempt to follow the same text as in the other 
diagnostics - I don't see "one of:" being used anywhere else in 
compiler.properties.


Maurizio

On 26/05/2020 05:14, Vicente Romero wrote:

Hi Maurizio,

Right good catch I forgot to add the MONKEY_AT to the list of expected 
tokens. I have fixed that. I have published another iteration at [1]. 
Apart from the MONKEY_AT issue addressed at the parser this iteration 
also:


 - adds another error key to compiler.properties, this new key is 
oriented to show a more explicit error message when an interface with 
no `non-sealed` or `sealed` modifier extends a sealed interface. A 
class in this position can also be `final` but this is not possible 
for interfaces.
 - changes to PrintingProcessor and to 
test/langtools/tools/javac/processing/model/element/TestSealed.java 
suggested by Joe in this thread
 - adds a new subtest at SealedCompilationTests testing the sealed and 
non-sealed modifiers being followed by different modifiers or annotations
 - modified a subtest also at SealedCompilationTests, testPrinting, 
that was failing on Windows


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.03.delta/

On 5/25/20 6:37 AM, Maurizio Cimadamore wrote:

Hi,
changes look good, but the parser changes do not convince me. Now 
that the logic is more clearly defined, I see some issues e.g. there 
seems to be a tight coupling by where "sealed" or "non-sealed" is, 
and the fact that the following token must be e.g. "class". This is 
unlike other modifiers which can in fact appear in any configuation.


I believe that you can break the current logic by adding an extra 
annotation between the "sealed" token and the "class" token e.g.


sealed @foo class Bar { }

But maybe the quick fix would be to add MONKEY_AT to the set of 
expected tokens after sealed/non-sealed.


Maurizio


On 22/05/2020 19:25, Vicente Romero wrote:

Hi,

Thanks Jan and Maurizio for the reviews. I have created another 
iteration that I hope addresses all of the comments. I recognize 
that dealing with `sealed`, `non-sealed` is the most complicated 
part as there is no guide right now from the spec. So I have tried 
to make them contextual keywords, for some informal definition of 
the concept, I think that with more success for the `sealed` case. 
So going in more detail this iteration includes:


 - ClassTree::getPermitsClause now returns `List.of()`
 - Types::findDescriptorInternal has been modified to fail on sealed 
interfaces
 - several error messages have been updated as suggested, on this 
front given that when a class list the same interface twice in the 
implements clause, the second occurrence is the one flagged, I did 
the same for repeated names in the permits clause

 - modifications in ClassReader and ClassWriter as suggested
 - JavacParser, the bulk of the changes are concentrated here, as 
mentioned above the goal has been to try to implement the contextual 
keyword thing and also give a nice error message in several corner 
case situations detected in the reviews

 - more tests were added

Thanks,
Vicente

On 5/21/20 8:14 AM, Maurizio Cimadamore wrote:

Hi Vicente,
looks very good. Some comments below.

* the parser logic is clever in its use of position to apply 
context-dependent keyword detection; as Jan says, perhaps just 
share the code so that the position checks are not repeated.


* I found one very edge-case quirk in the context-dependent logic; 
not sure how we wanna address:


class Foo {
    sealed m() {}
}

This fails with:

Error: invalid method declaration; return type required

As javac parses non-sealed as a modifier, and then expects a type. 
I think this is probably reasonable, but it's not as 
context-dependent as it could be I guess.


* This case:

class Bar { }
sealed @interface Foo permits Bar

Fails as expected, but only because Bar doesn't extends Foo. I 
believe we'd like to ban sealed on annotations more eagerly. Same 
for non-sealed. For enums and records (which are non-extensible) 
the compiler does the right thing and tells me that I can't just 
use sealed/non-sealed there.


* The recovery logic in case preview features aren't enabled leaves 
something to be desired. For instance, if I compile this w/o 
--enable-preview:


 record Foo() {}

I get a very sensible error:

records are a preview feature and are disabled by default.
    (use --enable-preview to enable records)

However, if I compiler this w/o --enable-preview:

sealed class Foo {}

I get this:

Re: RFR: 8245677: Optimize lookups in empty HashMap

[Claes Redestad]

Hi Roger,

thank you for reviewing.

I felt adding the micro wouldn't add much, since HashMap is such a
common occurrence in various benchmarks already.

/Claes

On 2020-05-26 16:32, Roger Riggs wrote:

+1,

Looks fine.

Should the benchmark be included?

Thanks, Roger


On 5/25/20 8:12 AM, Jim Laskey wrote:

+1

(Copyright date)

On May 25, 2020, at 7:02 AM, Claes Redestad 
 wrote:


Hi,

sponsoring this patch from Christoph Dreis.

By pushing down the hash calculation we can optimize lookups
in empty HashMaps (also helps LinkedHashMap).

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

Testing: tier1+2

Original report: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066530.html 



/Claes

Re: RFR: 8245677: Optimize lookups in empty HashMap

[Roger Riggs]

+1,

Looks fine.

Should the benchmark be included?

Thanks, Roger


On 5/25/20 8:12 AM, Jim Laskey wrote:

+1

(Copyright date)


On May 25, 2020, at 7:02 AM, Claes Redestad  wrote:

Hi,

sponsoring this patch from Christoph Dreis.

By pushing down the hash calculation we can optimize lookups
in empty HashMaps (also helps LinkedHashMap).

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

Testing: tier1+2

Original report: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066530.html

/Claes

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Roger Riggs]

Looks good.

Thanks to Mark and you for the improvement and testing.



On 5/26/20 12:59 AM, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

This work was contributed by Mark Kralj-Taylor:

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html 



On the hotspot side we change the Linux implementations of 
javaTimeMillis() and javaTimeSystemUTC() so that they use 
clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping 
with our conditional use of this API I added a new guard


os::Posix::supports_clock_gettime()

and refined the existing supports_monotonic_clock() so that we can 
still use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the 
future (hopefully very near future) we will simply assume these APIs 
always exist.


On the core-libs side the existing test:

test/jdk/java/time/test/java/time/TestClock_System.java

is adjusted to track the precision in more detail.

Finally Mark has added instantNowAsEpochNanos() to the benchmark 
previously known as


test/micro/org/openjdk/bench/java/lang/Systems.java

which I've renamed to SystemTime.java as Mark suggested. I agree 
having these three functions measured together makes sense.


Testing:
  - test/jdk/java/time/test/java/time/TestClock_System.java
  - test/micro/org/openjdk/bench/java/lang/SystemTime.java
  - Tiers 1-3

Thanks,
David

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

[Daniel Fuchs]

Hi Alexey,

This is not a review. A few high level comments however:

For that kind of change that introduce a new environment
property you will need to file a CSR, and probably provide
some release notes as well.

Your changes seem to trigger new IllegalStateException and
UnsupportedOperationExceptions which are undocumented.
I believe they should be replaced by NamingException which
is documented at the public API level.

Also it would be good to have a test that validates that
the proposed changes works as expected.

I will not comment on the security libs changes - I'm clearly
out of my depth there. It's a bit distasteful that the
LdapCtxt/LdapClient have to have knowledge of channel binding
and extract the certificates from the SSLSocket to pass them to
the lower layer. Ideally this should rather be handled by the
SASL layers?

best regards,

-- daniel


On 25/05/2020 16:33, Alexey Bakhtin wrote:

Updated webrev is available 
at:http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Stephen Colebourne]

I'm not an official OpenJDK reviewer, nor would I be confident
reviewing the native code here.
Stephen

On Tue, 26 May 2020 at 14:28, David Holmes  wrote:
>
> On 26/05/2020 6:14 pm, Stephen Colebourne wrote:
> > AFAICT a nanosecond clock is fine from a java.time.* API perspective.
>
> Thanks Stephen. Is this a review or just a nod of approval? :)
>
> Cheers,
> David
> -
>
> > Stephen
> >
> > On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
> >>
> >> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> >> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> >>
> >> This work was contributed by Mark Kralj-Taylor:
> >>
> >> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
> >>
> >> On the hotspot side we change the Linux implementations of
> >> javaTimeMillis() and javaTimeSystemUTC() so that they use
> >> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> >> our conditional use of this API I added a new guard
> >>
> >> os::Posix::supports_clock_gettime()
> >>
> >> and refined the existing supports_monotonic_clock() so that we can still
> >> use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> >> (hopefully very near future) we will simply assume these APIs always exist.
> >>
> >> On the core-libs side the existing test:
> >>
> >> test/jdk/java/time/test/java/time/TestClock_System.java
> >>
> >> is adjusted to track the precision in more detail.
> >>
> >> Finally Mark has added instantNowAsEpochNanos() to the benchmark
> >> previously known as
> >>
> >> test/micro/org/openjdk/bench/java/lang/Systems.java
> >>
> >> which I've renamed to SystemTime.java as Mark suggested. I agree having
> >> these three functions measured together makes sense.
> >>
> >> Testing:
> >> - test/jdk/java/time/test/java/time/TestClock_System.java
> >> - test/micro/org/openjdk/bench/java/lang/SystemTime.java
> >> - Tiers 1-3
> >>
> >> Thanks,
> >> David

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[David Holmes]

Hi Vyom,

Thanks for looking at this.

On 26/05/2020 6:44 pm, Vyom Tiwari wrote:

Hi David,

we can remove the redundant local variable(jlong result) from if block 
as follows.


return jlong(ts.tv_sec) * MILLIUNITS +  jlong(ts.tv_nsec) / 
NANOUNITS_PER_MILLIUNIT;


Sure. I copied the code from os::javaTimeNanos. IIRC I introduced the 
local to allow adding a printf before the return when debugging the 
conversion :) I should have removed it again.


Thanks,
David


Vyom


On Tue, May 26, 2020 at 10:29 AM David Holmes > wrote:


bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

This work was contributed by Mark Kralj-Taylor:


https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html

On the hotspot side we change the Linux implementations of
javaTimeMillis() and javaTimeSystemUTC() so that they use
clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping
with
our conditional use of this API I added a new guard

os::Posix::supports_clock_gettime()

and refined the existing supports_monotonic_clock() so that we can
still
use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
(hopefully very near future) we will simply assume these APIs always
exist.

On the core-libs side the existing test:

test/jdk/java/time/test/java/time/TestClock_System.java

is adjusted to track the precision in more detail.

Finally Mark has added instantNowAsEpochNanos() to the benchmark
previously known as

test/micro/org/openjdk/bench/java/lang/Systems.java

which I've renamed to SystemTime.java as Mark suggested. I agree having
these three functions measured together makes sense.

Testing:
    - test/jdk/java/time/test/java/time/TestClock_System.java
    - test/micro/org/openjdk/bench/java/lang/SystemTime.java
    - Tiers 1-3

Thanks,
David



--
Thanks,
Vyom

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[David Holmes]

On 26/05/2020 6:14 pm, Stephen Colebourne wrote:

AFAICT a nanosecond clock is fine from a java.time.* API perspective.


Thanks Stephen. Is this a review or just a nod of approval? :)

Cheers,
David
-


Stephen

On Tue, 26 May 2020 at 06:01, David Holmes  wrote:


bug: https://bugs.openjdk.java.net/browse/JDK-8242504
webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/

This work was contributed by Mark Kralj-Taylor:

https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html

On the hotspot side we change the Linux implementations of
javaTimeMillis() and javaTimeSystemUTC() so that they use
clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
our conditional use of this API I added a new guard

os::Posix::supports_clock_gettime()

and refined the existing supports_monotonic_clock() so that we can still
use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
(hopefully very near future) we will simply assume these APIs always exist.

On the core-libs side the existing test:

test/jdk/java/time/test/java/time/TestClock_System.java

is adjusted to track the precision in more detail.

Finally Mark has added instantNowAsEpochNanos() to the benchmark
previously known as

test/micro/org/openjdk/bench/java/lang/Systems.java

which I've renamed to SystemTime.java as Mark suggested. I agree having
these three functions measured together makes sense.

Testing:
- test/jdk/java/time/test/java/time/TestClock_System.java
- test/micro/org/openjdk/bench/java/lang/SystemTime.java
- Tiers 1-3

Thanks,
David

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

[Alexey Bakhtin]

Hi Michael, Thomas,

Unfortunately we can not fix address type issue with the UnspecEmptyInetAddress 
subclass.
We can not create subclass of InetAddress without changing public API.
We can try similar approach but create subclass of ChannelBinding class 
instead. It is not so elegant like UnspecEmptyInetAddress approach but it could 
help to distinguish between TLS channel binding and Channel Bindings set by 
application.
Later we can remove this workaround In case we prove that UNSPEC type should be 
used in all types of Channel Bindings.

Updated webrev : http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v2/

Regards
Alexey

> On 26 May 2020, at 06:04, Thomas Maslen  wrote:
> 
> On Mon, May 25, 2020 at 3:15 AM Michael Osipov <1983-01...@gmx.net> wrote:
> [...]
> So I read the discussions. RFC 2744 shall not be changed, people
> admitted that the spec of SASL GS2 mechs is wrong and should be changed,
> but this has not happened yet. It remained at UNSPEC all the years.
> 
> So we have several issues here:
> * GSS-API C bindings and SASL requests are two distinct RFCs which
> require/mandate differnt things.
> * The change in JGSS in unrelated to this patch because GSS-API knows
> nothing about SASL and its fauly spec.
> 
> Since we are doing LDAP over SASL here and RFC 5801 requires to be
> UNSPEC (0) the SASL TlsChannelBinding class must take that into account.
> Unfortunately, JGSS implies with the args of the ChannelBinding the type
> fo the adress. So will change my opinion a bit:
> 
> No property for AD/non-AD is necessary, but handling of UNSPEC is
> required. JGSS shall remain at NULLADDR. The subtype
> UnspecEmptyInetAddress should be at least evaluated.
> 
> Michael
> 
>  No.  This isn't just about RFC 5801.  As Alexey Bakhtin observed, this also 
> applies to channel bindings for HTTP Negotiate Authentication (loosely aka 
> "SPNEGO"), not only for NTLM (which probably isn't at issue here) but also 
> for Kerberos -- that's where I first encountered this, working on a 
> proprietary Java Kerberos implementation.
> 
> More generally, if you want channel bindings to interoperate in the GSSAPI 
> Kerberos Mechanism for any protocol -- SASL GS2, HTTP Negotiate 
> Authentication, or anything else -- ignore the fact that RFC 2744 specifies 
> 255 for the "no address" case and do what everyone actually does:  use zero.
> 
> Here is a test from MIT Kerberos that (implicitly) uses zero:  
> https://github.com/krb5/krb5/blob/master/src/tests/gssapi/t_bindings.c
> 
> And here is one from Heimdal:  
> https://github.com/heimdal/heimdal/blob/5057d04f6a47f05f1ed7c617458722104d4c17dc/lib/gssapi/test_context.c

RFR: JDK-8245202: Convert existing jpackage tests to newer form.

[Andy Herrick]

Please review the fix to issue [1] at [2].

The change removes all the old form helper tests and converts the 6 
remaining tests that used the old helpers into 4 new tests using the new 
helpers.


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

[2] - http://cr.openjdk.java.net/~herrick/8245202/webrev.01/

/Andy

Re: Possible optimization in StringLatin1.regionMatchesCI

[Christoph Dreis]

Hi Martin,

> Not a review, but:
> Compare with the variant of this code in StringUTF16.
> StringLatin1 only ever needs to support the first 256 chars in Unicode

Does it really? That makes me wonder even more about the additional lowercase 
check.

> which can never change, unlike StringUTF16,

What do you mean by "can never change"?

> Do all the String tests still pass if you simplify the code?

You mean if I remove the additional lowercasing completely!?

--- a/src/java.base/share/classes/java/lang/StringLatin1.java   Tue May 26 
09:25:23 2020 +0200
+++ b/src/java.base/share/classes/java/lang/StringLatin1.java   Tue May 26 
13:01:14 2020 +0200
@@ -396,9 +396,6 @@
 if (u1 == u2) {
 continue;
 }
-if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
-continue;
-}
 return false;
 }
 return true;

Tier1 seems to pass still (apart from some tests that don't seem to like my 
German setup - also without the change)

> Should CharacterDataLatin1 have a method to compare two characters
> case-insensitively?

What do you mean by that? It needs to keep at least one check for the 
regionMatchesCI method.

> Be careful with Latin Small Letter sharp S
That seems to stay 223 regardless of uppercasing or lowercasing it.

I'm afraid your answer raised more question marks than it actually solved. ;-)

Cheers,
Christoph

>On Mon, May 25, 2020 at 2:16 PM Christoph Dreis
> wrote:
>>
>> Hi,
>>
>> I've recently looked through the StringLatin1 code - specifically 
>> regionMatchesCI.
>>
>> I think I have an optimization, but would need someone with more domain 
>> knowledge to verify if I'm missing nothing.
>>
>> Currently, the code does a conversion to uppercase and if that doesn't match 
>> it does an additional comparison of the lowercase characters.
>> What's confusing to me is that there are actually both upper- and lowercase 
>> checks needed, but that might be explained by the comment in the UTF-16 
>> version about the Georgian alphabet.
>>
>> Assuming that the additional lowercase check is needed, I was wondering if 
>> this must be on the uppercase variant. Wouldn't it be faster on the 
>> character itself to avoid potentially converting a lowercase character to an 
>> uppercase character and back?
>>
>> I think code is actually better explaining what I'm suggesting:
>>
>> --- a/src/java.base/share/classes/java/lang/StringLatin1.java   Wed May 13 
>> 16:18:16 2020 +0200
>> +++ b/src/java.base/share/classes/java/lang/StringLatin1.java   Mon May 25 
>> 22:59:13 2020 +0200
>> @@ -396,7 +396,7 @@
>>  if (u1 == u2) {
>>  continue;
>>  }
>> -if (Character.toLowerCase(u1) == Character.toLowerCase(u2)) {
>> +if (Character.toLowerCase(c1) == Character.toLowerCase(c2)) {
>>  continue;
>>  }
>>  return false;
>>
>>
>> And indeed the newer version seems to be faster if I use the following 
>> benchmark:
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class MyBenchmark {
>>
>> @State(Scope.Benchmark)
>> public static class ThreadState {
>> private String test1 = "test";
>> private String test2 = "best";
>> }
>>
>> @Benchmark
>> public boolean test(ThreadState threadState) {
>> return threadState.test1.equalsIgnoreCase(threadState.test2);
>> }
>>
>> }
>>
>> Benchmark  Mode  Cnt   ScoreError   
>> Units
>> MyBenchmark.testOld  avgt   10   8,843 ±  0,274   ns/op
>> MyBenchmark.testPatched  avgt   10   7,067 ±  0,177   ns/op
>>
>> Does this make sense? Do I miss something here? I would appreciate if 
>> someone can either explain the shortcomings of the solution above or - in 
>> case there aren't any - can maybe sponsor it.
>>
>> Cheers,
>> Christoph
>>
>>

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Vyom Tiwari]

Hi David,

we can remove the redundant local variable(jlong result) from if block as
follows.

return jlong(ts.tv_sec) * MILLIUNITS + jlong(ts.tv_nsec) /
NANOUNITS_PER_MILLIUNIT;

Vyom


On Tue, May 26, 2020 at 10:29 AM David Holmes 
wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
>
> This work was contributed by Mark Kralj-Taylor:
>
>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
>
> On the hotspot side we change the Linux implementations of
> javaTimeMillis() and javaTimeSystemUTC() so that they use
> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> our conditional use of this API I added a new guard
>
> os::Posix::supports_clock_gettime()
>
> and refined the existing supports_monotonic_clock() so that we can still
> use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> (hopefully very near future) we will simply assume these APIs always exist.
>
> On the core-libs side the existing test:
>
> test/jdk/java/time/test/java/time/TestClock_System.java
>
> is adjusted to track the precision in more detail.
>
> Finally Mark has added instantNowAsEpochNanos() to the benchmark
> previously known as
>
> test/micro/org/openjdk/bench/java/lang/Systems.java
>
> which I've renamed to SystemTime.java as Mark suggested. I agree having
> these three functions measured together makes sense.
>
> Testing:
>- test/jdk/java/time/test/java/time/TestClock_System.java
>- test/micro/org/openjdk/bench/java/lang/SystemTime.java
>- Tiers 1-3
>
> Thanks,
> David
>


-- 
Thanks,
Vyom

Re: [aarch64-port-dev ] RFR (XXL): 8223347: Integration of Vector API (Incubator): AArch64 backend changes

[Andrew Haley]

On 25/05/2020 09:26, Yang Zhang wrote:
> In jdk master, what we need to do is that writing m4 file for existing
> vector instructions and placed them to a new file aarch64_neon.ad.
> If no question, I will do it right away.

I'm not entirely sure that such a change is necessary now. In particular,
reorganizing the existing vector instructions is IMO excessive, but I
admit that it might be an improvement.

But to my earlier question. please: can the new instructions be moved
into jdk head first, and then merged into the Panama branch, or not?
It'd help if this was possible.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Stephen Colebourne]

AFAICT a nanosecond clock is fine from a java.time.* API perspective.
Stephen

On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
>
> This work was contributed by Mark Kralj-Taylor:
>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
>
> On the hotspot side we change the Linux implementations of
> javaTimeMillis() and javaTimeSystemUTC() so that they use
> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> our conditional use of this API I added a new guard
>
> os::Posix::supports_clock_gettime()
>
> and refined the existing supports_monotonic_clock() so that we can still
> use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> (hopefully very near future) we will simply assume these APIs always exist.
>
> On the core-libs side the existing test:
>
> test/jdk/java/time/test/java/time/TestClock_System.java
>
> is adjusted to track the precision in more detail.
>
> Finally Mark has added instantNowAsEpochNanos() to the benchmark
> previously known as
>
> test/micro/org/openjdk/bench/java/lang/Systems.java
>
> which I've renamed to SystemTime.java as Mark suggested. I agree having
> these three functions measured together makes sense.
>
> Testing:
>- test/jdk/java/time/test/java/time/TestClock_System.java
>- test/micro/org/openjdk/bench/java/lang/SystemTime.java
>- Tiers 1-3
>
> Thanks,
> David

Re: RFR (XS) 8245722: 32-bit build failures after JDK-8243491

[Aleksey Shipilev]

On 5/25/20 6:51 PM, Aleksey Shipilev wrote:
> Thanks!
> 
> Maurizio, want to ack this too?
Nevermind, I pushed with Thomas' review and triviality rule.

-- 
Thanks,
-Aleksey