Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]

[Chris Hegarty]

On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess  wrote:

>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update LastModified

LGTM.

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7928

Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v12]

[Chris Hegarty]

On Wed, 3 Nov 2021 13:23:36 GMT, Aleksei Efimov  wrote:

>> This change implements a new service provider interface for host name and 
>> address resolution, so that java.net.InetAddress API can make use of 
>> resolvers other than the platform's built-in resolver.
>> 
>> The following API classes are added to `java.net.spi` package to facilitate 
>> this:
>> - `InetAddressResolverProvider` -  abstract class defining a service, and 
>> is, essentially, a factory for `InetAddressResolver` resolvers.
>> - `InetAddressResolverProvider.Configuration ` - an interface describing the 
>> platform's built-in configuration for resolution operations that could be 
>> used to bootstrap a resolver construction, or to implement partial 
>> delegation of lookup operations.
>> - `InetAddressResolver` - an interface that defines methods for the 
>> fundamental forward and reverse lookup operations.
>> - `InetAddressResolver.LookupPolicy` - a class whose instances describe the 
>> characteristics of one forward lookup operation.  
>> 
>> More details in [JEP-418](https://openjdk.java.net/jeps/418).
>> 
>> Testing: new and existing `tier1:tier3` tests
>
> Aleksei Efimov has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 18 commits:
> 
>  - Merge branch 'master' into JDK-8244202_JEP418_impl
>  - Merge branch 'master' into JDK-8244202_JEP418_impl
>  - Replace 'system' with 'the system-wide', move comment section
>  - Add @ throws NPE to hosts file resolver javadoc
>  - Changes to address review comments
>  - Update tests to address SM deprecation
>  - Merge branch 'master' into JDK-8244202_JEP418_impl
>  - More javadoc updates to API classes
>  - Review updates + move resolver docs to the provider class (CSR update to 
> follow)
>  - Change InetAddressResolver method names
>  - ... and 8 more: 
> https://git.openjdk.java.net/jdk/compare/a316c06e...0542df51

LGTM.

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5822

Re: [jdk17] RFR: 8266345: (fs) Custom DefaultFileSystemProvider security related loops

[Chris Hegarty]

On Thu, 8 Jul 2021 18:21:31 GMT, Sean Mullan  wrote:

> Please review this fix to use the platform's default file system to avoid 
> recursive policy initialization
> issues when a SecurityManager is enabled and the VM is configured to use a 
> custom file system provider.

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk17/pull/232

Re: RFR: 8267990: Revisit some uses of `synchronized` in the HttpClient API

[Chris Hegarty]

On Mon, 31 May 2021 16:21:29 GMT, Daniel Fuchs  wrote:

> The Utils.remaining(List list) method assumes that it can and 
> should synchronize on the given list to prevent concurrent modification. In 
> 99% of the cases this assumption is wrong. There's only one such list (the 
> SSLFlowDelegate writeList) that requires this synchronization. 
> 
> Also the `SequentialScheduler.synchronizedScheduler` uses `synchronized`, it 
> could use a Lock instead and this would make it possible to assert that there 
> is no contention (since the logic of the SequentialScheduler is supposed to 
> prevent contention from occurring at this place).

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4275

RFR: 8267938: SCTP channel factory methods should check platform support

[Chris Hegarty]

The SCTP channel factory methods, namely SctpChannel::open, 
SctpServerChannel::open, and SctpMultiChannel::open, are specified to throw 
UnsupportedOperationException, if the SCTP protocol is not supported. 
Currently, underlying platform support is assumed once the appropriate 
libsctp.so.1 library is present (along with its supported interface functions). 
This may not always be the case, e.g. if the Linux sctp kernel module is not 
present or loaded. In which case a SocketException is thrown.

It would be more appropriate to check for EPROTONOSUPPORT and ESOCKTNOSUPPORT, 
and throw UOE rather than SE.

The existing java/net/SctpSanity.java tests already covers this case, when run 
on platforms without support.

-

Commit messages:
 - Initial changes

Changes: https://git.openjdk.java.net/jdk/pull/4246/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4246&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267938
  Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4246.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4246/head:pull/4246

PR: https://git.openjdk.java.net/jdk/pull/4246

Re: RFR: 8267587: Update java.util to use enhanced switch [v5]

[Chris Hegarty]

On Tue, 25 May 2021 11:49:18 GMT, Tagir F. Valeev  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> Tagir F. Valeev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   More vertical alignment

src/java.base/share/classes/java/util/Calendar.java line 1507:

> 1505: }
> 1506: case "japanese" -> cal = new 
> JapaneseImperialCalendar(zone, locale, true);
> 1507: default -> throw new IllegalArgumentException("unknown 
> calendar type: " + type);

In this case, it would be good to yield the result of the switch expression and 
assign that to a local, rather than assigning in each of the case arms, e.g:

final Calendar cal = switch (type) {
case "gregory" -> new GregorianCalendar(zone, locale, true);
case "iso8601" -> {
GregorianCalendar gcal = new GregorianCalendar(zone, locale, true);
// make gcal a proleptic Gregorian
gcal.setGregorianChange(new Date(Long.MIN_VALUE));
// and week definition to be compatible with ISO 8601
setWeekDefinition(MONDAY, 4);
yield gcal;
}
case "buddhist" -> {
var buddhistCalendar = new BuddhistCalendar(zone, locale);
buddhistCalendar.clear();
yield buddhistCalendar;
}
case "japanese" -> new JapaneseImperialCalendar(zone, locale, true);
default -> throw new IllegalArgumentException("unknown calendar type: " 
+ type);
};

-

PR: https://git.openjdk.java.net/jdk/pull/4161

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Chris Hegarty]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  wrote:

> Please review this implementation of [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> The code change is divided into 3 commits. Please review them one by one.
> 
> 1. 
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>  The essential change for this JEP, including the `@Deprecate` annotations 
> and spec change. It also update the default value of the 
> `java.security.manager` system property to "disallow", and necessary test 
> change following this update.
> 2. 
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>  Manual changes to several files so that the next commit can be generated 
> programatically.
> 3. 
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>  Automatic changes to other source files to avoid javac warnings on 
> deprecation for removal
> 
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
> generated programmatically, see the comment below for more details. If you 
> are only interested in a portion of the 3rd commit and would like to review 
> it as a separate file, please comment here and I'll generate an individual 
> webrev.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> any file to minimize unnecessary merge conflict.
> 
> Furthermore, since the default value of `java.security.manager` system 
> property is now "disallow", most of the tests calling 
> `System.setSecurityManager()` need to launched with 
> `-Djava.security.manager=allow`. This is covered in a different PR at 
> https://github.com/openjdk/jdk/pull/4071.

The changes in the net area look fine.

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

[Chris Hegarty]

On Wed, 28 Apr 2021 10:42:54 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address first batch of review comments

Like Paul, I tracked the changes to this API in the Panama repo. Most of my 
remaining comments are small and come from re-reading the API docs.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 
270:

> 268: 
> 269: /**
> 270:  * Converts a Java string into a null-terminated C string, using the 
> platform's default charset,

Sorry if this has come up before, but, is the platform's default charset the 
right choice here? For other areas, we choose UTF-8 as the default. In fact, 
there is a draft JEP to move the default charset to UTF-8. So if there is an 
implicit need to match the underlying platform's charset then this may need to 
be revisited.  For now, I just want to check that this is not an accidental 
reliance on the platform's default charset, but a deliberate one.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 101:

> 99:  * Lifecycle and confinement
> 100:  *
> 101:  * Memory segments are associated to a resource scope (see {@link 
> ResourceScope}), which can be accessed using

Typo?? "Memory segments are associated *with* a resource scope"

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 112:

> 110: MemoryAccess.getLong(segment); // already closed!
> 111:  * }
> 112:  * Additionally, access to a memory segment in subject to the 
> thread-confinement checks enforced by the owning scope; that is,

Typo?? "Additionally, access to a memory segment *is* subject to ..."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 114:

> 112:  * Additionally, access to a memory segment in subject to the 
> thread-confinement checks enforced by the owning scope; that is,
> 113:  * if the segment is associated with a shared scope, it can be accessed 
> by multiple threads; if it is associated with a confined
> 114:  * scope, it can only be accessed by the thread which own the scope.

Typo?? "it can only be accessed by the thread which ownS the scope."

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
 line 693:

> 691:  */
> 692: static MemorySegment allocateNative(MemoryLayout layout, 
> ResourceScope scope) {
> 693: Objects.requireNonNull(scope);

Should the allocateNative methods declare that they throw ISE, if the given 
ResourceScope is not alive?   ( I found myself asking this q, then considering 
the behaviour of a SegmentAllocator that is asked to allocate after a RS has 
been closed )

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3699

Re: JEP411: Missing use-case: Monitoring / restricting libraries

[Chris Hegarty]

On 28 Apr 2021, at 11:38, Markus Gronlund 
mailto:markus.gronl...@oracle.com>> wrote:

Hi Lim,

JFR specific feedback can be posted to: 
hotspot-jfr-...@openjdk.java.net

Thanks Markus. That is the appropriate list to send JFR feedback.

Just to add, I filed an Enhancement Request in JIRA, 8265962: "Evaluate adding 
Networking JFR events” [1], to track the possibility of adding JFR events to 
the JDK libraries that perform low-level networking activity (which is mostly 
in the purview of the networking and libraries area). If we had such, then it 
would be possible to monitor *all* low-level network activity performed by the 
platform, regardless of which higher-level library is performing the activity. 
Clearly such would not capture URLs, but rather the network activity that would 
be triggered by, say, an HTTP Client library. This seems like a more fruitful 
and uniform approach, rather than trying to add JFR events to, say, every HTTP 
library.

-Chris.

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


Thanks
Markus

-Original Message-
From: Lim 
mailto:lim.chainz11+mail...@gmail.com>>
Sent: den 28 april 2021 12:18
To: Markus Gronlund 
mailto:markus.gronl...@oracle.com>>
Cc: Ron Pressler mailto:ron.press...@oracle.com>>; 
security-dev@openjdk.java.net
Subject: Re: JEP411: Missing use-case: Monitoring / restricting libraries

Hi Markus, thank you for giving me the guidance for performing the JFR 
programmatically.
I am able to test if my use case is suitable. Where do I provide my 
feedback/issue of using the streamed JFR?

On Wed, Apr 21, 2021 at 10:32 PM Markus Gronlund 
mailto:markus.gronl...@oracle.com>> wrote:
If the existing event probes in the JDK does not give you the information you 
need, like the name of URL's, it can be a reached by building your own "custom 
events" via the Events API [3]. It can be harder to add events to unknown code 
dynamically, but it can be done and you can use java.lang.Instrument to build 
an agent to inject the custom event.

I understand that new events can be added in code that I have control of using 
the Events API but in this case, which is the name of URLs is not feasible.

Firstly, using a Java agent to instrument bytecode cannot be scaled because 
there are a lot of HTTP libraries, including the built in Java APIs and 3rd 
parties such as Apache HTTP, OkHttp. They can also roll their own "HTTP 
wrapper" if the author doesn't want dependency. In addition, these 3rd party 
libraries can be shaded and relocated, making it harder to target via 
instrumentation.

Obfuscation can also have an impact on reliability of instrumentation, since 
obfuscation can be changed in every version and what if the obfuscation has 
"anti-tamper/anti-debug" features? This is not scalable if we need to monitor 
for each library that might call URLs.

If there is a general problem area and provides a good scaling factor, and the 
URL information might just be such a case, it can make sense to investigate if 
this information can be provided directly by the JDK, by extending existing or 
new JFR events.

I believe that the majority of the HTTP libraries, and code that roll their own 
are using the built in Java APIs, thus monitoring the built in API that is used 
for making URLs calls make sense. Then, it can be scaled to most of the 
libraries compared to instrumenting each one which has its own problem stated 
above.

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator) [v2]

[Chris Hegarty]

On Wed, 28 Apr 2021 09:06:29 GMT, Chris Hegarty  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address first batch of review comments
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
>  line 205:
> 
>> 203:  * until all the resource scope handles acquired from it have been 
>> {@link #release(Handle)} released}.
>> 204:  * @return a resource scope handle.
>> 205:  */
> 
> Given recent changes, it might be good to generalise the opening sentence 
> here. Maybe :
>  " Acquires a resource scope handle associated with this resource scope. If 
> the resource scope is explicit ... "   Or some such.

The spec for the acquire method talks only of explicit resource scopes. The 
comment is suggesting that the spec could be generalised a little, since it is 
possible to acquire a resource scope handle associated with implicit scopes.

-

PR: https://git.openjdk.java.net/jdk/pull/3699

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator)

[Chris Hegarty]

On Mon, 26 Apr 2021 17:10:13 GMT, Maurizio Cimadamore  
wrote:

> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
 line 135:

> 133: } finally {
> 134:segment.scope().release(segmentHandle);
> 135: }

I do like the symmetry in this example, but just to add an alternative idiom:
`segmentHandle.scope().release(segmentHandle)`
, which guarantees to avoid release throwing and IAE

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
 line 186:

