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>
