The patch is attached.
Thanks,
Tigran.
----- Original Message -----
> From: "Weijun Wang" <weijun.w...@oracle.com>
> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
> Cc: "security-dev" <security-dev@openjdk.java.net>
> Sent: Friday, April 24, 2020 4:21:27 PM
> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
> You can send me a patch and I can sponsor it.
>
> Thanks,
> Max
>
>> On Apr 24, 2020, at 9:54 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>> wrote:
>>
>> Hi Max,
>>
>> Do you want me to go though official contribution (I already have signed OCA
>> for
>> glassfish) or
>> someone from oracle team will take care about it?
>>
>> Thanks,
>> Tigran.
>>
>> ----- Original Message -----
>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>> Sent: Friday, April 24, 2020 3:29:02 PM
>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>
>>> Hi Tigran,
>>>
>>> I read "Collection.html#optional-restrictions" carefully, and yes you're
>>> correct. Sorry for my ignorance.
>>>
>>> This method is called quite a lot. It might be worth an enhancement.
>>>
>>> Thanks,
>>> Max
>>>
>>>> On Apr 24, 2020, at 9:00 PM, Mkrtchyan, Tigran <tigran.mkrtch...@desy.de>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> Here is the benchmark results:
>>>>
>>>> Benchmark Mode Cnt Score Error Units
>>>> SubjectBenchmark.jdkSubject thrpt 20 473086.025 ± 7661.215 ops/s
>>>> SubjectBenchmark.patchedSubject thrpt 20 2529016.530 ± 50982.465 ops/s
>>>>
>>>> On a one hand, patched version has 5x higher throughput. However, on an
>>>> other
>>>> hand
>>>> the throughput of the original code is sufficient for any real
>>>> application. The
>>>> benchmark code is attached.
>>>>
>>>> Sorry for the noise. Should have done benchmarks before trying to optimize
>>>> it.
>>>>
>>>> Tigran.
>>>>
>>>> ----- Original Message -----
>>>>> From: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>>> To: "Weijun Wang" <weijun.w...@oracle.com>
>>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>>> Sent: Friday, April 24, 2020 12:50:03 PM
>>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>>
>>>>> I read it differently:
>>>>>
>>>>> Attempting to query the presence of an ineligible element may throw an
>>>>> exception, or it may simply return false; some implementations
>>>>> will exhibit the former behavior and some will exhibit the latter.
>>>>>
>>>>> Implementation might thrown an exception or return false, but sill don't
>>>>> allow
>>>>> null values.
>>>>>
>>>>> ... would not result in the insertion of an ineligible element into the
>>>>> collection may throw an exception or it may
>>>>> succeed, at the option of the implementation.
>>>>>
>>>>>
>>>>> For example, TreeSet throws NPE depending on Comparator.
>>>>>
>>>>> Well, I understand, this is a corner case that possibly not worth to
>>>>> optimize.
>>>>> Let me write
>>>>> a benchmark to see how much difference it makes.
>>>>>
>>>>> Tigran.
>>>>>
>>>>>
>>>>> ----- Original Message -----
>>>>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>>>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>>>> Sent: Friday, April 24, 2020 12:23:48 PM
>>>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>>>
>>>>>> My understanding is by optional you have 2 options while designing the
>>>>>> Set:
>>>>>>
>>>>>> 1. Allow null
>>>>>>
>>>>>> 2. Not allow null
>>>>>>
>>>>>> Now that SecureSet has chosen the 2nd way, it is now in the "this set
>>>>>> does not
>>>>>> permit null elements" category and therefore it should "@throws
>>>>>> NullPointerException if the specified element is null".
>>>>>>
>>>>>> --Max
>>>>>>
>>>>>>> On Apr 24, 2020, at 6:13 PM, Mkrtchyan, Tigran
>>>>>>> <tigran.mkrtch...@desy.de> wrote:
>>>>>>>
>>>>>>> Hi Max,
>>>>>>>
>>>>>>> that's right, but java doc as well mentions that this exception is
>>>>>>> *optional*,
>>>>>>> and has a corresponding note:
>>>>>>>
>>>>>>> Some collection implementations have restrictions on the elements that
>>>>>>> they may
>>>>>>> contain. For example, some implementations prohibit null elements, and
>>>>>>> some
>>>>>>> have restrictions on the types of their elements. Attempting to add an
>>>>>>> ineligible element throws an unchecked exception, typically
>>>>>>> NullPointerException or ClassCastException. Attempting to query the
>>>>>>> presence of
>>>>>>> an ineligible element may throw an exception, or it may simply return
>>>>>>> false;
>>>>>>> some implementations will exhibit the former behavior and some will
>>>>>>> exhibit the
>>>>>>> latter. More generally, attempting an operation on an ineligible
>>>>>>> element whose
>>>>>>> completion would not result in the insertion of an ineligible element
>>>>>>> into the
>>>>>>> collection may throw an exception or it may succeed, at the option of
>>>>>>> the
>>>>>>> implementation. Such exceptions are marked as "optional" in the
>>>>>>> specification
>>>>>>> for this interface.
>>>>>>>
>>>>>>> Thus, returning *false* is justified and spec compliant.
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tigran.
>>>>>>>
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>>> From: "Weijun Wang" <weijun.w...@oracle.com>
>>>>>>>> To: "Tigran Mkrtchyan" <tigran.mkrtch...@desy.de>
>>>>>>>> Cc: "security-dev" <security-dev@openjdk.java.net>
>>>>>>>> Sent: Friday, April 24, 2020 11:42:41 AM
>>>>>>>> Subject: Re: NPE is used in javax.security.auth.Subject for flowcontrol
>>>>>>>
>>>>>>>> Hi Tigran,
>>>>>>>>
>>>>>>>> In java.util.Set, we have:
>>>>>>>>
>>>>>>>> * @throws NullPointerException if the specified element is null and
>>>>>>>> this
>>>>>>>> * set does not permit null elements
>>>>>>>> * (<a href="Collection.html#optional-restrictions">optional</a>)
>>>>>>>> */
>>>>>>>> boolean contains(Object o);
>>>>>>>>
>>>>>>>> As an implementation, SecureSet must follow the spec to throw an NPE.
>>>>>>>> If it
>>>>>>>> returns null, some unexpected thing might happen when the contains()
>>>>>>>> method is
>>>>>>>> called somewhere else.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Max
>>>>>>>>
>>>>>>>>> On Apr 24, 2020, at 4:21 PM, Mkrtchyan, Tigran
>>>>>>>>> <tigran.mkrtch...@desy.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Dear Java-SE security developers,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Imagine a following code:
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>> Subject s1 = ... ;
>>>>>>>>>
>>>>>>>>> Subject s2 = ... ;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> s2.getPrincipals().addAll(s1.getPrincipals());
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>> The Subject's SecureSet.addAll checks that provided Set doesn't
>>>>>>>>> contains 'null'
>>>>>>>>> values
>>>>>>>>> by calling collectionNullClean, which calls SecureSet#contains:
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>> try {
>>>>>>>>> hasNullElements = coll.contains(null);
>>>>>>>>> } catch (NullPointerException npe) {
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>> The SecureSet#contains itself checks for 'null' values, the NPE is
>>>>>>>>> always
>>>>>>>>> generated.
>>>>>>>>>
>>>>>>>>> This as introduced by commit e680ab7f208e
>>>>>>>>>
>>>>>>>>> https://hg.openjdk.java.net/jdk/jdk/diff/e680ab7f208e/jdk/src/share/classes/javax/security/auth/Subject.java
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> As SecureSet doesn't allow null values, it will be much simpler to
>>>>>>>>> return false
>>>>>>>>> right away:
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>> public boolean contains(Object o) {
>>>>>>>>> if (o == null) {
>>>>>>>>> // null values rejected by add
>>>>>>>>> return false;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> ...
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks in advance,
>>>>>>>>> Tigran.
> >>> <SubjectBenchmark.java>
# HG changeset patch
# User Tigran Mkrtchyan <tigran.mkrtch...@desy.de>
# Date 1587750415 -7200
# Fri Apr 24 19:46:55 2020 +0200
# Branch optimize-subject
# Node ID 937a07d825c7f3e50d3c87d667c347835ce86f0b
# Parent d18064736c65b4945f8ce43623b9a07bca0bd088
avoid throwing NPE in javax.security.auth.Subject.SecureSet#contains
diff --git a/src/java.base/share/classes/javax/security/auth/Subject.java b/src/java.base/share/classes/javax/security/auth/Subject.java
--- a/src/java.base/share/classes/javax/security/auth/Subject.java
+++ b/src/java.base/share/classes/javax/security/auth/Subject.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1998, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -1202,8 +1202,13 @@ public final class Subject implements ja
public boolean contains(Object o) {
- Objects.requireNonNull(o,
- ResourcesMgr.getString("invalid.null.input.s."));
+ if (o == null) {
+
+ // As SecureSet#add will reject all null values there is no need for a deep check.
+ // This short-cut makes collectionNullClean more effective when an instance of
+ // SecureSet is passed as the argument.
+ return false;
+ }
final Iterator<E> e = iterator();
while (e.hasNext()) {