> 184:  * this resource scope is confined, and this method is 
> called from a thread other than the thread owning this resource scope
> 185:  * this resource scope is shared and a resource associated 
> with this scope is accessed while this method is called
> 186:  * one or more handles (see {@link #acquire()}) associated 
> with this resource scope have not been closed

Minor spec suggestion: "... associated with this resource scope have not been 
*released*"

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ResourceScope.java
 line 205:

> 203:  * until all the resource scope handles acquired from it have been 
> {@link #release(Handle)} released}.
> 204:  * @return a resource scope handle.
> 205:  */

Given recent changes, it might be good to generalise the opening sentence here. 
Maybe :
 " Acquires a resource scope handle associated with this resource scope. If the 
resource scope is explicit ... "   Or some such.

-

PR: https://git.openjdk.java.net/jdk/pull/3699

Re: RFR: 8264774: Implementation of Foreign Function and Memory API (Incubator)

[Chris Hegarty]

On Tue, 27 Apr 2021 18:40:24 GMT, Alan Bateman  wrote:

>> This PR contains the API and implementation changes for JEP-412 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/412
>
> src/java.base/share/classes/sun/nio/ch/IOUtil.java line 466:
> 
>> 464: }
>> 465: 
>> 466: private static final JavaNioAccess NIO_ACCESS = 
>> SharedSecrets.getJavaNioAccess();
> 
> It might be cleaner to move to acquire/release methods to their own 
> supporting class as it's not really IOUtil.

I went back and forth on this a number of times already. I think where we 
landed is a reasonable place, given the current shape of the code.

Scope is a private property of Buffer, and as such should be consulted anywhere 
where a buffer's address is being accessed. In fact, a prior prototype would 
not allow access to the underlying address value without the caller passing a 
valid handle for the buffer view's scope. It's hard to find the sweet-spot here 
between code reuse and safety, but the high-order bit is that the code 
accessing the address is auditable and testable to avoid accessing memory 
unsafely. Maybe there is a better alternative implementation code structure (at 
the cost of some duplication), but it is not obvious to me what that is (and I 
have given it some thought). Suggestions welcome.

Note, there is a little more follow-on work to be done in this area, if we are 
to expand support to other non-TCP channel implementations. Maybe investigation 
into possible code refactorings could be done as part of that?

-

PR: https://git.openjdk.java.net/jdk/pull/3699

Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]

[Chris Hegarty]

On Mon, 12 Apr 2021 14:09:55 GMT, Conor Cleary  wrote:

>> ### Description
>> This fix is part of a previous effort to both cleanup/modernise JNDI code, 
>> the details of which can be seen in 
>> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number 
>> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where 
>> only a single object unique to the requirements of the method is used. The 
>> issues these occurrences of AICs cause are highlighted below.
>> 
>> - AICs, in the cases concerned with this fix, are used where only one 
>> operation is required. While AICs can be useful for more complex 
>> implementations (using interfaces, multiple methods needed, local fields 
>> etc.), Lambdas are better suited here as they result in a more readable and 
>> concise solution.
>> 
>> ### Fixes
>> - Where applicable, occurrences of AICs were replaced with lambdas to 
>> address the issues above resulting primarily in more readable/concise code.
>
> Conor Cleary has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update copyright headers
>  - Tidied up lambdas

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3416

Re: RFR: 8263754: HexFormat 'fromHex' methods should be static

[Chris Hegarty]

On Thu, 25 Mar 2021 20:08:14 GMT, Roger Riggs  wrote:

> A number of HexFormat methods converting from strings to numbers do not use 
> delimiter, prefix, suffix, and uppercase parameters and would be more 
> convenient if the methods were static.
> 
> These  APIs were added early in JDK 17 and are being updated before GA.
> This PR updates existing uses in the JDK but there may be compiler warnings 
> in non-JDK source files.
> 
>public boolean isHexDigit(int);
>public int fromHexDigit(int);
>public int fromHexDigits(java.lang.CharSequence);
>public int fromHexDigits(java.lang.CharSequence, int, int);
>public long fromHexDigitsToLong(java.lang.CharSequence);
>public long fromHexDigitsToLong(java.lang.CharSequence, int, int);

Marked as reviewed by chegar (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3205

Re: RFR: 8148937: (str) Adapt StringJoiner for Compact Strings [v3]

[Chris Hegarty]

On Mon, 15 Mar 2021 21:47:28 GMT, Сергей Цыпанов 
 wrote:

>> Hello,
>> 
>> as of now `java.util.StringJoiner` still uses `char[]` as a storage for 
>> joined Strings.
>> 
>> This applies for the cases when all joined Strings as well as delimiter, 
>> prefix and suffix contain only ASCII symbols.
>> 
>> As a result when `StringJoiner.toString()` is called `byte[]` stored in 
>> Strings is inflated in order to fill in `char[]` and after that `char[]` is 
>> compressed when constructor of String is called:
>> String delimiter = this.delimiter;
>> char[] chars = new char[this.len + addLen];
>> int k = getChars(this.prefix, chars, 0);
>> if (size > 0) {
>> k += getChars(elts[0], chars, k);// inflate byte[]
>> 
>> for(int i = 1; i < size; ++i) {
>> k += getChars(delimiter, chars, k);
>> k += getChars(elts[i], chars, k);
>> }
>> }
>> 
>> k += getChars(this.suffix, chars, k);
>> return new String(chars);// compress char[] -> byte[]
>> This can be improved by utilizing new method `String.getBytes(byte[], int, 
>> int, byte, int)` [introduced](https://github.com/openjdk/jdk/pull/402) in 
>> [JDK-8224986](https://bugs.openjdk.java.net/browse/JDK-8254082)
>> covering both cases when resulting String is Latin1 or UTF-16
>> 
>> I've prepared a patch along with benchmark proving that this change is 
>> correct and brings improvement.
>> 
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class StringJoinerBenchmark {
>> 
>>   @Benchmark
>>   public String stringJoiner(Data data) {
>> String[] stringArray = data.stringArray;
>> return Joiner.joinWithStringJoiner(stringArray);
>>   }
>> 
>>   @State(Scope.Thread)
>>   public static class Data {
>> 
>> @Param({"latin", "cyrillic", "mixed"})
>> private String mode;
>> 
>> @Param({"8", "32", "64"})
>> private int length;
>> 
>> @Param({"5", "10", "100"})
>> private int count;
>> 
>> private String[] stringArray;
>> 
>> @Setup
>> public void setup() {
>>   RandomStringGenerator generator = new RandomStringGenerator();
>> 
>>   stringArray = new String[count];
>> 
>>   for (int i = 0; i < count; i++) {
>> String alphabet = getAlphabet(i, mode);
>> stringArray[i] = generator.randomString(alphabet, length);
>>   }
>> }
>> 
>> private static String getAlphabet(int index, String mode) {
>>   var latin = "abcdefghijklmnopqrstuvwxyz"; //English
>>   var cyrillic = "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>> 
>>   String alphabet;
>>   switch (mode) {
>> case "mixed" -> alphabet = index % 2 == 0 ? cyrillic : latin;
>> case "latin" -> alphabet = latin;
>> case "cyrillic" -> alphabet = cyrillic;
>> default -> throw new RuntimeException("Illegal mode " + mode);
>>   }
>>   return alphabet;
>> }
>>   }
>> }
>> 
>> public class Joiner {
>> 
>>   public static String joinWithStringJoiner(String[] stringArray) {
>> StringJoiner joiner = new StringJoiner(",", "[", "]");
>> for (String str : stringArray) {
>>   joiner.add(str);
>> }
>> return joiner.toString();
>>   }
>> }
>> 
>> 
>> (count)  (length)(mode)  
>>   Java 14 patched Units
>> stringJoiner  5 8 latin   78.836 
>> ±   0.20867.546 ±   0.500 ns/op
>> stringJoiner  532 latin   92.877 
>> ±   0.42266.760 ±   0.498 ns/op
>> stringJoiner  564 latin  115.423 
>> ±   0.88373.224 ±   0.289 ns/op
>> stringJoiner 10 8 latin  152.587 
>> ±   0.429   161.427 ±   0.635 ns/op
>> stringJoiner 1032 latin  189.998 
>> ±   0.478   164.099 ±   0.963 ns/op
>> stringJoiner 1064 latin  238.679 
>> ±   1.419   176.825 ±   0.533 ns/op
>> stringJoiner100 8 latin 1215.612 
>> ±  17.413  1541.802 ± 126.166 ns/op
>> stringJoiner10032 latin 1699.998 
>> ±  28.407  1563.341 ±   4.439 ns/op
>> stringJoiner10064 latin 2289.388 
>> ±  45.319  2215.931 ± 137.583 ns/op
>> stringJoiner  5 8  cyrillic   96.692 
>> ±   0.94780.946 ±   0.371 ns/op
>> stringJoiner  532  cyrillic  107.806 
>> ±   0.42984.717 ±   0.541 ns/op
>> stringJoiner  564  cyrillic  150.762 
>> ±   2.26796.214 ±   1.251 ns/op
>> stringJoiner 10 8  cyrillic  190.57

Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]

[Chris Hegarty]

On Wed, 17 Feb 2021 16:38:03 GMT, Сергей Цыпанов 
 wrote:

>> Non-static classes hold a link to their parent classes, which in many cases 
>> can be avoided.
>
> Сергей Цыпанов has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8261880: Remove static from declarations of Holder nested classes

The changes in java/net look ok to me.

-

PR: https://git.openjdk.java.net/jdk/pull/2589

Re: RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks

[Chris Hegarty]

On Wed, 17 Feb 2021 10:30:45 GMT, Chris Hegarty  wrote:

>> In another bug  this question from me  was answered by  Alan Bateman :
>> 
>> Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I 
>> started to wonder what happens to the allocated memory in the same file in 
>> handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early 
>> return cases incl. the CHECK_NULL , is there some deallocation missing there 
>> too ?
>> 
>> 
>> Yes, the error paths in handleSendFailed should be looked at. If 
>> NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. 
>> Furthermore, if the NewObject fails and bufferObj != NULL then the memory 
>> for the direct buffer will need to be freed too (as JNI NewDirectByteBuffer 
>> does not setup a cleaner).
>> 
>> 
>> So I added freeing of the malloced memory to handleSendFailed .
>> Please review !
>> 
>> Thanks, Matthias
>
> Marked as reviewed by chegar (Reviewer).

This changes look good to me.  Additionally, and separately, I have filed 
JDK-8261881.

>From 8261881 : The SCTP implementation uses NewDirectByteBuffer to create a 
>new direct byte-buffer in native code. Such creations of a direct byte-buffer 
>are not automatically associated with a Cleaner, therefore the native memory 
>associated with the buffer may not be released when the buffer is no longer 
>reachable. 

The Java code that results in the native NewDirectByteBuffer should be examined 
with a view to creating a cleaner and assigning it to the direct byte-buffer.

-

PR: https://git.openjdk.java.net/jdk/pull/2586

Re: RFR: JDK-8261791: handleSendFailed in SctpChannelImpl.c potential leaks

[Chris Hegarty]

On Tue, 16 Feb 2021 12:26:54 GMT, Matthias Baesken  wrote:

> In another bug  this question from me  was answered by  Alan Bateman :
> 
> Btw. while adjusting Java_sun_nio_ch_sctp_SctpChannelImpl_receive0 , I 
> started to wonder what happens to the allocated memory in the same file in 
> handleSendFailed ( if ((addressP = malloc(dataLength)) == NULL) ) in early 
> return cases incl. the CHECK_NULL , is there some deallocation missing there 
> too ?
> 
> 
> Yes, the error paths in handleSendFailed should be looked at. If 
> NewDirectByteBuffer or recvmsg fails then addressP needs to be freed. 
> Furthermore, if the NewObject fails and bufferObj != NULL then the memory for 
> the direct buffer will need to be freed too (as JNI NewDirectByteBuffer does 
> not setup a cleaner).
> 
> 
> So I added freeing of the malloced memory to handleSendFailed .
> Please review !
> 
> Thanks, Matthias

Marked as reviewed by chegar (Reviewer).

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 236:

> 234: return;
> 235: }
> 236: 

This looks fine. If NewDirectByteBuffer returns NULL there will be a pending 
OutOfMemoryError.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 251:

> 249: free(addressP);
> 250: handleSocketError(env, errno);
> 251: return;

At this point the direct ByteBuffer has been successfully allocated and refers 
to the memory at addressP. But the direct ByteBuffer, while in the Java heap, 
will not have any references to it, so freeing addressP should be fine here.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 257:

> 255: //TODO: assert false: "should not reach here";
> 256: free(addressP);
> 257: return;

same a previous comment - looks ok.

src/jdk.sctp/unix/native/libsctp/SctpChannelImpl.c line 270:

> 268: return;
> 269: }
> 270: (*env)->SetObjectField(env, resultContainerObj, src_valueID, 
> resultObj);

If NewObject returns NULL, there will be a pending OutOfMemoryError. Freeing 
addressP should be safe here, same reasoning as above.

-

PR: https://git.openjdk.java.net/jdk/pull/2586

Integrated: 8261160: Add a deserialization JFR event

[Chris Hegarty]

On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty  wrote:

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

This pull request has now been integrated.

Changeset: 3dc6f52a
Author:    Chris Hegarty 
URL:   https://git.openjdk.java.net/jdk/commit/3dc6f52a
Stats: 620 lines in 12 files changed: 598 ins; 5 del; 17 mod

8261160: Add a deserialization JFR event

Co-authored-by: Sean Coffey 
Co-authored-by: Chris Hegarty 
Reviewed-by: coffeys, rriggs, dfuchs, egahlin

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v4]

[Chris Hegarty]

On Thu, 11 Feb 2021 16:02:09 GMT, Roger Riggs  wrote:

>> Chris Hegarty has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Filter **C**onfigured
>
> Marked as reviewed by rriggs (Reviewer).

Speaking to Erik offline, he suggested to split the `exceptionMessage` into 
`exceptionType` and `exceptionMessage` ( since the code was somewhat 
overloading the existing Sting field by using toString to force the exception 
class name and message into a single sting ). While the exceptionXXX field 
should rarely be non-null, they add very little overhead.

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v5]

[Chris Hegarty]

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

Chris Hegarty has updated the pull request incrementally with one additional 
commit since the last revision:

  Split exception into type and message

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2479/files
  - new: https://git.openjdk.java.net/jdk/pull/2479/files/d764f75f..8cb8f75e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=03-04

  Stats: 48 lines in 4 files changed: 34 ins; 6 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2479.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v3]

[Chris Hegarty]

On Thu, 11 Feb 2021 15:45:33 GMT, Daniel Fuchs  wrote:

>> Chris Hegarty has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fix failing test
>
> src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 42:
> 
>> 40: 
>> 41: @Label("Filter configured")
>> 42: public boolean filterConfigured;
> 
> Should that be "Filter Configured" for consistency?

Good catch. Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v4]

[Chris Hegarty]

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

Chris Hegarty has updated the pull request incrementally with one additional 
commit since the last revision:

  Filter **C**onfigured

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2479/files
  - new: https://git.openjdk.java.net/jdk/pull/2479/files/5737ee7c..d764f75f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2479.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v3]

[Chris Hegarty]

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

Chris Hegarty has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix failing test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2479/files
  - new: https://git.openjdk.java.net/jdk/pull/2479/files/8ecf63ce..5737ee7c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2479.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v2]

[Chris Hegarty]

On Thu, 11 Feb 2021 14:27:19 GMT, Roger Riggs  wrote:

>> Marked as reviewed by rriggs (Reviewer).
>
> As proposed, events are only created if there is a serialFilter in effect 
> (and enabled by JFR configuration).
> Being able to create the events without a serialFilter in effect would be 
> useful for monitoring, especially if it could be controlled by a separate JFR 
> configuration option.  (always, never, serial-filter , etc.)

I updated the PR and addressed all comments so far. Specifically:

@RogerRiggs The generation of the event is independent of whether the filter is 
set or not.  I also added a piece of state to determine if a filter is set or 
not. I think it could be useful to analyse all Deserialisation events to, say, 
ensure that there are none operating without a filter ( and the `filterStatus` 
state is ambitious in the `null` case ).

@coffeys I would like GlobalFilterTest to run regardless of whether or not the 
jfr module is present, but of course running the test with jfr enabled is 
desirable too, so I added a separate at test tag for that case.

@egahlin Excellent suggestions on the naming, etc. I adopted all.  And added a 
test to ensure that the creation and generation of the event does not 
inadvertently trigger class initialization if the filter rejects the attempt ( 
thanks @dfuch )

@dfuch Thanks for the improved label suggestion, it is now merged in.

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event [v2]

[Chris Hegarty]

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

Chris Hegarty has updated the pull request incrementally with one additional 
commit since the last revision:

  Review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2479/files
  - new: https://git.openjdk.java.net/jdk/pull/2479/files/ca0bcf44..8ecf63ce

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=00-01

  Stats: 124 lines in 5 files changed: 76 ins; 2 del; 46 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2479.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event

[Chris Hegarty]

On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty  wrote:

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

The logging in ObjectInputStream remains conditional on whether a filter is 
installed, which that seems reasonable since the logging is filter specific, 
while the JRF event is not (but both carry effectively the same information).

The new jdk.Deserialization event uses a String to carry the filterStatus 
value. The value could be represented by its enum ordinal, but then the tool 
consuming the event would need to map that back to its string value to be 
meaningful.

-

PR: https://git.openjdk.java.net/jdk/pull/2479

RFR: 8261160: Add a deserialization JFR event

[Chris Hegarty]

This issue adds a new event to improve diagnostic information of Java 
deserialization. The event captures the details of deserialization activity 
from ObjectInputStream. The event details are similar to that of the serial 
filter, but is agnostic of whether a filter is installed or not. The event also 
captures the filter status, if there is one.

-

Commit messages:
 - minor cleanup; all tests passing
 - Unconditionally fire a deser event regardless of filter, and add test
 - commit 2
 - commit 1

Changes: https://git.openjdk.java.net/jdk/pull/2479/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2479&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261160
  Stats: 516 lines in 12 files changed: 496 ins; 5 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2479.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2479/head:pull/2479

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: JDK-8257401: Use switch expressions in jdk.internal.net.http and java.net.http [v4]

[Chris Hegarty]

On Wed, 2 Dec 2020 09:42:09 GMT, Kartik Ohri 
 wrote:

>> Hi!
>> Kindly review this patch to replace switch statements with switch 
>> expressions (where it makes sense) in the http client modules. The rationale 
>> is to improve readability of the code.
>> Regards,
>> Kartik
>
> Kartik Ohri has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

I think that the actual source changes look good.

A few notes:
1. there are whitespace issues. jcheck is failing.
2. Please so not force push - just push. Force push messes up prior comments in 
the thread.

-

