Re: RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources
On Sat, 21 Nov 2020 08:32:17 GMT, Christoph Langer wrote: > There is a flaw in sun.security.ssl.SSLSocketImpl::close() which leads to > leaking socket resources after JDK-8224829. > > The close method calls duplexCloseOutput() and duplexCloseInput(). In case of > an exception in any of these methods, the call to closeSocket() is bypassed, > and the underlying Socket may not be closed. > > This manifests in a real life leak after JDK-8224829 has introduced a call to > getSoLinger() on the path of duplexCloseOutput -> closeNotify. If socket impl > / OS socket hadn't been created yet it is done at that place. But then after > duplexCloseOutput eventually fails with a SocketException since the socket > wasn't connected, closing fails to call Socket::close(). > > This problem can be reproduced by this code: > SSLSocket sslSocket = > (SSLSocket)SSLSocketFactory.getDefault().createSocket(); > sslSocket.getSSLParameters(); > sslSocket.close(); > > This is what happens when SSLContext.getDefault().getDefaultSSLParameters() > is called, with close() being eventually called by the finalizer. > > I'll open this PR as draft for now to start discussion. I'll create a > testcase to reproduce the issue and add it soon. > > I propose to modify the close method such that duplexClose is only done on a > connected/bound socket. Maybe it even suffices to only do it when connected. > > Secondly, I'm proposing to improve exception handling a bit. So in case > there's an IOException on the path of duplexClose, it is caught and logged. > But the real close moves to the finally block since it should be done > unconditionally. I changed the check for when to do duplexClose to only do it when socket isConnected(). I also added a testcase which should work on all platforms. For windows I borrowed some functionality introduced lately with test java/lang/ProcessBuilder/checkHandles/CheckHandles.java which I moved to the test library for that reason. Now it's ready to review. - PR: https://git.openjdk.java.net/jdk/pull/1363
RFR: 8256818: SSLSocket that is never bound or connected leaks socket resources
There is a flaw in sun.security.ssl.SSLSocketImpl::close() which leads to leaking socket resources after JDK-8224829. The close method calls duplexCloseOutput() and duplexCloseInput(). In case of an exception in any of these methods, the call to closeSocket() is bypassed, and the underlying Socket may not be closed. This manifests in a real life leak after JDK-8224829 has introduced a call to getSoLinger() on the path of duplexCloseOutput -> closeNotify. If socket impl / OS socket hadn't been created yet it is done at that place. But then after duplexCloseOutput eventually fails with a SocketException since the socket wasn't connected, closing fails to call Socket::close(). This problem can be reproduced by this code: SSLSocket sslSocket = (SSLSocket)SSLSocketFactory.getDefault().createSocket(); sslSocket.getSSLParameters(); sslSocket.close(); This is what happens when SSLContext.getDefault().getDefaultSSLParameters() is called, with close() being eventually called by the finalizer. I'll open this PR as draft for now to start discussion. I'll create a testcase to reproduce the issue and add it soon. I propose to modify the close method such that duplexClose is only done on a connected/bound socket. Maybe it even suffices to only do it when connected. Secondly, I'm proposing to improve exception handling a bit. So in case there's an IOException on the path of duplexClose, it is caught and logged. But the real close moves to the finally block since it should be done unconditionally. - Commit messages: - Add testcase for socket leak - Only duplexClose when socket isConnected() - 8256818: SSLSocket that is never bound or connected leaks socket resources Changes: https://git.openjdk.java.net/jdk/pull/1363/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1363=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256818 Stats: 130 lines in 5 files changed: 98 ins; 15 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/1363.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1363/head:pull/1363 PR: https://git.openjdk.java.net/jdk/pull/1363
Re: RFR: JDK-8256475: Fix Behavior when Installer name differs from applicatio… [v4]
On Thu, 19 Nov 2020 23:32:22 GMT, Alexander Matveev wrote: >> Andy Herrick has updated the pull request incrementally with one additional >> commit since the last revision: >> >> JDK-8256475: Fix Behavior when Installer name differs from application >> name. > > Marked as reviewed by almatvee (Committer). @azuev-java - can you review as well ? - PR: https://git.openjdk.java.net/jdk/pull/1300
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
Ok, i've taking the time to read some literature about random generators because for me the Mersenne Twister was still the king. The current API proposed as clearly two levels, you have the user level and the implementation level, at least the implementation level should seen as a SPI RandomGenerator is the user facing API, for me it should be the sole interface exposed by the API, the others (leap, arbitrary leap and split) should be optional methods. In term of factory methods, we should have user driven methods: - getDefaultGenerator() that currently returns a L64X128MixRandom and can be changed in the future - getFastGenerator() that currently returns a Xoroshiro128PlusPlus and can be changed in the future - getDefault[Splitable|Leapable|etc]Generator that returns a default generator with the methods splits|leaps|etc defined - of / getByName that returns a specific generator by its name (but mov ed in a SPI class) The example in the documentation should use getDefaultGenerator() and not of() to avoid the problem all the programming languages currently have by having over-specified that the default generator is a Mersenne Twister. All methods that returns a stream of the available implementations should be moved in the SPI package. Rémi --- An honest question, why do we need so many interfaces for the different categories of RandomGenerator ? My fear is that we are encoding the state of our knowledge of the different kinds of random generators now so it will not be pretty in the future when new categories of random generator are discovered/invented. If we can take example of the past to predict the future, 20 years ago, what should have been the hierarchy at that time. Is it not reasonable to think that we will need new kinds of random generator in the future ? I wonder if it's not better to have one interface and several optional methods like we have with the collections, it means that we are loosing the possibilities to precisely type a method that only works with a precise type of generator but it will be more future proof. Rémi - Mail original - > De: "Jim Laskey" > À: "core-libs-dev" , "security-dev" > > Envoyé: Mercredi 18 Novembre 2020 14:52:56 > Objet: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators > This PR is to introduce a new random number API for the JDK. The primary API > is > found in RandomGenerator and RandomGeneratorFactory. Further description can > be > found in the JEP https://openjdk.java.net/jeps/356 . > > javadoc can be found at > http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html > > old PR: https://github.com/openjdk/jdk/pull/1273 > > - > > Commit messages: > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862; Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - 8248862: Implement Enhanced Pseudo-Random Number Generators > - ... and 15 more: > https://git.openjdk.java.net/jdk/compare/f7517386...2b3e4ed7 > > Changes: https://git.openjdk.java.net/jdk/pull/1292/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8248862 > Stats: 13319 lines in 25 files changed: 0 ins; 2132 del; 77 mod > Patch: https://git.openjdk.java.net/jdk/pull/1292.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292 > > PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Wed, 4 Nov 2020 09:21:12 GMT, Tagir F. Valeev wrote:
>> src/java.base/share/classes/java/util/stream/Stream.java line 1192:
>>
>>> 1190: @SuppressWarnings("unchecked")
>>> 1191: default List toList() {
>>> 1192: return (List) Collections.unmodifiableList(new
>>> ArrayList<>(Arrays.asList(this.toArray(;
>>
>> Why can't we return `listFromTrustedArrayNullsAllowed` here as in
>> `ReferencePipeline`?
>> Or at least, we should avoid unnecessary copying of arrays. See [how this is
>> done](https://github.com/amaembo/streamex/blob/master/src/main/java/one/util/streamex/AbstractStreamEx.java#L1313)
>> in StreamEx.
>
> `listFromTrustedArrayNullsAllowed` is clearly not an option, as it will
> produce shared-secret leaking (see
> [JDK-8254090](https://bugs.openjdk.java.net/browse/JDK-8254090) for a similar
> case). StreamEx solution is dirty as it relies on the implementation detail.
> I believe, OpenJDK team is not very interested in providing optimal
> implementations for third-party stream implementations, as third-party
> implementations will likely update by themselves when necessary. At least, my
> suggestion to make the default `mapMulti` implementation better was declined.
This implementation duplicates the array twice, once in this.toArray() and once
in the constructor of ArrayList that calls toArray() on Collections.ArrayList.
I'm sure there is a better way
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Wed, 18 Nov 2020 22:20:26 GMT, Stuart Marks wrote: >> This change introduces a new terminal operation on Stream. This looks like a >> convenience method for Stream.collect(Collectors.toList()) or >> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this >> method directly on Stream enables it to do what can't easily by done by a >> Collector. In particular, it allows the stream to deposit results directly >> into a destination array (even in parallel) and have this array be wrapped >> in an unmodifiable List without copying. >> >> In the past we've kept most things from the Collections Framework as >> implementations of Collector, not directly on Stream, whereas only >> fundamental things (like toArray) appear directly on Stream. This is true of >> most Collections, but it does seem that List is special. It can be a thin >> wrapper around an array; it can handle generics better than arrays; and >> unlike an array, it can be made unmodifiable (shallowly immutable); and it >> can be value-based. See John Rose's comments in the bug report: >> >> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 >> >> This operation is null-tolerant, which matches the rest of Streams. This >> isn't specified, though; a general statement about null handling in Streams >> is probably warranted at some point. >> >> Finally, this method is indeed quite convenient (if the caller can deal with >> what this operation returns), as collecting into a List is the most common >> stream terminal operation. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust List.copyOf to null-check and copy allowNulls lists. > Fix equals, hashCode, indexOf, lastIndexOf to handle nulls properly. > Add MOAT tests for new lists; add equals and hashCode tests. src/java.base/share/classes/java/util/ImmutableCollections.java line 222: > 220: default: > 221: return (List) new ListN<>(input, false); > 222: } Using a switch expression + arrow (->) here will allow you too factor the cast, even if it disappear in the generated bytecode, I think it makes the code more readable - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Thu, 5 Nov 2020 17:26:48 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 199:
>>
>>> 197: * safely reused as the List's internal storage, avoiding a
>>> defensive copy. Declared
>>> 198: * with Object... instead of E... as the parameter type so that
>>> varargs calls don't
>>> 199: * accidentally create an array of type other than Object[].
>>
>> Why would that be a problem? If the resulting list is immutable, then the
>> actual array type doesn't really matter, right?
>
> It's an implementation invariant that the internal array be Object[]. Having
> it be something other than Object[] can lead to subtle bugs. See
> [JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for example.
you can still calls the varargs with an already created array
listFromTrustedArray(new String[] { "foo" });
I think at least an assert is missing
assert input.getClass() == Object.class;
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Wed, 18 Nov 2020 14:16:03 GMT, Pavel Rappo wrote:
>> @dfuch May I ask how can I create a CSR? I checked
>> https://wiki.openjdk.java.net/display/csr/CSR+FAQs and it says:
>>
>>> Q: How do I create a CSR ?
>>> A: Do not directly create a CSR from the Create Menu. JIRA will let you do
>>> this right up until the moment you try to save it and find your typing was
>>> in vain.
>>> Instead you should go to the target bug, select "More", and from the drop
>>> down menu select "Create CSR". This is required to properly associate the
>>> CSR with the main bug, just as is done for backports.
>>
>> However, I don't have an account at https://bugs.openjdk.java.net/ yet.
>> Therefore, I don't see the "More" button on
>> https://bugs.openjdk.java.net/browse/JDK-8247402. Could you please tell me
>> how do I proceed? Thank you.
>
> @johnlinp, you cannot create a CSR by yourself at the moment. Someone else
> will have to do that for you. Might as well be me. So here's my proposal:
> come up with the meat, then I'll help you with the paperwork.
>
> For starters, have a look at existing CSRs (you don't need a JBS account for
> that). For example,
> https://bugs.openjdk.java.net/issues/?jql=issuetype%3DCSR%20and%20Subcomponent%3Djava.util%3Acollections%20
>
> Fill in an informal CSR template inline in this thread, and we'll proceed
> from that point. Is that okay?
@pavelrappo Please see my proposed CSR below. Thank you.
# Map::compute should have a clearer implementation requirement.
## Summary
java.util.Map::compute should have a clearer implementation requirement in its
documentation.
## Problem
The documentation of the implementation requirements for Map::compute has the
following problems:
1. It lacks of return statements for most of the if-else cases.
1. The indents are 3 spaces, while the convention is 4 spaces.
1. The if-else is overly complicated and can be simplified.
## Solution
Rewrite the documentation of Map::compute to match its default implementation.
## Specification
diff --git a/src/java.base/share/classes/java/util/Map.java
b/src/java.base/share/classes/java/util/Map.java
index b1de34b42a5..c3118a90581 100644
--- a/src/java.base/share/classes/java/util/Map.java
+++ b/src/java.base/share/classes/java/util/Map.java
@@ -1113,17 +1113,12 @@ public interface Map {
* {@code
* V oldValue = map.get(key);
* V newValue = remappingFunction.apply(key, oldValue);
- * if (oldValue != null) {
- *if (newValue != null)
- * map.put(key, newValue);
- *else
- * map.remove(key);
- * } else {
- *if (newValue != null)
- * map.put(key, newValue);
- *else
- * return null;
+ * if (newValue != null) {
+ * map.put(key, newValue);
+ * } else if (oldValue != null) {
+ * map.remove(key);
* }
+ * return newValue;
* }
*
* The default implementation makes no guarantees about detecting if the
-
PR: https://git.openjdk.java.net/jdk/pull/714
