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.