Marked as reviewed by chegar (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1364

Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers [v3]

[Chris Hegarty]

On Mon, 12 Oct 2020 13:50:30 GMT, Daniel Fuchs  wrote:

>> Hi,
>> 
>> This is a fix that upgrades the old HTTP and HTTPS legacy stack to use 
>> virtual-thread friendly locking instead of
>> synchronized monitors.
>> Most of the changes are mechanical - but there are still a numbers of subtle 
>> non-mechanical differences that are
>> outlined below:
>> 1. src/java.base/share/classes/sun/net/www/MessageHeader.java:
>>`MessageHeader::print(PrintStream)` => synchronization modified to not 
>> synchronize on this while printing
>>( a snapshot of the data is taken before printing instead)
>> 
>> 2. src/java.base/share/classes/sun/net/www/MeteredStream.java:
>> `MeteredStream::close` was missing synchronization: it is now protected 
>> by the lock
>> 
>> 3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java:
>> `ChunkedOutputStream` no longer extends `PrintStream` but extends 
>> `OutputStream` directly.
>> Extending `PrintStream` is problematic for virtual thread. After careful 
>> analysis, it appeared that
>> `ChunkedOutputStream` didn't really need to extend `PrintStream`. 
>> `ChunkedOutputStream`
>> is already wrapping a `PrintStream`.  `ChunkedOutputStream` is never 
>> returned directly to user
>> code but is wrapped in another stream. `ChunkedOutputStream` completely 
>> reimplement and
>> reverse the flush logic implemented by its old super class`PrintStream` 
>> which leads me to believe
>> there was no real reason for it to extend `PrintStream` - except for 
>> being able to call its `checkError`
>> method - which can be done by using `instanceof ChunkedOutputStream` in 
>> the caller instead of
>> casting to PrintStream.
>> 
>> 4. src/java.base/share/classes/sun/net/www/http/HttpClient.java:
>> Synchronization removed from `HttpClient::privilegedOpenServer` and 
>> replaced by an `assert`.
>> There is no need for a synchronized here as the method is only called 
>> from a method that already
>> holds the lock.
>> 
>> 5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java:
>> Synchronization removed from `KeepAliveCache::removeVector` and replaced 
>> by an `assert`.
>> This method is only called from methods already protected by the lock.
>> Also the method has been made private.
>> 
>> 6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java
>> `queuedForCleanup` is made volatile as it is read and written directly 
>> from outside the class
>> and without protection by the `KeepAliveCleanerEntry`.
>> Lock protection is also added to `close()`, which was missing it.
>> Some methods that have no lock protection and did not need it because 
>> only called from
>> within code blocks protected by the lock have aquired an `assert 
>> isLockHeldByCurrentThread();`
>> 
>> 7. Concrete subclasses of `AuthenticationInfo` that provide an 
>> implementation for
>> `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, 
>> String raw)` have
>> acquired an `assert conn.isLockheldByCurrentThread();` as the method 
>> should only be called
>> from within a lock-protected block in `s.n.w.p.h.HttpURLConnection`
>> 
>> 8. 
>> src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java
>> Several methods in this class have a acquired an `assert 
>> isLockheldByCurrentThread();`
>> to help track the fact that AuthenticationInfo::setHeaders is only 
>> called while
>> holding the lock.
>> Synchronization was also removed from some method that didn't need it 
>> because only
>> called from within code blocks protected by the lock:
>> `getOutputStream0()`: synchronization removed and replaced by an `assert 
>> isLockheldByCurrentThread();`
>> `setCookieHeader()`:  synchronization removed and replaced by an `assert 
>> isLockheldByCurrentThread();`
>> `getInputStream0()`:  synchronization removed and replaced by an `assert 
>> isLockheldByCurrentThread();`
>> `StreamingOutputStream`: small change to accomodate point 3. above 
>> (changes in ChunkedOutputStream).
>> 
>> 9. 
>> src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.:
>> synchronization removed from `setHeaders` and replace by an assert. See 
>> point 7. above.
>> 
>> 10. 
>> src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
>> synchronization removed from `setHeaders` and replace by an assert. See 
>> point 7. above.
>> 
>> 11. 
>> src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
>> synchronization removed from `setHeaders` and replace by an assert. See 
>> point 7. above.
>
> Daniel Fuchs has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull 
> request contains four additional commits since
> the last revision:

Re: RFR: 8229867: Re-examine synchronization usages in http and https protocol handlers

[Chris Hegarty]

On Thu, 8 Oct 2020 11:22:09 GMT, Daniel Fuchs  wrote:

> Hi,
> 
> This is a fix that upgrades the old HTTP and HTTPS legacy stack to use 
> virtual-thread friendly locking instead of
> synchronized monitors.
> Most of the changes are mechanical - but there are still a numbers of subtle 
> non-mechanical differences that are
> outlined below:
> 1. src/java.base/share/classes/sun/net/www/MessageHeader.java:
>`MessageHeader::print(PrintStream)` => synchronization modified to not 
> synchronize on this while printing
>( a snapshot of the data is taken before printing instead)
> 
> 2. src/java.base/share/classes/sun/net/www/MeteredStream.java:
> `MeteredStream::close` was missing synchronization: it is now protected 
> by the lock
> 
> 3. src/java.base/share/classes/sun/net/www/http/ChunkedOutputStream.java:
> `ChunkedOutputStream` no longer extends `PrintStream` but extends 
> `OutputStream` directly.
> Extending `PrintStream` is problematic for virtual thread. After careful 
> analysis, it appeared that
> `ChunkedOutputStream` didn't really need to extend `PrintStream`. 
> `ChunkedOutputStream`
> is already wrapping a `PrintStream`.  `ChunkedOutputStream` is never 
> returned directly to user
> code but is wrapped in another stream. `ChunkedOutputStream` completely 
> reimplement and
> reverse the flush logic implemented by its old super class`PrintStream` 
> which leads me to believe
> there was no real reason for it to extend `PrintStream` - except for 
> being able to call its `checkError`
> method - which can be done by using `instanceof ChunkedOutputStream` in 
> the caller instead of
> casting to PrintStream.
> 
> 4. src/java.base/share/classes/sun/net/www/http/HttpClient.java:
> Synchronization removed from `HttpClient::privilegedOpenServer` and 
> replaced by an `assert`.
> There is no need for a synchronized here as the method is only called 
> from a method that already
> holds the lock.
> 
> 5. src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java:
> Synchronization removed from `KeepAliveCache::removeVector` and replaced 
> by an `assert`.
> This method is only called from methods already protected by the lock.
> Also the method has been made private.
> 
> 6. src/java.base/share/classes/sun/net/www/http/KeepAliveStream.java
> `queuedForCleanup` is made volatile as it is read and written directly 
> from outside the class
> and without protection by the `KeepAliveCleanerEntry`.
> Lock protection is also added to `close()`, which was missing it.
> Some methods that have no lock protection and did not need it because 
> only called from
> within code blocks protected by the lock have aquired an `assert 
> isLockHeldByCurrentThread();`
> 
> 7. Concrete subclasses of `AuthenticationInfo` that provide an implementation 
> for
> `AuthenticationInfo::setHeaders(HttpURLConnection conn, HeaderParser p, 
> String raw)` have
> acquired an `assert conn.isLockheldByCurrentThread();` as the method 
> should only be called
> from within a lock-protected block in `s.n.w.p.h.HttpURLConnection`
> 
> 8. 
> src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java
> Several methods in this class have a acquired an `assert 
> isLockheldByCurrentThread();`
> to help track the fact that AuthenticationInfo::setHeaders is only called 
> while
> holding the lock.
> Synchronization was also removed from some method that didn't need it 
> because only
> called from within code blocks protected by the lock:
> `getOutputStream0()`: synchronization removed and replaced by an `assert 
> isLockheldByCurrentThread();`
> `setCookieHeader()`:  synchronization removed and replaced by an `assert 
> isLockheldByCurrentThread();`
> `getInputStream0()`:  synchronization removed and replaced by an `assert 
> isLockheldByCurrentThread();`
> `StreamingOutputStream`: small change to accomodate point 3. above 
> (changes in ChunkedOutputStream).
> 
> 9. 
> src/java.base/share/classes/sun/net/www/protocol/http/NegotiateAuthentication.java.:
> synchronization removed from `setHeaders` and replace by an assert. See 
> point 7. above.
> 
> 10. 
> src/java.base/unix/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
> synchronization removed from `setHeaders` and replace by an assert. See 
> point 7. above.
> 
> 11. 
> src/java.base/windows/classes/sun/net/www/protocol/http/ntlm/NTLMAuthentication.java:
> synchronization removed from `setHeaders` and replace by an assert. See 
> point 7. above.

Mostly looks good. Just a few comments.

src/java.base/share/classes/sun/net/www/MessageHeader.java line 330:

> 328: at the end. Omits pairs with a null key. Omits
> 329: colon if key-value pair is the requestline. */
> 330: private  void print(int nkeys, String[] keys, String[] values, 
> PrintStream p) {

While not strictly necessary, this met

Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[Chris Hegarty]

> On 8 Apr 2020, at 10:13, Rahul  wrote:
> 
> Updated patch after considering the impact of returning default parameters on 
> the http client.
> TLS versions earlier limited to 1.2 and above by client, now will support all 
> versions(wrt the scenarios for this bug).
> 
>Issue: https://bugs.openjdk.java.net/browse/JDK-8239595
>Issue: https://bugs.openjdk.java.net/browse/JDK-8239594
> 
>Webrev: 
> http://cr.openjdk.java.net/~jboes/rayayada/webrevs/8239595/webrev.01/

Thanks for persevering with this. The changes and test look good to me.

-Chris.

Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[Chris Hegarty]

Thank you for these clarifications. We will now consider how these affect, if 
at all, the HTTP Client.

-Chris.

> On 27 Mar 2020, at 17:47, Xuelei Fan  wrote:
> 
> On 3/27/2020 10:36 AM, Chris Hegarty wrote:
>> Thank you Xuelei, this very helpful.
>> Sorry, but I am going to ask just a few more clarifying questions to make 
>> sure that we’re on the same page.
>>> On 27 Mar 2020, at 16:23, Xuelei Fan  wrote:
>>> 
>>> On 3/27/2020 5:52 AM, Chris Hegarty wrote:
>>>> Xuelei,
>>>> Before commenting further on the interaction of the HTTP Client with 
>>>> various contorted configurations, I would like to get a better 
>>>> understanding of the `jdk.tls.client.protocols` property.
>>>> Is there a specification or other documentation describing 
>>>> `jdk.tls.client.protocols` ?
>>> See the jdk.tls.client.protocols line in table 'Table 8-3 System Properties 
>>> and Customized Items" in JSSE Reference Guides:
>>> 
>>> "https://docs.oracle.com/en/java/javase/14/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-A41282C3-19A3-400A-A40F-86F4DA22ABA9
>>> 
>>> For your quick reference, I copied the note here:
>>> 
>>> ---
>>> Customized Item:
>>> Default handshaking protocols for TLS/DTLS clients.
>>> 
>>> Notes:
>>> To enable specific SunJSSE protocols on the client, specify them in a 
>>> comma-separated list within quotation marks; all other supported protocols 
>>> are not enabled on the client
>> “supported” here means protocols that are supported by the provider, and may 
>> be used within a specific context. This translates, for the default 
>> SSLContext, to the API call getSupportedSSLParameters().getProtocols(), 
>> right?
> Yes.
> 
>> getSupportedSSLParameters().getProtocols() returns a superset of 
>> getDefaultSSLParameters().getProtocols(). Conversely, 
>> getDefaultSSLParameters().getProtocols() is a strict subset of 
>> getSupportedSSLParameters().getProtocols(), right?
> Yes.
> 
>> The `jdk.tls.client.protocols` property has no affect on 
>> getSupportedSSLParameters().getProtocols()  only 
>> getDefaultSSLParameters().getProtocols(), right?
> Yes.
> 
>> In which case, getDefaultSSLParameters().getProtocols() returns the value of 
>>  `jdk.tls.client.protocols`.
>>> For example,
>>> 
>>>If jdk.tls.client.protocols="TLSv1,TLSv1.1", then the default protocol 
>>> settings on the client for TLSv1 and TLSv1.1 are enabled, while SSLv3, 
>>> TLSv1.2, TLSv1.3, and SSLv2Hello are not enabled
>>> 
>>>If jdk.tls.client.protocols="DTLSv1.2" , then the protocol setting on 
>>> the client for DTLS1.2 is enabled, while DTLS1.0 is not enabled
>>> ---
>> Seems that the term “client” here is referring to client-initiated 
>> exchanges, rather than any specific technology.
>> The assumption, which is reasonable, is that “clients” will use the default 
>> context. Again, this is reasonable default out-of-the-box behavior.
> The client refer to the client side SSLSocket or SSLEngine created with the 
> default SSLContext.  or example:
>SSLContext sslContext = SSLContext.getInstance("TLS");
>SSLEngine sslEngine = sslContext.createSSLEngine();
>sslEngine.setUseClientMode(true);
> 
> The sslEngine object is a client that impacted by the property.
> 
> While if
>sslEngine.setUseClientMode(false);
> 
> then the object should not be impacted by the property.
> 
> Xuelei
> 
>>>> It is my understanding that the property only affects the *default* 
>>>> protocol’s ( not the supported protocols ) of the *default* context. That 
>>>> is, the context returned by `SSLContext.getInstance("Default”)`,
>>> It is correct that the property impact the default SSLContext only.  The 
>>> default SSLContext instance could get from:
>>>SSLContext.getInstance("Default");
>>>SSLContext.getInstance("TLS");
>>>SSLContext.getInstance("DTLS”);
>> Thanks for this clarification.
>>> 
>>>> and the protocol values returned by the following invocation on that 
>>>> context `getDefaultSSLParameters().getProtocols()`. Is this correct? If 
>>>> not, what does it do?
>>> Yes.
>> Thanks,
>> -Chris.

Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[Chris Hegarty]

Thank you Xuelei, this very helpful.

Sorry, but I am going to ask just a few more clarifying questions to make sure 
that we’re on the same page.

> On 27 Mar 2020, at 16:23, Xuelei Fan  wrote:
> 
> On 3/27/2020 5:52 AM, Chris Hegarty wrote:
>> Xuelei,
>> Before commenting further on the interaction of the HTTP Client with various 
>> contorted configurations, I would like to get a better understanding of the 
>> `jdk.tls.client.protocols` property.
>> Is there a specification or other documentation describing 
>> `jdk.tls.client.protocols` ?
> See the jdk.tls.client.protocols line in table 'Table 8-3 System Properties 
> and Customized Items" in JSSE Reference Guides:
> 
> "https://docs.oracle.com/en/java/javase/14/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-A41282C3-19A3-400A-A40F-86F4DA22ABA9
> 
> For your quick reference, I copied the note here:
> 
> ---
> Customized Item:
> Default handshaking protocols for TLS/DTLS clients.
> 
> Notes:
> To enable specific SunJSSE protocols on the client, specify them in a 
> comma-separated list within quotation marks; all other supported protocols 
> are not enabled on the client

“supported” here means protocols that are supported by the provider, and may be 
used within a specific context. This translates, for the default SSLContext, to 
the API call getSupportedSSLParameters().getProtocols(), right? 

getSupportedSSLParameters().getProtocols() returns a superset of 
getDefaultSSLParameters().getProtocols(). Conversely, 
getDefaultSSLParameters().getProtocols() is a strict subset of 
getSupportedSSLParameters().getProtocols(), right?

The `jdk.tls.client.protocols` property has no affect on 
getSupportedSSLParameters().getProtocols()  only 
getDefaultSSLParameters().getProtocols(), right? In which case, 
getDefaultSSLParameters().getProtocols() returns the value of  
`jdk.tls.client.protocols`.

> For example,
> 
>If jdk.tls.client.protocols="TLSv1,TLSv1.1", then the default protocol 
> settings on the client for TLSv1 and TLSv1.1 are enabled, while SSLv3, 
> TLSv1.2, TLSv1.3, and SSLv2Hello are not enabled
> 
>If jdk.tls.client.protocols="DTLSv1.2" , then the protocol setting on the 
> client for DTLS1.2 is enabled, while DTLS1.0 is not enabled
> ---

Seems that the term “client” here is referring to client-initiated exchanges, 
rather than any specific technology.

The assumption, which is reasonable, is that “clients” will use the default 
context. Again, this is reasonable default out-of-the-box behavior.

>> It is my understanding that the property only affects the *default* 
>> protocol’s ( not the supported protocols ) of the *default* context. That 
>> is, the context returned by `SSLContext.getInstance("Default”)`,
> It is correct that the property impact the default SSLContext only.  The 
> default SSLContext instance could get from:
>SSLContext.getInstance("Default");
>SSLContext.getInstance("TLS");
>SSLContext.getInstance("DTLS”);

Thanks for this clarification.

> 
>> and the protocol values returned by the following invocation on that context 
>> `getDefaultSSLParameters().getProtocols()`. Is this correct? If not, what 
>> does it do?
> Yes.

Thanks,
-Chris.

Re: RFR JDK-8239595/JDK-8239594 : ssl context version is not respected/jdk.tls.client.protocols is not respected

[Chris Hegarty]

Xuelei,

Before commenting further on the interaction of the HTTP Client with various 
contorted configurations, I would like to get a better understanding of the 
`jdk.tls.client.protocols` property. 

Is there a specification or other documentation describing 
`jdk.tls.client.protocols` ?

It is my understanding that the property only affects the *default* protocol’s 
( not the supported protocols ) of the *default* context. That is, the context 
returned by `SSLContext.getInstance("Default”)`, and the protocol values 
returned by the following invocation on that context 
`getDefaultSSLParameters().getProtocols()`. Is this correct? If not, what does 
it do?

-Chris.

> On 26 Mar 2020, at 16:58, Xuelei Fan  wrote:
> 
> With this update, the logic looks like: if TLSv1.3 is not enabled in the 
> SSLContext, use TLSv1.2 instead;  Otherwise, use TLSv1.3 and TLSv1.2.
> 
> There may be a couple of issues:
> 1. TLSv1.2 may be not enabled, although TLSv1.3 is enabled.
> For example:
>   System.setProperty("jdk.tls.client.protocols", "TLSv1.3")
>   System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
> 
> 2. TLSv1.2 may be not supported in the SSLContext.
> For example:
>   SSLContext context = SSLContext.getInstance("DTLS");
>   HttpClient.newBuilder().sslContext(context)...
> 
> 3. The application may not want to use TLS 1.2.
> For example:
>   System.setProperty("jdk.tls.client.protocols", "TLSv1.1, TLSv1.0")
> 
> The System property may be shared by code other than httpclient.  So the 
> setting may not consider the impact on httpclient.
> 
> I may use enabled protocols only. If no TLSv1.2/TLSv1.3, I may use an empty 
> protocol array, and test to see what happens in the httpclient implementation 
> stack.
> 
> Xuelei
> 
> On 3/26/2020 9:28 AM, Sean Mullan wrote:
>> Cross-posting to security-dev as this involves TLS/SSL configuration.
>> --Sean
>> On 3/26/20 10:02 AM, rahul.r.ya...@oracle.com wrote:
>>> Hello,
>>> 
>>> Request to have my fix reviewed for issues:
>>> 
>>>  JDK-8239595 : ssl context version is not respected
>>>  JDK-8239594 : jdk.tls.client.protocols is not respected
>>> 
>>> The fix updates 
>>> jdk.internal.net.http.HttpClientImpl.getDefaultParams(SSLContext ctx)
>>> to use ctx.getDefaultSSLParameters()instead of 
>>> ctx.getSupportedSSLParameters(),
>>> as the latter does not respect the context parameters set by the user.
>>> 
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8239595
>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8239594
>>> 
>>> Webrev: 
>>> http://cr.openjdk.java.net/~jboes/rayayada/webrevs/8239595/webrev.00/
>>> 
>>> -- Rahul

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

[Chris Hegarty]

> On 13 Jan 2020, at 17:19, Seán Coffey  wrote:
> 
> Thanks for the reviews. All callers of EventHelper log methods are ensuring 
> that isLoggingSecurity() is true before proceeding. I've added an assert null 
> check in the 4 logger methods to ensure expectations are in place.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Thanks. LGTM.

-Chris.

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

[Chris Hegarty]

> On 13 Jan 2020, at 13:14, Daniel Fuchs  wrote:
> 
> On 13/01/2020 10:28, Seán Coffey wrote:
>> some off line comments suggested that I could move the jar initialization 
>> checks to the EventHelper class. With that in place, the EventHelper utility 
>> class should never initialize the logging framework early during jar 
>> initialization.
>> http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
> 
> Looks good to me Seán. Probably safer than the other alternatives.

+1

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

-Chris

Re: Reading from a closed socket: different behavior between Linux and other operating systems

[Chris Hegarty]

I agree with David, this is an expected difference between the socket
layers on different operating systems. The different behaviours are
acceptable implementations on said operating systems. Unfortunately,
this results in different behaviour of the Java socket (and TCP socket
channel) APIs, when running on different platforms (as you are
observing).

I am not suggesting that you do this, but if the intention is that the
server issues a forceful / abortive close, then ( at least in the
minimal repo case ) one could:

// force RST
clientChannel.setOption(StandardSocketOptions.SO_LINGER, 0);

I read over the Lucene issue that you sited; it appears that the
higher-level problem you are encountering is how/when to issue an HTTP
retry request when there is a socket or perceived SSL exception. And how
the SSLSocketImpl handles the difference between the above socket layer
behaviours.

SSLSocket aggregates both the network socket operations and SSL protocol
implementation, which makes it more difficult to discern where the
underlying issue is coming from, than say, if one was to use
SocketChannel along with SSLEngine. However, I would expect that any
SSLException that has a SocketException as its cause could be considered
as a "network problem" - as described by the SocketException message.
The wrapping SSLException in such a case should provide additional
SSL protocol specific information to help with diagnosis, e.g. the
current protocol state.

I'm sure that this is not the answer that you were looking for, but
maybe you can consider it when making a decision on whether this can be
fixed in the HTTP Client that you are using.

-Chris.

> On 3 Jan 2020, at 13:53, David Lloyd  wrote:
> 
> This is, AFAICT, expected based on the differences between the socket layers 
> of the various operating systems involved and their handling of closed 
> sockets.  If you write a similar test program in C using OS specific APIs, I 
> believe you will see similar results.  I don't think this is a problem with 
> the JDK, nor is it likely to be something that can be fixed in the JDK (since 
> the error reported by the OS is, as far as I know, unlikely to be universally 
> sufficient to extrapolate the exact cause of failure).

Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors

[Chris Hegarty]

The changes to the SCTP code seem ok.

-Chris.

> On 17 Dec 2019, at 03:00, Patrick Zhang OS  
> wrote:
> 
> Thanks Martin.
> 
> Hi net-dev, and/or security-dev Reviewers,
> 
> Please help review and sponsor this patch if acceptable.
> It does not tend to bring any functionality changes, instead to propose an 
> early fix, for the build (linking) errors with further toolchain (GCC 10).
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8235903
> Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/
> 
> Regards
> Patrick
> 
> From: Martin Buchholz 
> Sent: Monday, December 16, 2019 10:44 AM
> To: Patrick Zhang OS ; net-dev 
> ; OpenJDK 
> Cc: core-libs-dev 
> Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple 
> definition" link errors
> 
> forwarded to other teams for review.
> 
> On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS 
> mailto:patr...@os.amperecomputing.com>> wrote:
> Hi
> 
> Please review this patch, if it should be reviewed by any group other than 
> core-libs, please help forward it. Thanks.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8235903
> Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/
> 
> A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple 
> definition" link errors when building the jdk tip.
> 
> [PATCH] PR85678: Change default to -fno-common
> https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html
> 
> For example, the error message looks like:
> * For target support_native_java.base_libjava_BUILD_LIBJAVA_link:
> build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple 
> definition of `parentPathv'
> build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): first 
> defined here
> collect2: error: ld returned 1 exit status
> 
> This was not an issue because the original default -fcommon allowed "global 
> variables defined without an initializer" be handled as COMMON symbols, so it 
> would not warn the problem like "same variable is accidentally defined in 
> more than one compilation unit".
> 
> About -fcommon vs -fno-cmmon:
> https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-common
> 
> Moving forward, building jdk with latest versions of GCC will trigger this 
> error. Specifying "--with-extra-cflags='-fcommon'"  can make it work, but it 
> just got things hidden again.
> 
> In addition, -fcommon's behavior "is inconsistent with C++, and on many 
> targets implies a speed and code size penalty on global variable references. 
> It is mainly useful to enable legacy code to link without errors."
> 
> Last, in case that other jdk developers would revisit this problem once 
> again, I suggest fixing the error explicitly instead of using "-fcommon"
> 
> Regards
> Patrick

Re: JDK 14 RFR of JDK-8231262: Suppress warnings on non-serializable instance fields in security libs serializable classes

[Chris Hegarty]

On 09/10/2019 14:54, Sean Mullan wrote:

...

X509CertImpl extends X509Certificate which extends Certificate. 
Certificate has a writeReplace method.


Another possible follow-on is to add readObject methods, that 
unconditionally throw, to both X509Certificate and X509CertImpl, since 
serialized instances of these types should not appear in the stream. 
That would be a nice addition to the suggestion to make all the fields 
transient - and improve the readability of the code.


-Chris.

Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma

[Chris Hegarty]

Ivan,

> On 4 Oct 2019, at 03:00, Ivan Gerasimov  wrote:
> 
> ...
> 
> I've adopted your suggested changes and the test:
> http://cr.openjdk.java.net/~igerasim/8230407/02/webrev/ 
> 

LGTM.

> CSR was also updated accordingly:
> https://bugs.openjdk.java.net/browse/JDK-8231805 
> 

I added myself as reviewer.

-Chris.

Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma

[Chris Hegarty]

Ivan,

> On 3 Oct 2019, at 04:41, Ivan Gerasimov  wrote:
> 
> ...
> 
> So, I filed CSR: https://bugs.openjdk.java.net/browse/JDK-8231805 to cover 
> the addition of @throws paragraph in the javadoc of SocketPermission.
> 
> I would really appreciate it, if someone helped to review it.
> 
Since we’re here ... ;-)
It would be good to specify the NPE behavior of the constructor. Here are the 
changes for SocketPermission. If you agree, fold them into your patch and CSR. 
( I’ve included test changes to verify the new tighter spec )

https://cr.openjdk.java.net/~chegar/8230407.extra/

-Chris.

Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma

[Chris Hegarty]

Ivan,

On 01/10/2019 21:26, Ivan Gerasimov wrote:

Hello!

The constructors of SocketPermission and FilePermission expect a String 
argument with comma-separated list of actions.


If the list is malformed, then the constructors throw 
IllegalArgumentException.


It turns out that the current implementation fails to throw IAE if the 
list starts with a leading comma.


Would you please help review a simple fix, which will make the behavior 
more consistent?


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8230407
WEBREV: http://cr.openjdk.java.net/~igerasim/8230407/00/webrev/


The implementation changes look ok.

The SocketPermission constructor should be updated to specify IAE too, 
right?


-Chris.

Re: JDK 14 RFR of JDK-8231262: Suppress warnings on non-serializable instance fields in security libs serializable classes

[Chris Hegarty]

On 26/09/2019 21:10, Joe Darcy wrote:

..

Yes; applying SuppressWarnings("serial") to the class will silence this 
new warning for all the fields. 


Will is also suppress other warnings, like on the serialization-related 
magic methods? Which would be unfortunate.


-Chris.

Re: JDK 14 RFR of JDK-8231262: Suppress warnings on non-serializable instance fields in security libs serializable classes

[Chris Hegarty]

> On 19 Sep 2019, at 18:32, Joe Darcy  wrote:
> 
> Hello,
> 
> Ahead of augmenting javac's serial lint checks under JDK-8160675, it would be 
> helpful to mark fields in security libs classes where the class is 
> serializable, but a non-transient instance field does *not* have a 
> serialiable type. Such classes may have difficulties being serialized at 
> runtime:
> 
> JDK-8231262 : Suppress warnings on non-serializable instance fields in 
> security libs serializable classes
> http://cr.openjdk.java.net/~darcy/8231262.0/

The changes look good to me.

The fields in PrivateCredentialPermission and SecureRandom, could be made final 
and assigned null, ensuring non-Serializable types will never leak into them. 
But equally, this could be left to a follow on change for someone working in 
the security area.

-Chris.

Re: RFR: 8224656: Problem list java/security/SecureClassLoader/DefineClass.java until JDK-8224635 is fixed

[Chris Hegarty]

Daniel,

> On 23 May 2019, at 11:30, Daniel Fuchs  wrote:
> 
> Hi,
> 
> I have observed an intermittent failure on Solaris.
> So I changed the patch to problem-list the test on all platform.
> 
> Is that still OK?
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8224656/webrev.01 
> 

Looks good.

-Chris.

Re: RFR: 8224656: Problem list java/security/SecureClassLoader/DefineClass.java until JDK-8224635 is fixed

[Chris Hegarty]

Daniel,

> On 23 May 2019, at 10:32, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find a patch below that temporarily problem lists
> java/security/SecureClassLoader/DefineClass.java
> on linux and windows until JDK-8224635 [1] is fixed:
> 
> 8224656: Problem list java/security/SecureClassLoader/DefineClass.java
> until JDK-8224635 is fixed
> https://bugs.openjdk.java.net/browse/JDK-8224656
> 
> webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8224656/webrev.00/
> 
Reviewed.

Thanks,
-Chris.

Re: [ipv6] RFR: 8224256: test/jdk/java/security/SecureClassLoader/DefineClass.java hardcodes 127.0.0.1

[Chris Hegarty]

Arthur,

> On 21 May 2019, at 00:50, Arthur Eubanks  > wrote:
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8224256 
> webrev: 
> http://cr.openjdk.java.net/~aeubanks/8224256/webrev.00/index.html 
> 
> 
> test/jdk/java/security/SecureClassLoader/DefineClass.java checks that a 
> security manager works with "http://127.0.0.1 ". This 
> fails on IPv6-only systems.
> 
> This change tests 127.0.0.1 if IPv4 is available, then tests ::1 if IPv6 is 
> available. Created a new class for testing ::1 since we can't have duplicate 
> classes.

I think this is ok.

I’ve included security-dev, as the test in question is in their area.

-Chris. 

Re: [ipv6] RFR: 8224256: test/jdk/java/security/SecureClassLoader/DefineClass.java hardcodes 127.0.0.1

[Chris Hegarty]

Arthur,

> On 21 May 2019, at 00:50, Arthur Eubanks  > wrote:
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8224256 
> webrev: 
> http://cr.openjdk.java.net/~aeubanks/8224256/webrev.00/index.html 
> 
> 
> test/jdk/java/security/SecureClassLoader/DefineClass.java checks that a 
> security manager works with "http://127.0.0.1 ". This 
> fails on IPv6-only systems.
> 
> This change tests 127.0.0.1 if IPv4 is available, then tests ::1 if IPv6 is 
> available. Created a new class for testing ::1 since we can't have duplicate 
> classes.

I think this is ok.

I’ve included security-dev, as the test in question is in their area.

-Chris. 

Re: [ipv6]: 8224081: SOCKS v4 doesn't work with IPv6

[Chris Hegarty]

Arthur,

> On 17 May 2019, at 17:50, Arthur Eubanks  wrote:
> 
> Looks good.
> 
> Trivially, maybe amend the comment to be more explicit
> 
>86   // SOCKS V4 ( requires IPv4 )
> 
> -Chris.
> Done
> http://cr.openjdk.java.net/~aeubanks/8224081/webrev.02/ 
> 
> 
> I will wait for another review from security-dev.

You have my Review ( conditional on a Reviewer for the test in the security 
area ).

-Chris.

RFR 8220598: Malformed copyright year range in a few files in java.base

[Chris Hegarty]

Trivially, there should be a comma after the year. Just add it.


$ hg diff src/java.base/share/classes/jdk/ src/java.base/share/classes/sun
diff --git a/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java 
b/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
--- a/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
+++ b/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2017 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
diff --git a/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java 
b/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
--- a/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
+++ b/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, 2018 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
diff --git a/src/java.base/share/classes/sun/security/util/IOUtils.java 
b/src/java.base/share/classes/sun/security/util/IOUtils.java
--- a/src/java.base/share/classes/sun/security/util/IOUtils.java
+++ b/src/java.base/share/classes/sun/security/util/IOUtils.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2017 Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it

-Chris.

Re: RFR [13] JDK-8215430: Remove the internal package com.sun.net.ssl

[Chris Hegarty]

> On 26 Feb 2019, at 03:53, Xuelei Fan  wrote:
> 
> 
> 
> It makes sense to me to leave the AbstractDelegateHttpsURLConnection methods 
> as public.  We may need to re-org the code later, but it is out of the scope 
> of this update.
> 
> Here is the new webrev (only AbstractDelegateHttpsURLConnection updated):
>http://cr.openjdk.java.net/~xuelei/8215430/webrev.01/

I think that this is probably ok ( since the package is not exported ),
but it seems a little distasteful to not have the API move through
the terminal deprecation route before being removed.

-Chris.

Re: Code Review Request, JDK-8214418 HttpClient falls in running with 100% cpu usage after an error signalled on channel

[Chris Hegarty]

Xuelei,

Is it possible to update the synopsis of this bug to better
reflect the underlying issue ( rather than one particular
symptom )?

Also, it is possible to construct a small, non-HTTP related,
targeted test that verifies the fix?

-Chris.

On 08/01/2019 23:00, Xue-Lei Fan wrote:

ping ...

Xuelei

On 12/22/2018 9:20 AM, Xue-Lei Fan wrote:

Hi,

Could I get the update reviewed?
    http://cr.openjdk.java.net/~xuelei/8214418/webrev.00/

The reproducing testing case passed with the update.

The issue is caused by the handshake status "NEED_WRAP" while the 
connection is half-closed. An application may just call wrap() when 
the handshake status is "NEED_WRAP". For compatibility, I changed the 
handshake status from NEED_WRAP back to NOT_HANDSHAKING for inbound 
half-closed connection.  An application can use 
SSLEngine.isOutboundDone() for the determination if SSLEngine.wrap() 
should be called.


Thanks,
Xuelei

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Chris Hegarty]

On Dec 4, 2018, at 1:11 AM, Seán Coffey  wrote:

whoops:
latest webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213952.v4/webrev/
Regards,
Sean.




Looks good.

-Chris.

Re: JDK 12 RFR of JDK-8213911: Use example.com in java.net and other examples

[Chris Hegarty]

Looks ok to me.

-Chris.

On 26/11/2018 21:38, joe darcy wrote:

Hello,

Please review a simple doc-only change to address:

     JDK-8213911: Use example.com in java.net and other examples
     http://cr.openjdk.java.net/~darcy/8213911.0/

Patch below.

Thanks,

-Joe


--- old/src/java.base/share/classes/java/net/HostPortrange.java 
2018-11-26 13:32:47.07800 -0800
+++ new/src/java.base/share/classes/java/net/HostPortrange.java 
2018-11-26 13:32:46.90200 -0800

@@ -60,7 +60,7 @@
  HostPortrange(String scheme, String str) {
  // Parse the host name.  A name has up to three components, the
  // hostname, a port number, or two numbers representing a port
-    // range.   "www.sun.com:8080-9090" is a valid host name.
+    // range.   "www.example.com:8080-9090" is a valid host name.

  // With IPv6 an address can be 2010:836B:4179::836B:4179
  // An IPv6 address needs to be enclose in []
--- old/src/java.base/share/classes/java/net/InetAddress.java 2018-11-26 
13:32:47.47400 -0800
+++ new/src/java.base/share/classes/java/net/InetAddress.java 2018-11-26 
13:32:47.29800 -0800

@@ -1168,7 +1168,7 @@
   * No name service is checked for the validity of the address.
   *
   *  The host name can either be a machine name, such as
- * "{@code java.sun.com}", or a textual representation of its IP
+ * "{@code www.example.com}", or a textual representation of its IP
   * address.
   *  No validity checking is done on the host name either.
   *
@@ -1213,7 +1213,7 @@
   * Determines the IP address of a host, given the host's name.
   *
   *  The host name can either be a machine name, such as
- * "{@code java.sun.com}", or a textual representation of its
+ * "{@code www.example.com}", or a textual representation of its
   * IP address. If a literal IP address is supplied, only the
   * validity of the address format is checked.
   *
@@ -1259,7 +1259,7 @@
   * based on the configured name service on the system.
   *
   *  The host name can either be a machine name, such as
- * "{@code java.sun.com}", or a textual representation of its IP
+ * "{@code www.example.com}", or a textual representation of its IP
   * address. If a literal IP address is supplied, only the
   * validity of the address format is checked.
   *
--- old/src/java.base/share/classes/java/net/SocketPermission.java 
2018-11-26 13:32:47.85000 -0800
+++ new/src/java.base/share/classes/java/net/SocketPermission.java 
2018-11-26 13:32:47.67800 -0800

@@ -63,7 +63,7 @@
   * or as "localhost" (for the local machine).
   * The wildcard "*" may be included once in a DNS name host
   * specification. If it is included, it must be in the leftmost
- * position, as in "*.sun.com".
+ * position, as in "*.example.com".
   * 
   * The format of the IPv6reference should follow that specified in http://www.ietf.org/rfc/rfc2732.txt";>RFC 2732: Format
@@ -115,11 +115,11 @@
   * note that if the following permission:
   *
   * 
- *   p1 = new SocketPermission("puffin.eng.sun.com:", 
"connect,accept");

+ *   p1 = new SocketPermission("foo.example.com:", "connect,accept");
   * 
   *
   * is granted to some code, it allows that code to connect to port 
 on

- * {@code puffin.eng.sun.com}, and to accept connections on that port.
+ * {@code foo.example.com}, and to accept connections on that port.
   *
   * Similarly, if the following permission:
   *
@@ -211,7 +211,7 @@
  // all the IP addresses of the host
  private transient InetAddress[] addresses;

-    // true if the hostname is a wildcard (e.g. "*.sun.com")
+    // true if the hostname is a wildcard (e.g. "*.example.com")
  private transient boolean wildcard;

  // true if we were initialized with a single numeric IP address
@@ -274,9 +274,9 @@
   * 
   * Examples of SocketPermission instantiation are the following:
   * 
- *    nr = new SocketPermission("www.catalog.com", "connect");
- *    nr = new SocketPermission("www.sun.com:80", "connect");
- *    nr = new SocketPermission("*.sun.com", "connect");
+ *    nr = new SocketPermission("www.example.com", "connect");
+ *    nr = new SocketPermission("www.example.com:80", "connect");
+ *    nr = new SocketPermission("*.example.com", "connect");
   *    nr = new SocketPermission("*.edu", "resolve");
   *    nr = new SocketPermission("204.160.241.0", "connect");
   *    nr = new SocketPermission("localhost:1024-65535", "listen");
@@ -400,7 +400,7 @@

  // Parse the host name.  A name has up to three components, the
  // hostname, a port number, or two numbers representing a port
-    // range.   "www.sun.com:8080-9090" is a valid host name.
+    // range.   "www.example.com:8080-9090" is a valid host name.

  // With IPv6 an address can be 2010:836B:4179::836B:4179
  // An IPv6 address needs to be encl

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Chris Hegarty]

> On 5 Nov 2018, at 18:52, Xuelei Fan  wrote:
> 
> Hi Chris and Sean,
> 
> There are a few feedback for the CSR approval.  I updated to use 
> Optional for the returned value.  For more details, please refer 
> to:
>  https://bugs.openjdk.java.net/browse/JDK-8213161
>  http://cr.openjdk.java.net/~xuelei/8212261/webrev.03/

Looks ok to me.

-Chris.

Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Chris Hegarty]

On 05/11/18 15:59, Alan Bateman wrote:

On 05/11/2018 13:05, Langer, Christoph wrote:


...

I think you'll need to do a write-up of the overall proposal so that 
folks can jump in and point out the implications. It's not easy to do 
this in a code review of a small piece of the solution.


Right, that's the main motivation for my previous questions. It is good
to get a *complete* view of what is intended before reviewing the code.
( Sorry, if I've missed some of the previous context ).

-Chris.

Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Chris Hegarty]

On 05/11/18 12:54, Langer, Christoph wrote:

Hi Chris,

yes, there's no impact on Java SE with this item. No API is changed. I've set the scope 
to JDK, as it affects the features that are available with the "jar" filesystem 
provider from module jdk.zipfs.
...


The reason I asked about the CSR scope clarification is that it was
unclear to me what the ultimate intentions are, given that the previous
mails ( linked to from the CSR ) did have Java SE API changes ( in the
java.util.zip package ).

Are you now happy to reduce the scope of the proposal to be confined to
the "jar" filesystem provider, or is this more of an initial step
towards ultimately adding new Java SE APIs, to zip, to access these
permissions ( as was in previous emails )? As well as possibly updating
the tools to add such?

-Chris.

Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Chris Hegarty]

Hi Christoph,

On 05/11/18 07:32, Langer, Christoph wrote:

..

CSR: https://bugs.openjdk.java.net/browse/JDK-8213082


Can you please add a `Scope` value to the CSR?

I can't quite tell, but I assume it should be `JDK` or
`Implementation`, right? I want to clarify that there is
no impact on Java SE.

-Chris.

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Chris Hegarty]

Thanks for the updates Xuelei, some minor comments inline..

> On 1 Nov 2018, at 23:42, Xuelei Fan  wrote:
> 
> On 11/1/2018 11:24 AM, Sean Mullan wrote:
>> On 10/31/18 11:52 AM, Chris Hegarty wrote:
>>> Xuelei,
>>> 
>>> On 30/10/18 20:55, Xuelei Fan wrote:
>>>> Hi,
>>>> 
>>>> For the current HttpsURLConnection, there is not much security parameters 
>>>> exposed in the public APIs.  An application may need richer information 
>>>> for the underlying TLS connections, for example the negotiated TLS 
>>>> protocol version.
>>>> 
>>>> Please let me know if you have concerns to add a new method 
>>>> HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, 
>>>> by the end of Nov. 2, 2018.
>>>> 
>>>> Here is the proposal:
>>>>  https://bugs.openjdk.java.net/browse/JDK-8213161
>> Are there any security issues associated with returning the SSLSession, 
>> since it is mutable?
> It should be fine.  The update APIs of the session (invalidating, bind 
> values) does not impact the connection.

Alternatively, as is done in the new HTTP Client, an immutable
SSLSession instance can be returned.

>> + *   SHOULD override this method with appropriate 
>> implementation.
>> s/appropriate/an appropriate/
>> I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is 
>> more common in RFCs. I don't see that much in javadocs.
>> + * @implNote The JDK Reference Implementation supports this operation.
>> + *   As an application may have to use this operation for more
>> + *   security parameters, it is recommended to support this
>> + *   operation in all implementations.
>> I think it should be obvious that the JDK implementation would override this 
>> method so not sure that first sentence is necessary. The other sentence 
>> seems like it could be combined with the previous sentence, ex:
>> "Subclasses should override this method with an appropriate implementation 
>> since an application may need to access additional parameters associated 
>> with the SSL session."
> Updated accordingly, in the CSR and webrev:
>https://bugs.openjdk.java.net/browse/JDK-8213161 
> <https://bugs.openjdk.java.net/browse/JDK-8213161>
The CSR looks good. I made a few minor edits to the verbiage
and added myself as reviewer.

The title will need to be updated to reflect the addition of the
new method in SecureCacheResponse. Maybe:

"Add SSLSession accessors to HttpsURLConnection and
SecureCacheResponse"

-Chris.

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Chris Hegarty]

> On 31 Oct 2018, at 20:03, Xuelei Fan  wrote:
>> ...
> Yes.  I had the same concern about the optional operation.  However, I came 
> to a different conclusion if I'm from viewpoint of these users that need to 
> use this new operation.
> 
> If an application have to use this new operation, for example to access the 
> negotiated TLS version, this operation is essential to it. Unfortunately, 
> because of compatibility impact, we cannot force all implementation supports 
> this new added operation.  It is a potential problem to those applications 
> that need it.
> 
> It would be nice if an implementation can support this operation as soon as 
> possible.  If we just add the operation, providers may not aware there is a 
> need to update their implementation.  Unfortunately. there is not much we can 
> do to notify the provider.
> 
> If we deprecated the duplicated methods, it is easier for providers to catch 
> up with this new added operation, and move forward to support it. As the 
> deprecating will show up building warnings, or even error if run in strict 
> building mode.

I have no issue with the addition of the new method to access the
SSLSession. Unfortunately, due to the evolutionary constraints of this
API, the `getSSLSession` method will likely need to be specified to
throw UOE. The actual JDK's HTTPS protocol handler implementation will
not throw UOE.

Clearly there is a desire for non-JDK HTTPS protocol handler
implementations to quickly determine the need to update their code and
override the default implementation of `getSSLSession` ( to do something
useful ), hence the request to deprecate the the existing individual
security parameter accessor methods.

I don't believe that deprecating these individual security parameter
accessors is a good idea for the following reasons:

1) Deprecated warnings are only issued at compile-time, so only HTTPS
   protocol handler implementations being recompiled may encounter the
   warnings. Existing binary artifacts will not.

2) More importantly. Forever more, developers wanting access to say,
   the peer principle or the server's certificate chain, will be
   encouraged to get them through an optional API ( rather than a
   non-optional API ). This increases the risk that the code will
   encounter an UOE. I don't think that this increased risk is justified
   by the desire to not-have-two-ways-to-do-the-same-thing. I would
   agree if this were a new API, but it is not.

HTTPS protocol handler implementations actively being maintained will
likely quickly get requests from their users to provide a better
implementation of this new method, as folk move towards TLS 1.3, etc.
Maybe such requests will be sufficient to help adoption, at which point
the deprecation of these methods could then be re-considered.

-Chris.

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Chris Hegarty]

Xuelei,

On 30/10/18 20:55, Xuelei Fan wrote:

Hi,

For the current HttpsURLConnection, there is not much security 
parameters exposed in the public APIs.  An application may need richer 
information for the underlying TLS connections, for example the 
negotiated TLS protocol version.


Please let me know if you have concerns to add a new method 
HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, 
by the end of Nov. 2, 2018.


Here is the proposal:
     https://bugs.openjdk.java.net/browse/JDK-8213161

Thanks,
Xuelei


The new method looks fine.

On the deprecation, minimally the annotation should contain
the "since" element, which will have a value of `12`.

Also, I wonder, now that I see the spec, whether or not it is
actually a good idea to deprecate these methods. The reason
I ask this is that the new method, getSSLSession, can throw
UOE, which effectively makes it an optional method. Access
to the specific security parameters provided by the existing
methods is non-optional. This is a clear difference, and
possibly a reason, for folk not interested in the "new"
parameters, to continue to use the existing methods.

-Chris

Re: RFR[12] JDK-8211978: move testlibrary/jdk/testlibrary/SimpleSSLContext.java and testkeys to network testlibrary

[Chris Hegarty]

On 15/10/18 11:07, sha.ji...@oracle.com wrote:

Please review the updated webrev:
http://cr.openjdk.java.net/~jjiang/8211978/webrev.01/
AbstractSSLTubeTest.java and FlowTest.java now use the same 
internal.net.http.SimpleSSLContext.java


This looks good to me. Thanks John.

Trivially, can you please fix an existing line length issue
in ManyRequests2.java and ManyRequestsLegacy.java? Where the
@summary and @run lines could be split better across a few
lines ( no need to regenerate the webrev ).

-Chris.

Re: RFR[12] JDK-8211978: move testlibrary/jdk/testlibrary/SimpleSSLContext.java and testkeys to network testlibrary

[Chris Hegarty]

On 15/10/18 10:53, Weijun Wang wrote:

This is bad.


In the absence of better test library, module-patching,
and jtreg support, this is what we end up with. It's
not too bad.


Maybe we can duplicate the ASCII-fied data. I don't have a strong opinion now.


Duplicating the testkeys, regardless of ASCII-ification
seems worse than the duplication of this small trivial
implementation class.

-Chris.


--Max


On Oct 15, 2018, at 5:21 PM, Daniel Fuchs  wrote:

Hi Max,

These tests are whitebox tests - the tests classes are patched
into the java.net.http module, and as a consequence they can't
compile against library classes which are on the classpath.
This is the unfortunate reason for the presence of a
SimpleSSLContext clone in there.

best regards,

-- daniel

On 15/10/2018 03:53, Weijun Wang wrote:

Hi John
I see testkeys is directly referenced in AbstractSSLTubeTest.java and 
FlowTest.java by an inner class also named SimpleSSLContext. Is it very 
different from the one in SimpleSSLContext.java? Can they be combined?
If so, then we have a chance to ASCII-fy the testkeys file and directly include 
it inside SimpleSSLContext.java. This way we can delete one more binary file 
and do not need to care about file permissions. What a marvelous gain would 
that be!
Thanks
Max

Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Chris Hegarty]

Matthias,

> On 9 Aug 2018, at 08:07, Baesken, Matthias  wrote:
> 
>> Did we get to a conclusion on whether to have central infrastructure to
>> read/parse the security property? As I recall, this one was originally
>> proposed before the generalization of the networking solution. There are
>> details related to trimming that would be better not to duplicate if
>> possible.
> 
> Hi Alan / Chris ,
> 
> I am aware of  
> 
> https://bugs.openjdk.java.net/browse/JDK-8207846
> 
> where jdk.net.includeInExceptions   has been generalized to be used for other 
> use cases than  exceptions in the networking area .
> This  has been pushed in the meantine.
> 
> Could you point me to the other discussion, is there already a webrev posted 
> for this ?

Given that 8207846 was being targeted to JDK 11, during RDP 1,
it was decided to keep the changes as minimal as possible. The
supporting implementation, that reads and parses the property,
remains in src/java.base/share/classes/sun/net/util/SocketExceptions.java.
Alan referred to this in one of the review comments for 8207846 [1].
If this mechanism is to be used outside of the networking area,
and I think it can, then the implementation in SocketExceptions
should probably be moved to some other more appropriate
internal package.

>> 
>> Also, I think one of my comments on the original patch is that we should
>> get the style and line lengths a bit more consistent with the existing
>> code (the patch added excessively long lines for example).
>> 
> 
> Sure I'll look into it !


-Chris.

[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054501.html

> Best regards, Matthias
> 
> 
>> -Original Message-
>> From: Alan Bateman 
>> Sent: Mittwoch, 8. August 2018 20:30
>> To: Baesken, Matthias ; Chris Hegarty
>> 
>> Cc: core-libs-...@openjdk.java.net; Lindenmaier, Goetz
>> ; Langer, Christoph
>> ; OpenJDK Dev list > d...@openjdk.java.net>
>> Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
>> parsing of jar archives
>> 
>> On 07/08/2018 16:00, Baesken, Matthias wrote:
>>> Ping   😊  , any reviews / comments ?
>> Did we get to a conclusion on whether to have central infrastructure to
>> read/parse the security property? As I recall, this one was originally
>> proposed before the generalization of the networking solution. There are
>> details related to trimming that would be better not to duplicate if
>> possible.
>> 
>> Also, I think one of my comments on the original patch is that we should
>> get the style and line lengths a bit more consistent with the existing
>> code (the patch added excessively long lines for example).
>> 
>> -Alan.

Re: CSR Review: 8208641: SSLSocket should throw an exception when configuring DTLS

[Chris Hegarty]

Ok, thanks. Looks like the “Default” case is not-an-issue.

-Chris.

> On 8 Aug 2018, at 22:29, Xuelei Fan  wrote:
> 
> The "Default" algorithm defined in the SunJSSE provider is for TLS protocols.
> 
> Xuelei
> 
> On 8/8/2018 1:28 PM, Sean Mullan wrote:
>> On 8/8/18 1:58 PM, Anthony Scarpino wrote:
>>> I don't see where your example of getDefault() is any different than the 
>>> current code path.  A user has to define a SSLContext when using 
>>> getDefault() which as far as I can tell goes through the code path of 
>>> SSLContext.get[Server]SocketFactory()
>> SSL{Server}SocketFactory.getDefault() says: "Otherwise, this method returns 
>> SSLContext.getDefault().get{Server}SocketFactory(). If that call fails, an 
>> inoperative factory is returned."
>> So it doesn't look like we have to do anything special for this case, but as 
>> part of this fix, we should check/test that an inoperative factory is 
>> returned if a DTLS context is the default.
>> --Sean
>>> 
>>> Tony
>>> 
>>> On 08/08/2018 08:52 AM, Chris Hegarty wrote:
>>>> +1 to everything Sean said, and just to add ...
>>>> 
>>>> This change will prevent an SSLContext from giving out socket
>>>> factories when it has been configured with DTLS. What about
>>>> SSL[Server]SocketFactory::getDefault when the default SSL
>>>> context is DTLS? I don’t see that getDefault can throw an UOE,
>>>> should it?Or should / could this be resolved at the socket
>>>> factory level, when trying to create new sockets rather than at
>>>> the factory retrieval time?
>>>> 
>>>> -Chris.
>>>> 
>>>>> On 8 Aug 2018, at 15:46, Sean Mullan  wrote:
>>>>> 
>>>>> On 8/7/18 8:05 PM, Xuelei Fan wrote:
>>>>>> Hi Tony,
>>>>>> The Specification section looks more like the implementation details. We 
>>>>>> may change the implementation details again in the future.  It may be 
>>>>>> more suitable to move it to the Solution section, or just remove it.
>>>>> 
>>>>> I agree, I would omit the diffs and put N/A for the Specification section.
>>>>> 
>>>>>> In the Specification section, I may just say something like, "No APIs 
>>>>>> changes.  The SunJSSE provider is updated to throw 
>>>>>> UnsupportedOperationException if SSLContext.SSLServerSocketFactory() or 
>>>>>> SSLContext.SSLSocketFactory() get called for DTLS algorithms SSLContext".
>>>>> 
>>>>> I think the CSR should also say that the SunJSSE implementation of the 
>>>>> engineGetSocketFactory and engineGetServerSocketFactory methods of 
>>>>> SSLContextSpi have been changed to throw UnsupportedOperationException. 
>>>>> That is the specific API behavior change.
>>>>> 
>>>>> A few other comments on the CSR:
>>>>> 
>>>>> "SSLContext.getSSLSocketFactory() and 
>>>>> SSLContext.getSSLServerSocketFactory()"
>>>>> 
>>>>> Typo, there is no "SSL" in the method names.
>>>>> 
>>>>> The Compatibility Risk field has several typos ("there" -> "their", "how 
>>>>> -> now", ...) and is a bit hard to understand. Wasn't TLS being used 
>>>>> before instead of DTLS in certain scenarios? Because the API silently 
>>>>> passed in certain cases, and now we will be throwing an Exception, maybe 
>>>>> it might be better to say the risk is low.
>>>>> 
>>>>> In the Summary section, it says "..., but it is not documented." I 
>>>>> suggest opening a docs bug to improve the DTLS documentation so that it 
>>>>> is more clear this scenario is not supported.
>>>>> 
>>>>> I think the Interface Kind should be Java API since we are changing the 
>>>>> behavior of an implementation of a standard API. I asked Joe Darcy this 
>>>>> question yesterday, and he agreed.
>>>>> 
>>>>> --Sean
>>>>> 
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>> On 8/7/2018 4:14 PM, Anthony Scarpino wrote:
>>>>>>> Hi Xuelei,
>>>>>>> 
>>>>>>> I have updated the csr and I believe I have addressed your comments.
>>>>>>

Re: CSR Review: 8208641: SSLSocket should throw an exception when configuring DTLS

[Chris Hegarty]

+1 to everything Sean said, and just to add ...

This change will prevent an SSLContext from giving out socket
factories when it has been configured with DTLS. What about
SSL[Server]SocketFactory::getDefault when the default SSL
context is DTLS? I don’t see that getDefault can throw an UOE,
should it?Or should / could this be resolved at the socket 
factory level, when trying to create new sockets rather than at
the factory retrieval time?

-Chris.

> On 8 Aug 2018, at 15:46, Sean Mullan  wrote:
> 
> On 8/7/18 8:05 PM, Xuelei Fan wrote:
>> Hi Tony,
>> The Specification section looks more like the implementation details. We may 
>> change the implementation details again in the future.  It may be more 
>> suitable to move it to the Solution section, or just remove it. 
> 
> I agree, I would omit the diffs and put N/A for the Specification section.
> 
>> In the Specification section, I may just say something like, "No APIs 
>> changes.  The SunJSSE provider is updated to throw 
>> UnsupportedOperationException if  SSLContext.SSLServerSocketFactory() or 
>> SSLContext.SSLSocketFactory() get called for DTLS algorithms SSLContext".
> 
> I think the CSR should also say that the SunJSSE implementation of the 
> engineGetSocketFactory and engineGetServerSocketFactory methods of 
> SSLContextSpi have been changed to throw UnsupportedOperationException. That 
> is the specific API behavior change.
> 
> A few other comments on the CSR:
> 
> "SSLContext.getSSLSocketFactory() and SSLContext.getSSLServerSocketFactory()"
> 
> Typo, there is no "SSL" in the method names.
> 
> The Compatibility Risk field has several typos ("there" -> "their", "how -> 
> now", ...) and is a bit hard to understand. Wasn't TLS being used before 
> instead of DTLS in certain scenarios? Because the API silently passed in 
> certain cases, and now we will be throwing an Exception, maybe it might be 
> better to say the risk is low.
> 
> In the Summary section, it says "..., but it is not documented." I suggest 
> opening a docs bug to improve the DTLS documentation so that it is more clear 
> this scenario is not supported.
> 
> I think the Interface Kind should be Java API since we are changing the 
> behavior of an implementation of a standard API. I asked Joe Darcy this 
> question yesterday, and he agreed.
> 
> --Sean
> 
>> Thanks,
>> Xuelei
>> On 8/7/2018 4:14 PM, Anthony Scarpino wrote:
>>> Hi Xuelei,
>>> 
>>> I have updated the csr and I believe I have addressed your comments.
>>> 
>>> thanks
>>> 
>>> Tony
>>> 
>>> On 08/07/2018 01:43 PM, Xuelei Fan wrote:
 Hi Tony,
 
 Would you mind make it clear that this impact the JDK JSSE provider only?  
 Third party's provider may be able to support DTLS with SSLSocket.
 
 I think there may be no specification change.  The 
 SSLContext.getServerSocketFactory() and SSLContext.getSocketFactory() 
 defines the spec if the algorithm is not supported by the underlying 
 provider, "UnsupportedOperationException - if the underlying provider does 
 not implement the operation.".  I may prefer to make it clear that this is 
 just a behavior change of the JDK JSSE provider (SunJSSE).  The SunJSSE 
 provider now throws UnsupportedOperationException for creating 
 SSL(Server)SocketFactory with DTLS SSLContext, because it does not 
 actually support DTLS SSLSocket.
 
 In Solution section, "Throwing a UnsupportedOperationException when 
 getting a socket from the SSLServerSocketFactory or SSLSocketFactory for 
 DTLS."   I guess you meant, throwing a UOE when calling 
 SSLContext.getServerSocketFactory() and SSLContext.getSocketFactory()?
 
 Thanks,
 Xuelei
 
 On 8/7/2018 12:17 PM, Anthony Scarpino wrote:
> I need a review of a CSR for SSLSocket should throw an exception when 
> configuring DTLS.  We are targeting this for 12 right now.
> 
> https://bugs.openjdk.java.net/browse/JDK-8209031
> 
> thanks
> 
> Tony
> 
>>> 

Re: RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

Thanks for the review Sean,

> On 23 Jul 2018, at 16:58, Sean Mullan  wrote:
> ...
>>   http://cr.openjdk.java.net/~chegar/8207846/webrev.00/
> 
> A few nits and wording suggestions in the java.security file:
> 
> "By default, several exception messages do not include potentially sensitive 
> information such as file names, host names, or port numbers."
> 
> I think the following sounds a bit better:
> 
> "By default, exception messages should not include potentially sensitive
> information such as file names, host names, or port numbers."
> 
> Also, the 2nd and 3rd sentences basically say the same thing. I would remove 
> the 2nd sentence.
> 
> "The categories, to enable enhanced exception message information, are:"
> 
> I would remove ", to enable enhanced exception message information," since it 
> seems redundant (and I believe is grammatically incorrect).
> 
> hostInfo - IOExceptions thrown by java.net.Socket and also the ...
> 
> Remove "also" (not really necessary).

Agreed. Here’s where this ended up.

#
# Enhanced exception message information
#
# By default, exception messages should not include potentially sensitive
# information such as file names, host names, or port numbers. This property
# accepts one or more comma separated values, each of which represents a
# category of enhanced exception message information to enable. Values are
# case-insensitive. Leading and trailing whitespaces, surrounding each value,
# are ignored. Unknown values are ignored.
#
# The categories are:
#
#  hostInfo - IOExceptions thrown by java.net.Socket and the socket types in the
# java.nio.channels package will contain enhanced exception
# message information
#
# The property setting in this file can be overridden by a system property of
# the same name, with the same syntax and possible values.
#
#jdk.includeInExceptions=hostInfo

-Chris

Re: Bug in HttpClient

[Chris Hegarty]

The following issue has been filed in JIRA to track the problem with 
an HTTP/1.0 response without a Content-Length header:
  
  https://bugs.openjdk.java.net/browse/JDK-8207966

-Chris.

> On 20 Jul 2018, at 08:38, Severin Gehwolf  wrote:
> 
> Adding net-dev
> 
> On Fri, 2018-07-20 at 08:52 +0200, Thomas Lußnig wrote:
>> Hi,
>> i found an bug in JDK 10 with the new HttpClient. It does not handle
>> responses wihtout contentlength correctly.
>> Normally i would expect that the content is returned even without
>> content length. Since i can not open an JDK bug
>> i hope some person from the list can do it. Below is an example that
>> show the problem.
>> 
>> Gruß Thomas Lußnig
>> import java.io.InputStream;
>> import java.io.OutputStream;
>> import java.net.InetSocketAddress;
>> import java.net.ServerSocket;
>> import java.net.Socket;
>> import java.net.URI;
>> import java.time.Duration; 
>> import javax.net.ServerSocketFactory;
>> import jdk.incubator.http.HttpClient;
>> import jdk.incubator.http.HttpRequest;
>> import jdk.incubator.http.HttpResponse; 
>> public class Client1 {
>>   static void server(final boolean withContentLength) {
>> try(ServerSocket ss =
>> ServerSocketFactory.getDefault().createServerSocket()) {
>>ss.setReuseAddress(true);
>>ss.bind(new InetSocketAddress("127.0.0.1",80));
>>final byte[] buf = new byte[120400];
>>try(Socket s = ss.accept()) {
>>  System.out.println("Accepted:
>> "+s.getRemoteSocketAddress());
>>  try(  OutputStream os =
>> s.getOutputStream(); InputStream is = s.getInputStream()) {
>> is.read(buf);
>> is.read(buf);
>> os.write("HTTP/1.0 200
>> OK\r\nConnection: close\r\nContent-Type: text/xml; charset=UTF-
>> 8\r\n".getBytes());
>> if(withContentLength)
>> os.write("Content-Length: 4\r\n".getBytes());
>> os.write("\r\n".getBytes());
>> os.write("".getBytes());
>> os.flush();
>>  }
>>}
>> } catch(final Throwable t) { t.printStackTrace(); }
>>  }
>>   static void client() {
>> try {
>>final HttpClient client =
>> HttpClient.newBuilder().version(HttpClient.Version.HTTP_2).build();
>>final HttpResponse response = client
>>.send(HttpRequest.newBuilder(new URI("htt
>> p://127.0.0.1/test")).timeout(Duration.ofMillis(120_000))
>> 
>> .POST(HttpRequest.BodyPublisher.fromString("body")).build(),
>> HttpResponse.BodyHandler.asString());
>>System.out.println("Received reply: " +
>> response.statusCode());
>>System.out.println("Received body: " +
>> response.body());
>> } catch(final Throwable t) { t.printStackTrace(); }
>>  }
>>public static void main(final String[] args) throws Exception
>> {
>> new Thread(()->server(true)).start();
>> client();
>> new Thread(()->server(false)).start();
>> client();
>>   }
>> }

Re: RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

Sean,

> On 20 Jul 2018, at 18:07, Sean Mullan  wrote:
> 
> On 7/20/18 11:08 AM, Chris Hegarty wrote:
>> This is ambiguous, and needs to be clarified. Surely, it is
>> better to use the same wording as the serial filter:
>>  "Whitespace is significant and is considered part of the value."
> 
> Kind of on the fence on that one. If this were a general property/value 
> format, I would agree, but these values are fixed and not potentially 
> complicated expressions.

Sure, they are simple strings consisting of only printable characters.

> For example, does this mean:
> 
> jdk.includeInExceptions=hostInfo,jarInfo
> 
> and
> 
> jdk.includeInExceptions=hostInfo, jarInfo
> 
> are different? And I assume the latter " jarInfo" would be ignored?
> 
> If you are strongly in favor of this, I would highly recommend logging a 
> warning when there is an unknown value, otherwise typos and such would be 
> hard to detect (although this doesn't necessarily need to be in the 
> specification).

My original whitespace handling avoided the potential issue where a 
rogue lead or trailing whitespace accidentally found its way into a
value, therefore avoiding the scenario above ( and potentially emitting
a warning ).

Whitespace is significant and should be specified as such, since ...
  
  jdk.includeInExceptions=“host   Info” 
IS NOT equal to
  jdk.includeInExceptions=“hostInfo”
and system property values can contain spaces
  jdk.includeInExceptions=“foo bar,hostInfo,jar Info,” 

After given this some more thought, I now think that I gave in to the
comment to change whitespace handing too easy. While maybe not
consistent, with the already inconsistent, whitespace handling in
java.security, I think ( for this particular case ) the original - trim
leading and trailing - is the right thing to do. It avoids your above
scenario where someone accidentally adds a leading space, which could be
difficult to debug/find without a warning - which we should avoid if
possible.

I’d like to re-propose the original webrev for consideration ( whitespace
handling is the only change ):

  http://cr.openjdk.java.net/~chegar/8207846/webrev.00/


-Chris.

Re: RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

> On 20 Jul 2018, at 16:15, Roger Riggs  wrote:
> 
> Hi Chris,
> 
> The updated text is fine.
> Thanks for your consideration.

Updated webrev as discussed.
  http://cr.openjdk.java.net/~chegar/8207846/webrev.01/

-Chris.

Re: RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

Roger,

> On 20 Jul 2018, at 15:36, Roger Riggs  wrote:
> 
> Hi Chris,
> 
> It is important to be clear about how whitespace is treated and within the 
> java.security file
> there are other uses that explicitly define how whitespace is used.

Right, and the usages are already inconsistent. Nothing we can
do about that now.

> I am more concerned about how command line properties are understood and used 
> how we have to document them.
> Allowing whitespace quickly gets bogged down in how shells handle quotes, 
> telling people they have to
> quote them and when/whether you have to quote the quotes.

You cannot disallow whitespace, simple ignore them or consider
them part of the value.

> Having a consistent treatment of command line and security properties keeps 
> the
> story simple and easier to support.

This file is already inconsistent, trimming happens in some cases.
Whitespaces are either trimmed, ignored, or considered as like
any other character.

> The jdk.serialFilter property had the same issue and is explicit in the 
> java.security file
> that spaces are just another character and are not treated specially.

This is a reasonable position.

> Its a slippery slope, if we start compensating/ignoring whitespace in some 
> properties
> then we will have to keep explaining how some are treated differently.
> I would keep the original non-whitespace description.

Original: "This property may be set to one or more values,
separated by commas, and with no white-space”

This is ambiguous, and needs to be clarified. Surely, it is
better to use the same wording as the serial filter:

 "Whitespace is significant and is considered part of the value."

> Case-insensistive compares are another slippery slope but make a bit more 
> sense for usability.

The complete updated text:

#
# Enhanced exception message information
#
# By default, several exception messages do not include potentially sensitive
# information such as file names, host names, or port numbers. This property may
# be used to enable categories of enhanced information in exception messages.
# The property accepts one or more comma separated values, each of which
# represents a category of enhanced exception message information to enable.
# Values are case-insensitive. Whitespace is significant and is considered part
# of the value. Unknown values are ignored.
#
# The categories, to enable enhanced exception message information, are:
#
#  hostInfo - IOExceptions thrown by java.net.Socket and also the socket types
# in the java.nio.channels package will contain enhanced exception
# message information
#
# The property setting in this file can be overridden by a system property of
# the same name, with the same syntax and possible values.
#
#jdk.includeInExceptions=hostInfo

-Chris.

Re: RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

Roger,

> On 20 Jul 2018, at 14:52, Roger Riggs  wrote:
> 
> Hi Chris,
> 
> It is very unusual for the processing of system properties to do *any* 
> parsing except for delimiters
> including removing spaces, etc.  It complicates the handling and sets a bad 
> precedent
> that makes it more complex for users and developers to know how to set 
> property values.
> The whitespace trimming should be removed.

The addition of the whitespace trimming was to clarify the intent of
the existing wording regarding whitespace. It was not intended to
set a precedent, good or bad.

Regardless of any subjective opinion, whitespace are allowable
within system property values, so it’s a matter of whether or not
we want to deal with them explicitly or just leave it as an
implementation detail.

I’m ok to drop the trimming if you feel strongly about it. But just
to note that this now diverges even more from the original.

-Chris.

> $.02, Roger
> 
>>   http://cr.openjdk.java.net/~chegar/8207846/webrev.00/
>> 
>> -Chris.
>> 
>> P.S. It appears that jtreg does not support quoted system property values
>> with spaces on the @run line. I’ll file an issue against jtreg for this.
>> 
> 

RFR [11] 8207846: Generalize the jdk.net.includeInExceptions security property

[Chris Hegarty]

JDK-8204233 added a new security property, `jdk.net.includeInExceptions`,
to include additional, potentially security sensitive, information in
exception detail messages in the networking area. The property accepts a
comma separated list of values that specifies the particular type of
extra detail information to add.

Since its addition, in JDK 11, further uses have arisen to include
additional, potentially security sensitive, information in exception
detail messages in other areas, namely the java.util.jar APIs. See
JDK-8205525, and 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054284.html

Given that this mechanism will likely be used more generally across
different parts of the platform, it seem prudent to rename the property
to be less area-specific, thus allowing for additional argument values
to be specified, like for example `jarPath`.

The following are the suggested changes to the java.security file:

$ hg extdiff -p diff -o -C1 src/java.base/share/conf/security/java.security
*** 1062,1074 
  
  #
! # Enhanced exception message text
  #
! # By default, socket exception messages do not include potentially sensitive
! # information such as hostnames or port numbers. This property may be set to 
one
! # or more values, separated by commas, and with no white-space. Each value
! # represents a category of enhanced information. Currently, the only category 
defined
! # is "hostInfo" which enables more detailed information in the IOExceptions
! # thrown by java.net.Socket and also the socket types in the 
java.nio.channels package.
! # The setting in this file can be overridden by a system property of the same 
name
! # and with the same syntax and possible values.
! #jdk.net.includeInExceptions=hostInfo
--- 1062,1084 
  
+ 
+ #
+ # Enhanced exception message information
+ #
+ # By default, several exception messages do not include potentially sensitive
+ # information such as file names, host names, or port numbers. This property 
may
+ # be used to enable categories of enhanced information in exception messages.
+ # The property accepts one or more comma separated values, each of which
+ # represents a category of enhanced exception message information to enable.
+ # Values are case-insensitive. Leading and trailing whitespaces, surrounding
+ # each value, are ignored. Unknown values are ignored.
+ #
+ # The categories, to enable enhanced exception message information, are:
+ #
+ #  hostInfo - IOExceptions thrown by java.net.Socket and also the socket types
+ #  in the java.nio.channels package will contain enhanced 
exception
+ #  message information
  #
! # The property setting in this file can be overridden by a system property of
! # the same name, with the same syntax and possible values.
  #
! #jdk.includeInExceptions=hostInfo


Full webrev:
  http://cr.openjdk.java.net/~chegar/8207846/webrev.00/

-Chris.

P.S. It appears that jtreg does not support quoted system property values
with spaces on the @run line. I’ll file an issue against jtreg for this.

RFR [11] 8205671: Remove HTTP Client tests erroneously problem listed by the TLS 1.3 integration

[Chris Hegarty]

Seems that the integration of TLS 1.3 erroneously added a number of HTTP Client 
tests to the ProblemList. Previous to the TLS 1.3 push, work was done to ensure 
that the HTTP Client tests ran successfully with the changes in the TLS 1.3 
branch. These tests should not have been problem listed.

diff --git a/test/jdk/ProblemList.txt b/test/jdk/ProblemList.txt
--- a/test/jdk/ProblemList.txt
+++ b/test/jdk/ProblemList.txt
@@ -561,22 +561,6 @@
 
 java/net/DatagramSocket/SendDatagramToBadAddress.java   7143960 
macosx-all
 
-java/net/httpclient/ConcurrentResponses.java8204977 
generic-all
-java/net/httpclient/DigestEchoClientSSL.java8204977 
generic-all
-java/net/httpclient/EncodedCharsInURI.java  8204977 
generic-all
-java/net/httpclient/FlowAdapterSubscriberTest.java  8204977 
generic-all
-java/net/httpclient/ImmutableFlowItems.java 8204977 
generic-all
-java/net/httpclient/ManyRequests.java   8204977 
generic-all
-java/net/httpclient/ManyRequests2.java  8204977 
generic-all
-java/net/httpclient/SplitResponseSSL.java   8204977 
generic-all
-java/net/httpclient/http2/BasicTest.java8204977 
generic-all
-java/net/httpclient/http2/ContinuationFrameTest.java8204977 
generic-all
-java/net/httpclient/ProxyAuthDisabledSchemesSSL.java8204977 
generic-all
-java/net/httpclient/RequestBodyTest.java8204977 
generic-all
-java/net/httpclient/SmokeTest.java  8204977 
generic-all
-java/net/httpclient/CancelledResponse.java  8204977 
solaris-all
-java/net/httpclient/InvalidSSLContextTest.java  8204980 
generic-all
-
 
 
 # jdk_nio


-Chris.

Re: [RFR] 8184328: JDK8u131 socketRead0 hang at SSL read

[Chris Hegarty]

Xuelei,

Without diving deeper into this issue, Rob’s suggested approach seems 
reasonable to me, and better than existing out-of-the-box behaviour. I’m not 
sure what issues you are thinking of, with using the read timeout in 
combination with a retry mechanism, in this manner? If the network is so slow, 
surely there will be other issues with connecting and reading, why is closing 
any different.

-Chris.

> On 13 Sep 2017, at 16:52, Rob McKenna  wrote:
> 
> Hi Xuelei,
> 
> This behaviour is already exposed via the autoclose boolean in:
> 
> https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLSocketFactory.html#createSocket-java.net.Socket-java.io.InputStream-boolean-
> 
> My position would be that allowing 5 retries allows us to say with some
> confidence that we're not going to get a close_notify from the server.
> If this is the case I think its reasonable to close the connection.
> 
> W.r.t. a separate timeout, the underlying mechanics of a close already
> depend on the readTimeout in this situation. (waiting on a close_notify
> requires performing a read so the read timeout makes sense in this
> context) I'm happy to alter that but I think that the combination of
> a timeout and a retry count is straightforward and lower impact.
> 
> In my opinion the default behaviour of potentially hanging indefinitely
> is worse than the alternative here. (bearing in mind that we are closing
> the underlying socket)
> 
> I'll file a CSR as soon as we settle on the direction this fix will
> take.
> 
>-Rob
> 
> On 13/09/17 05:52, Xuelei Fan wrote:
>> In theory, there are intermittent compatibility problems as this update may
>> not close the SSL connection over the existing socket layer gracefully, even
>> for high speed networking environments, while the underlying socket is
>> alive.  The impact could be serious in some environment.
>> 
>> For safe, I may suggest turn this countermeasure off by default.  And
>> providing options to turn on this countermeasure:
>> 1. Close the SSL connection gracefully by default; or
>> 2. Close the SSL connection after a timeout.
>> 
>> It's hardly to say 5 times receiving timeout is better/safer than timeout
>> once in this context.  As you have already had a system property to control,
>> you may be able to use options other than the customized socket receiving
>> timeout, so that the closing timeout is not mixed/confused/dependent on/with
>> the receiving timeout.
>> 
>> Put all together:
>> 1. define a closing timeout, for example "jdk.tls.waitForClose".
>> 2. the property default value is zero, no behavior changes.
>> 3. applications can set positive milliseconds value for the property. The
>> SSL connection will be closed in the set milliseconds (or about the maximum
>> value between SO_TIMEOUT and closing timeout), the connection is not grant
>> to be gracefully.
>> 
>> What do you think?
>> 
>> BTW, please file a CSR as this update is introducing an external system
>> property.
>> 
>> Thanks,
>> Xuelei
>> 
>> On 9/11/2017 3:29 PM, Rob McKenna wrote:
>>> Hi folks,
>>> 
>>> In high latency environments a client SSLSocket with autoClose set to false
>>> can hang indefinitely if it does not correctly recieve a close_notify
>>> from the server.
>>> 
>>> In order to rectify this situation I would like to suggest that we
>>> implement an integer JDK property (jdk.tls.closeRetries) which instructs
>>> waitForClose to attempt the close no more times than the value of the
>>> property. I would also suggest that 5 is a reasonable default.
>>> 
>>> Note: each attempt times out based on the value of
>>> Socket.setSoTimeout(int timeout).
>>> 
>>> Also, the behaviour here is similar to that of waitForClose() when
>>> autoClose is set to true, less the retries.
>>> 
>>> http://cr.openjdk.java.net/~robm/8184328/webrev.01/
>>> 
>>>-Rob
>>> 

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Chris Hegarty]

On 02/06/17 00:14, Ioi Lam wrote:

...

The gem is hidden in the compile.0.jta file. It contains something like:

  -sourcepath :/jdk/foobar/test/lib:

So if my test refers to a class under /test/lib, such as
jdk.test.lib.process.ProcessTools, javac will be able to locate it under
/jdk/foobar/test/lib/jdk/test/lib/process/ProcessTools.java, and will
build it automatically.

So really, there's no reason why the test must explicitly do an @build
of the library classes that it uses.


Sure, you're relying on the implicit compilation of dependencies
by javac. Look at the output, where it compiles the library
classes to.  It is part of the classes directory for the
individual test. That means that the library classes will need
to be compiled many many times.  The @build tag will compile
the library classes to a common output directory, where they
can be reused ( unless I'm missing something ).

-Chris.

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Chris Hegarty]

> On 1 Jun 2017, at 04:27, Felix Yang  wrote:
> 
> Hi Chris and Daniel,
> 
>new webrev with a few of explicit builds than wildcard.
> 
> http://cr.openjdk.java.net/~xiaofeya/8181299/webrev.01/

This seems very odd to me:

http://cr.openjdk.java.net/~xiaofeya/8181299/webrev.01/test/sun/security/tools/jarsigner/AltProvider.java.sdiff.html

The test is using jdk.test.lib.util.JarUtils ( which comes from the 
top-level test library ), but the @build specifies the JarUtils ( in
the unnamed package ) from the jdk test library.

Also, shouldn’t it use jdk.test.lib.compiler.CompilerUtils, rather than
CompilerUtils?

-Chris.

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Chris Hegarty]

Igor,

> On 1 Jun 2017, at 04:32, Igor Ignatyev  wrote:
> 
> Hi Felix,
> 
> I have suggested the exact opposite change[1-3] to fix the same problem.

I’m sorry, but this is all just too confusing. After your change, who, or what, 
is
responsible for building/compiling the test library dependencies?

Test library code has no @modules tags, so does not explicitly declare its
module dependencies. Instead module dependencies, required by test
library code, are declared in the test using the library. If we wildcard, or
otherwise leave broad build dependencies, from tests then there is no
way to know what new module dependencies may be added in the future.
That is, one of, the reason(s) I asked Felix to be explicit about the build
dependencies.

-Chris.

> [1] https://bugs.openjdk.java.net/browse/JDK-8181391
> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-June/048012.html
> [3] http://cr.openjdk.java.net/~iignatyev//8181391/webrev.00/index.html

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Chris Hegarty]

> On 31 May 2017, at 10:42, Felix Yang  wrote:
> 
> Hi there,
> 
>please review the patch to various jdk tests to explicitly compiling test 
> libraries and the lib's dependencies. Though it could be a jtreg issue (I 
> think so), it is necessary to get the tests running firstly.
> 
> Bug:
> 
>https://bugs.openjdk.java.net/browse/JDK-8181299
> 
> Webrev:
> 
>http://cr.openjdk.java.net/~xiaofeya/8181299/webrev.00/

This may be ok to get the tests running again, but explicit build targets
would be better. The contents, and module dependencies, from classes
in the test library are subject to change, so building all classes may 
require more modules than in the @modules tags in the test.

I agree with Daniel, each test should be run separately in a clean
environment, to verify that it can build the necessary dependencies.
This may be a straight forward way to identify explicit build dependencies
and avoid the wildcards.

-Chris.

Re: RFR 8177784 Use CounterMode intrinsic for AES/GCM

[Chris Hegarty]

On 06/04/17 21:39, Anthony Scarpino wrote:


I'd like to get a review for this performance change to use the existing
CounterMode parallelized intrinsic instead of GCTR's own version. The
two classes were nearly identical except for the doFinal() method which
doesn't belong in CounterMode.java.

I could have been more aggressive with this change, but I'm looking to
get this into 9, so I stayed away from completely merging GCTR into
CounterMode in case of incompatibilities.  All tests security and
hotspot tests pass.

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


This change looks good to me. Trivially, the class-level comment in
GCTR should be updated ( it refers to removed fields ). Also,
CounterMode.counter could be protected ( rather than package-private ).

-Chris.

Re: RFR: 8172048: Re-examine use of AtomicReference in java.security.Policy

[Chris Hegarty]

> On 2 Jan 2017, at 11:45, Claes Redestad  wrote:
> 
> On 12/31/2016 12:45 AM, David Holmes wrote:
>> I'll let you think about it so more. I'll be back in the office on Tuesday :)
> 
> After giving it some thought I think it's better to just document the need
> for some hygiene in the field declaration:
> 
> http://cr.openjdk.java.net/~redestad/8172048/webrev.02

Looks good Claes.

Minor suggestion: "For correctness COMMA care must …"

-Chris.

Re: RFR 8171340: HttpNegotiateServer/java test should not use system proxy on Mac

[Chris Hegarty]

On 16/12/16 03:43, Wang Weijun wrote:

Please take a review at

   http://cr.openjdk.java.net/~weijun/8171340/webrev.00/

All "openConnection()" modified to "openConnection(Proxy.NO_PROXY)".


Thank you Max. Reviewed.

-Chris.

Re: [9] RFR: 8166530: sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java fails intermittently

[Chris Hegarty]

> On 7 Oct 2016, at 21:33, Artem Smotrakov  wrote:
> 
> Hello,
> 
> Please review the patch below for 
> sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java test.
> 
> The test has been observed to fail intermittently, but the failure is not 
> reproducible standalone. The patch updates the test to follow the approach 
> from SSLSocketSample.java
> 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/1f044f413e6c/test/javax/net/ssl/templates/SSLSocketSample.java
> 
> I merged OriginServer.java to ProxyAuthTest.java since only this test uses 
> that file. I also added a couple of new common methods to SSLTest.java. They 
> are not used by ProxyAuthTest.java, but can be useful in other tests.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166530
> Webrev: http://cr.openjdk.java.net/~asmotrak/8166530/webrev.00/

From my point of view, this looks ok.

-Chris.

Re: RFR[9]: JDK-8165566: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently: Address already in use

[Chris Hegarty]

On 19/09/16 09:10, John Jiang wrote:

Hi,
Please review this fix for JDK-8165566 on test
sun/security/ssl/SocketCreation/SocketCreation.java.
This test takes multiple servers to use one port in sequence, but the
port may not be released by previous server.
This patch takes every server to be allocated a free port.

Webrev: http://cr.openjdk.java.net/~jjiang/8165566/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8165566


Looks good John.

-Chris.

Re: [9] RFR: 8164591: sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java failed with SSLHandshakeException

[Chris Hegarty]

On 15 Sep 2016, at 02:55, Xuelei Fan  wrote:
> 
> On 9/15/2016 9:45 AM, Artem Smotrakov wrote:
>> Well, in this particular case it's not clear that it has the same issue
>> with free port (at least to me). The exception occurred on client side,
>> so it's not the case where we don't know where the handshake came from.
>> 
> ;-) Yeh, you catch the point.  But there is a free-port issue although the 
> exception stack in the bug description may be not that case.
> 
> Let's look at a scenarios:
> 1. server open a server socket and listen.
> 2. other test case connect to the server socket.
> 3. this test case try to connect to server socket.
> 4. this test case would fail as the server only accept one connections.
> 
> I did not check it very carefully, I think for #4, the exception stack can be 
> similar to the one in the bug description.
> 
> Anyway, as a free port is used, there are free-port issues.  Please consider 
> to make the enhancement in the fix.  Otherwise, you cannot avoid the 
> intermittent failure for this test case in the current testing environment.

+1.   Please remove any use of the free-port anti-pattern.

-Chris.

> Xuelei
> 
>> I can make this enhancement, but like I said I don't think it's going to
>> help, so I would like to keep debug output on.
>> 
>> Artem
>> 
>> 
>> On 09/14/2016 06:39 PM, Xuelei Fan wrote:
>>> On 9/15/2016 9:23 AM, Artem Smotrakov wrote:
 Hi Xuelei,
 
 For this one, I am not sure that it would help here since the test
 failed after it already had started handshaking.
>>> It has the same issue as a free-port is used.  We don't actually know
>>> the handshake is coming from the right client.
>>> 
>>> Xuelei
>>> 
 I would prefer to have it as a separate enhancement.
 
 Artem
 
 
 On 09/14/2016 06:19 PM, Xuelei Fan wrote:
> As you were already there, I would suggest to consider the
> SSLSocketSample.java template as the comment in JDK-8163924 review
> thread.
> 
> Thanks,
> Xuelei
> 
> On 9/15/2016 9:13 AM, Artem Smotrakov wrote:
>> Not urgent, but I would appreciate if someone can get a chance to look
>> at this.
>> 
>> Artem
>> 
>> 
>> On 09/07/2016 03:17 PM, Artem Smotrakov wrote:
>>> Sending to net-...@openjdk.java.net as well.
>>> 
>>> Artem
>>> 
>>> 
>>> On 09/07/2016 12:28 PM, Artem Smotrakov wrote:
 Hello,
 
 Please review the following patch for
 sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java
 
 The test has been observed to fail a couple of times, but it's still
 not clear why it failed because there is not much info in logs. The
 patch updates the test to enable additional debug output, so that we
 have more info if it fails next time.
 
 While looking at the test, I notices a couple of issues, but they
 don't seem to cause these intermittent failures:
 - The test sets system properties for JSSE in a loop, but JSSE
 provider reads them only once while initialization. As a result,
 only
 values which were set in the first iteration are actually used.
 - The test doesn't close files and sockets sometimes.
 
 The patch also fixed the issues above, and there are a couple
 cosmetic changes.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8164591
 Webrev: http://cr.openjdk.java.net/~asmotrak/8164591/webrev.00/
 
 Artem
>>> 
>> 
 
>> 

Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Chris Hegarty]

> On 17 Aug 2016, at 19:52, Rajan Halade  wrote:
> 
> On 8/17/16 11:36 AM, Chris Hegarty wrote:
> 
>> On 17 Aug 2016, at 18:54, Rajan Halade  wrote:
>>> sun/net/www/protocol/https tests are redundant in jdk_security3 group, 
>>> these are included in jdk_net group.
>> Yes they are, but it is very important that anyone touching TLS verify
>> that HTTPS still works. If the jdk_net tests will be run to verity
>> such changes, then this should be fine.
> Yes, these are useful tests to run with https changes. I don't think they 
> need to be duplicated in jdk_security group as jdk_security and jdk_net are 
> part of core tests and with continuous integration system in place to run all 
> tests with each changeset, this duplication is not required.
>> 
>> Out of curiosity, how many tests are in sun/net/www/protocol/https,
>> and approximately how long do they run for?
> There are 27 tests and I see each of those finish within seconds (max I saw 
> was 8 secs for ReadTimeout.java).

Ok, thanks. No objection from me.

-Chris.

Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Chris Hegarty]

> On 17 Aug 2016, at 19:52, Rajan Halade  wrote:
> 
> On 8/17/16 11:36 AM, Chris Hegarty wrote:
> 
>> On 17 Aug 2016, at 18:54, Rajan Halade  wrote:
>>> sun/net/www/protocol/https tests are redundant in jdk_security3 group, 
>>> these are included in jdk_net group.
>> Yes they are, but it is very important that anyone touching TLS verify
>> that HTTPS still works. If the jdk_net tests will be run to verity
>> such changes, then this should be fine.
> Yes, these are useful tests to run with https changes. I don't think they 
> need to be duplicated in jdk_security group as jdk_security and jdk_net are 
> part of core tests and with continuous integration system in place to run all 
> tests with each changeset, this duplication is not required.
>> 
>> Out of curiosity, how many tests are in sun/net/www/protocol/https,
>> and approximately how long do they run for?
> There are 27 tests and I see each of those finish within seconds (max I saw 
> was 8 secs for ReadTimeout.java).

Ok, thanks. No objection from me.

-Chris.

Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Chris Hegarty]

On 17 Aug 2016, at 18:54, Rajan Halade  wrote:
> 
> sun/net/www/protocol/https tests are redundant in jdk_security3 group, these 
> are included in jdk_net group.

Yes they are, but it is very important that anyone touching TLS verify
that HTTPS still works. If the jdk_net tests will be run to verity
such changes, then this should be fine.

Out of curiosity, how many tests are in sun/net/www/protocol/https,
and approximately how long do they run for? 

-Chris.

RFR [9] 8156841: sun.security.pkcs11.SunPKCS11 poller thread retains a strong reference to the context class loader

[Chris Hegarty]

The SunPKCS11 poller thread has no need of any user defined class loader,
so should set its context class loader to null before starting, so as to not
inadvertently retain a reference to the creating thread’s context class loader.

In other areas that suffered from a similar issue we changed to use an
InnocuousThread, but I cannot fully satisfy myself that this is a safe
substation here, so I opted for the safest minimal fix. A future refactoring
exercise should consider using InnocuousThread. 

diff -r 92c31ec731eb 
src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
--- a/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
Wed Aug 10 11:54:12 2016 +0100
+++ b/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
Wed Aug 10 16:32:41 2016 +0100
@@ -809,20 +809,21 @@
 }
 }
 
 // create the poller thread, if not already active
 private void createPoller() {
 if (poller != null) {
 return;
 }
 final TokenPoller poller = new TokenPoller(this);
 Thread t = new Thread(null, poller, "Poller " + getName(), 0, false);
+t.setContextClassLoader(null);
 t.setDaemon(true);
 t.setPriority(Thread.MIN_PRIORITY);
 t.start();
 this.poller = poller;
 }
 
 // destroy the poller thread, if active
 private void destroyPoller() {
 if (poller != null) {
 poller.disable();

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

-Chris.

Re: RFR 8163489: Avoid using Utils.getFreePort() in TsacertOptionTest.java test

[Chris Hegarty]

On 9 Aug 2016, at 16:37, Weijun Wang  wrote:
> 
> http://cr.openjdk.java.net/~weijun/8163489/webrev.00

Thanks Max, this looks good( one less use of the get free port anti-pattern! ).

-Chris.

Re: RFR: 8159752: Grant de-privileged module permissions by default with java.security.policy override option

[Chris Hegarty]

> On 14 Jul 2016, at 21:05, Sean Mullan  wrote:
> 
> Please review this change to the default Policy provider implementation to 
> grant de-privileged module permissions by default even when the 
> java.security.policy override option is specified or when the 
> Policy.getInstance API is used:
> 
> http://cr.openjdk.java.net/~mullan/webrevs/8159752/webrev.00/ 
> 

This makes sense. A quick skim shows nothing untoward.

> A new system-wide policy file located in 
> ${java.home}/lib/security/default.policy has been created. It contains grant 
> statements containing the permissions that need to be granted to 
> de-privileged modules. These grant statements were previously located in the 
> ${java.home}/conf/security/java.policy file and have been relocated to the 
> default.policy file.
> 
> The default.policy file is now always loaded by the default Policy provider 
> implementation (sun/security/provider/PolicyFile). It is loaded if the 
> java.security.policy '=' or '==' option is specified, and also if the 
> application uses the Policy.getInstance methods and specifies the 
> "JavaPolicy" type. If the default.policy file cannot be loaded, an 
> InternalError is thrown, on the basis that the runtime cannot operate 
> correctly unless these permissions are granted.

I think this is ok, but of course it is unnecessary for a minimal image with 
just java.base. Probably not worth complicating things, but you could 
conditionally add include the permissions per module based on its presence.

> The rationale for making this change is that the runtime should be 
> responsible for granting the permissions it needs to operate correctly. We 
> should not expect users to have to determine or copy and paste these 
> permissions into their own policy files.

Sounds reasonable.

-Chris.

Re: [9] RFR: 8134267: javax/net/ssl/TLS/TestJSSE.java fails intermittently with BindException: Address already in use

[Chris Hegarty]

On 18 May 2016, at 22:57, Artem Smotrakov  wrote:

> Hello,
> 
> Please review the following patch for javax/net/ssl/TLS/TestJSSE.java test.
> 
> The test fails intermittently with BindException because it can use a busy 
> port. The test uses jdk.testlibrary.Utils.getFreePort() which creates a 
> server socket, and returns its local port number. Then this port number is 
> used to creates a new SSL server socket. It looks like the port may be 
> already busy when a new SSL socket is being created.

Thank you for changing this. getFreePort is a bad pattern that leads to
intermittent failures. We should remove its usage wherever possible.

> The patch removes usage of jdk.testlibrary.Utils.getFreePort(). Now the SSL 
> server creates a server socket by calling 
> SSLServerSocketFactory.createServerSocket(0) which opens a socket on a free 
> port. Then this port number is passed to the client.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8134267
> Webrev: http://cr.openjdk.java.net/~asmotrak/8134267/webrev.00/

The changes look good to me.

-Chris.

Re: RFR(XXS): 8155036: Remove sun.security.action.GetBooleanSecurityPropertyAction

[Chris Hegarty]

On 25 Apr 2016, at 17:47, Claes Redestad  wrote:

> Hi,
> 
> since JEP-260 encapsulates internal APIs, we could remove unused classes like 
> this one
> 
> hg rm 
> ./src/java.base/share/classes/sun/security/action/GetBooleanSecurityPropertyAction.java

That should be ok.

-Chris.

Re: RFR: 8154231: Simplify access to System properties from JDK code

[Chris Hegarty]

On 20 Apr 2016, at 20:34, Claes Redestad  wrote:

> 
> 
> On 2016-04-20 20:51, Chris Hegarty wrote:
>> On 20 Apr 2016, at 15:44, Claes Redestad  wrote:
>> 
>>> Hello,
>>> 
>>> now that the sun.security.action package is encapsulated we can simplify 
>>> internal code to get System properties.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8154231
>>> Webrev: http://cr.openjdk.java.net/~redestad/8154231/webrev.01/
>> This looks very nice.
> 
> Thanks!
> 
>> 
>> Did you accidentally remove supportsTransparentAuth in the unix version
>> of NTLMAuthentication.java ? It is used reflectively by
>> sun.net.www.protocol.http.NTLMAuthenticationProxy. Otherwise, this looks
>> fine to me.
> 
> Oops, nice catch. Updated.

I’m happy with this version. Thanks.

-Chris.

Re: RFR: 8154231: Simplify access to System properties from JDK code

[Chris Hegarty]

On 20 Apr 2016, at 15:44, Claes Redestad  wrote:

> Hello,
> 
> now that the sun.security.action package is encapsulated we can simplify 
> internal code to get System properties.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8154231
> Webrev: http://cr.openjdk.java.net/~redestad/8154231/webrev.01/

This looks very nice.

Did you accidentally remove supportsTransparentAuth in the unix version 
of NTLMAuthentication.java ? It is used reflectively by 
sun.net.www.protocol.http.NTLMAuthenticationProxy. Otherwise, this looks
fine to me.

-Chris.

Note to self; I need to revisit the sun.reflect properties.



> This adds a few convenience methods to GetPropertyAction[1] and 
> GetIntegerAction[2]. Since the code calling into this can be streamlined, 
> this leads to a net static footprint reduction of the java.base module and a 
> tiny startup improvement.
> 
> Thanks!
> 
> /Claes
> 
> [1] 
> http://cr.openjdk.java.net/~redestad/8154231/webrev.01/src/java.base/share/classes/sun/security/action/GetPropertyAction.java.udiff.html
> [2] 
> http://cr.openjdk.java.net/~redestad/8154231/webrev.01/src/java.base/share/classes/sun/security/action/GetIntegerAction.java.udiff.html

Re: RFR 8153371: Remove sun.misc.ManagedLocalsThread from jdk.crypto.pkcs11

[Chris Hegarty]

The changes in the webrev, along with the change to the module info, look good 
to me. 

-Chris.

> On 16 Apr 2016, at 01:22, Xuelei Fan  wrote:
> 
> Resend.
> 
> Looks like there jdk.crypto.pkcs11 has no dependence on jdk.unsupported
> any more.  Do you want to update the module-info.java in this update?
> 
> module jdk.crypto.pkcs11 {
>// Depends on SunEC provider for EC related functionality
>requires jdk.crypto.ec;
> -   // 8153371
> -   requires jdk.unsupported;
>provides java.security.Provider with sun.security.pkcs11.SunPKCS11;
> }
> 
> Otherwise, looks fine to me.
> 
> Xuelei
> 
>> On 4/16/2016 8:13 AM, Valerie Peng wrote:
>> NOTE: resend with the correct bug id.
>> 
>> Anyone can help reviewing this? Just a one-line replacement for
>> switching to the new public Thread constructor.
>> No new regression test as it should be covered by the existing one.
>> 
>> Webrev: http://cr.openjdk.java.net/~valeriep/8153371/webrev.00/
>> 
>> Thanks,
>> Valerie
> 

Re: JDK 9 RFR of JDK-8151763; Use more informative format for problem list

[Chris Hegarty]

Looks good Joe.

-Chris

> On 11 Mar 2016, at 22:28, joe darcy  wrote:
> 
> Hello,
> 
> As Jon Gibbons has noted off-list, the problem list entries can directly 
> include the bug number associated with the test in question, enabling better 
> reporting. This format should be used rather than the current convention of 
> putting the bug number in a comment.
> 
> Please review the webrev to adopt the revised format for the problem list:
> 
>http://cr.openjdk.java.net/~darcy/8151763.0/
> 
> I've verified jtreg produces the same test list with the old and new versions 
> of the problem list.
> 
> Thanks,
> 
> -Joe

Re: RFR [9] 7067728: Remove stopThread default java.policy

[Chris Hegarty]

On 14 Jan 2016, at 17:29, Mandy Chung  wrote:

> 
>> On Jan 14, 2016, at 9:19 AM, Chris Hegarty  wrote:
>> 
>>> There are existing tests whose grants this "stopThread” RuntimePermission 
>>> that may not be needed for the test.  The test policy likely copies that 
>>> from the default system java.policy.  We should update these test policy as 
>>> well.
>> 
>> I do see a few of these, and some will need discussion. Ok if I file a 
>> separate
>> bug on these, they are not directly related to this change, and do still 
>> pass, just
>> that the permission is superfluous.
>> 
> 
> Taking it out from the test policy file should be non-controversial and 
> trivial to verify.  

Right. I was thinking that maybe these tests should be updated to use the new
jtreg machanism, java.security.policy, rather than just removing stopThread? 

> I can see why you prefer to separate the test update from this change and I’m 
> okay.

Thanks. I’ll file a bug on it.

> 
>>>>> I would have expected some tests to need modifying here (or other 
>>>>> places!).
>>>> 
>>>> I haven’t seen any test failures resulting from this change ( not sure
>>>> if that is a good or a bad thing! ).  Though, there were several 
>>>> implementation
>>>> bugs that needed to be resolved before being able to remove default grant.
>>> 
>>> jtreg policy tag overrides the system default security policy with the 
>>> specified file.  Tests that call Thread::stop and tested with security 
>>> manager must have  "stopThread” RuntimePermission set in the test policy.  
>>> jtreg was enhanced to add a new java.security.policy tag to extend the 
>>> system security policy [1].  
>> 
>> Thanks for this explanation. I always get confused with how jtreg supports
>> this.
>> 
>>> Only tests using java.security.policy tag and calling Thread::stop will 
>>> need to be modified.
>> 
>> I can find no such tests.
> 
> That matches what I expect since most of the tests using the new 
> java.security.policy tag are related to deprivileging work and new tests only.

Great.

-Chris.

Re: RFR [9] 7067728: Remove stopThread default java.policy

[Chris Hegarty]

On 14 Jan 2016, at 16:52, Mandy Chung  wrote:

>> On Jan 14, 2016, at 2:05 AM, Chris Hegarty  wrote:
>> 
>> The "stopThread” RuntimePermission is granted by default. The Thread.stop
>> methods have been deprecated for more than 15 years. It seems reasonable,
>> in a major release, to remove the default grant of stopThread.
> 
> +1 to remove "stopThread” RuntimePermission from java.policy.

Thanks for the review Mandy.

> There are existing tests whose grants this "stopThread” RuntimePermission 
> that may not be needed for the test.  The test policy likely copies that from 
> the default system java.policy.  We should update these test policy as well.

I do see a few of these, and some will need discussion. Ok if I file a separate
bug on these, they are not directly related to this change, and do still pass, 
just
that the permission is superfluous.

>>> I would have expected some tests to need modifying here (or other places!).
>> 
>> I haven’t seen any test failures resulting from this change ( not sure
>> if that is a good or a bad thing! ).  Though, there were several 
>> implementation
>> bugs that needed to be resolved before being able to remove default grant.
> 
> jtreg policy tag overrides the system default security policy with the 
> specified file.  Tests that call Thread::stop and tested with security 
> manager must have  "stopThread” RuntimePermission set in the test policy.  
> jtreg was enhanced to add a new java.security.policy tag to extend the system 
> security policy [1].  

Thanks for this explanation. I always get confused with how jtreg supports
this.

> Only tests using java.security.policy tag and calling Thread::stop will need 
> to be modified.

I can find no such tests.

-Chris.

> Mandy
> [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7900898