NPE is used in javax.security.auth.Subject for flowcontrol

[Weijun Wang]

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.mkrtchyan at 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.mkrtchyan at desy.de>
>> To: "Weijun Wang" <weijun.wang at oracle.com>
>> Cc: "security-dev" <security-dev at 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.wang at oracle.com>
>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan at desy.de>
>>> Cc: "security-dev" <security-dev at 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.mkrtchyan at 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.wang at oracle.com>
>>>>> To: "Tigran Mkrtchyan" <tigran.mkrtchyan at desy.de>
>>>>> Cc: "security-dev" <security-dev at 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.mkrtchyan at 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>