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

Mkrtchyan, Tigran tigran.mkrtchyan at desy.de
Fri Apr 24 17:58:52 UTC 2020


The patch is attached.


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 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.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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optimize-subject.patch
Type: text/x-patch
Size: 1566 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20200424/6b56be3c/optimize-subject-0001.patch>


More information about the security-dev mailing list