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

Weijun Wang weijun.wang at oracle.com
Fri Apr 24 14:21:27 UTC 2020 

You can send me a patch and I can sponsor it.

Thanks,
Max

> On Apr 24, 2020, at 9:54 PM, Mkrtchyan, Tigran <tigran.mkrtchyan at 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.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 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.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>

More information about the security-dev mailing list