Re: RFR 8243592: Subject$SecureSet::contains(null) is suboptimal
Hi Max, Thanks for following up on that. I like your approach - it doesn't introduce confusion or behaviour changes. I re-run the benchmark: Benchmark Mode CntScore Error Units SubjectBenchmark.jdkOriginalSubject thrpt 20 360202.816 ± 4071.762 ops/s SubjectBenchmark.maxPatchSubject thrpt 20 1609248.338 ± 48137.227 ops/s SubjectBenchmark.tigranPatchSubject thrpt 20 1973207.564 ± 89670.297 ops/s The performance difference can be ignored. Best regards, Tigran. P.S. Thanks for the pointer to the github repo! - Original Message - > From: "Weijun Wang" > To: "Sean Mullan" > Cc: "security-dev" > Sent: Wednesday, July 1, 2020 5:31:54 AM > Subject: Re: RFR 8243592: Subject$SecureSet::contains(null) is suboptimal > Please review an updated fix at > > https://cr.openjdk.java.net/~weijun/8243592/webrev.03 > > After some discussion, we think there is no need to call contains(null) at all > since it might not be reliable (see the new test). Now, collectionNullClean > inspects all items in the input collection and create a new LinkedList. > > Since there is no need to repopulate a list inside "new SecureSet", the new > implementation is not slower than the original proposal from Tigran. > > And there is no need for a CSR because there will be no behavior change. > > Thanks, > Max > > p.s. Welcome to the github playground at > https://github.com/openjdk/playground/pull/9, but formal code review should > stay in this mail thread. > >> On Apr 30, 2020, at 9:50 PM, Weijun Wang wrote: >> >> Webrev updated at >> >> http://cr.openjdk.java.net/~weijun/8243592/webrev.02/ >> >> I removed my old test and rewrite the existing SubjectNullTests.java test in >> TestNG with quite some new test cases. >> >> There are so many behavior changes (although we might say good programs are >> not >> affected) that I decided to add a CSR and a release note. Please also take a >> look. >> >>CSR : https://bugs.openjdk.java.net/browse/JDK-8244165 >> release note : https://bugs.openjdk.java.net/browse/JDK-8244169 >> >> Thanks, >> Max >> >> >>> On Apr 29, 2020, at 2:57 AM, Sean Mullan wrote: >>> >>> I don't think containsAll and retainsAll need to call collectionNullClean >>> either >>> because SecureSet.contains(null) returns false now. >>> >>> --Sean >>> >>> On 4/28/20 9:58 AM, Weijun Wang wrote: A new webrev is at http://cr.openjdk.java.net/~weijun/8243592/webrev.01/ I take this chance to do some formatting and add a new test. My first testng test! Thanks, Max > On Apr 28, 2020, at 8:16 PM, Sean Mullan wrote: > > On 4/27/20 10:39 PM, Weijun Wang wrote: >> Reading the Set spec, it looks like an NPE is still needed for add(), but >> remove() can be modified. > > Good point, I agree. > > --Sean > >> --Max >>> On Apr 27, 2020, at 10:27 PM, Sean Mullan >>> wrote: >>> >>> The fix looks fine to me. For consistency, you could make the same >>> change for >>> null elements in the other SecureSet methods: add, remove. >>> >>> --Sean >>> >>> On 4/25/20 3:39 AM, Weijun Wang wrote: Please take a review at http://cr.openjdk.java.net/~weijun/8243592/webrev.00/ This is helpful if we do any set arithmetic between 2 Subject objects. No new regression test, I intend to add a noreg-trivial label. *Tigran*: Please confirm you are OK with the "Contributed-by" line. Thanks, Max
Re: Thread leak by LdapLoginModule
Hi, found it: https://bugs.openjdk.java.net/browse/JDK-8237876 Thanks, Tigran. - Original Message - > From: "Daniel Fuchs" > To: "Sean Mullan" , "Tigran Mkrtchyan" > , "security-dev" > > Cc: "core-libs-dev" > Sent: Wednesday, June 10, 2020 4:29:36 PM > Subject: Re: Thread leak by LdapLoginModule > On 09/06/2020 23:21, Sean Mullan wrote: >>> The issue is not observed with java-14, thus I assume that the fix is >>> related to commit >>> >>> http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4 >>> >>> As java-11 is LTS, what is the procedure to get it fix back-ported? > > Hi, > > AFAICS the fix has already been backported to JDK 11.0.8. > > best regards, > > -- daniel
Thread leak by LdapLoginModule
Hi all,
with Java-11 we have notice a thread leak with ldap module.
We use LDAP to authenticate users with username+pasword by
directly calling LdapLoginModule. This was ok with java 7 and
java 8. With java 11 we see threads getting accumulated. here is a
test case that demonstrates it:
```
private static final String USERNAME_KEY = "javax.security.auth.login.name";
private static final String PASSWORD_KEY =
"javax.security.auth.login.password";
String ldapUrl = "ldap://;;
String peopleOU = "ou= ... o= ... c=...");
String user = ...;
String pass = ...;
@Test
public void threadLeakTest() throws AuthenticationException,
NoSuchPrincipalException, LoginException {
Map threadsBefore =
Thread.getAllStackTraces();
Map globalLoginOptions = Map.of(
"userProvider", ldapUrl + "/" + peopleOU,
"useSSL", "false",
"userFilter", "(uid={USERNAME})",
"useFirstPass", "true"
);
for (int i = 0; i < 10; i++) {
Map loginOptions = Map.of(
USERNAME_KEY, user,
PASSWORD_KEY, pass.toCharArray());
Subject subject = new Subject();
LdapLoginModule loginModule = new LdapLoginModule();
loginModule.initialize(subject, null, loginOptions,
globalLoginOptions);
loginModule.login();
loginModule.commit();
loginModule.logout();
}
Map threadsAfter =
Thread.getAllStackTraces();
assertEquals("Thread leak detected", threadsBefore.size() + 1,
threadsAfter.size());
}
```
The thread count difference is always equals to the number of iterations in the
loop, e.g. on each call a
thread is created and stays around. Eventually our server crashes with:
[19497.011s][warning][os,thread] Attempt to protect stack guard pages failed
(0x7fcc4c65c000-0x7fcc4c66).
OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x7fcc4c55b000,
16384, 0) failed; error='Not enough space' (errno=12)
The issue is not observed with java-14, thus I assume that the fix is related
to commit
http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4
As java-11 is LTS, what is the procedure to get it fix back-ported?
Regards,
Tigran.
Re:RFR 8243592: Subject$SecureSet::contains(null) is suboptimal
Looks good to me. Thanks, Tigran Original message From: Weijun Wang Date: Sat, Apr 25, 2020, 09:40 To: security-dev@openjdk.java.net Cc: "Mkrtchyan, Tigran" Subject: RFR 8243592: Subject$SecureSet::contains(null) is suboptimal Please take a review at http://cr.openjdk.java.net/~weijun/8243592/webrev.00/ This is helpful if we do any set arithmetic between 2 Subject objects. No new regression test, I intend to add a noreg-trivial label. *Tigran*: Please confirm you are OK with the "Contributed-by" line. Thanks, Max
Re: NPE is used in javax.security.auth.Subject for flowcontrol
The patch is attached. Thanks, Tigran. - Original Message - > From: "Weijun Wang" > To: "Tigran Mkrtchyan" > Cc: "security-dev" > 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 >> 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" >>> To: "Tigran Mkrtchyan" >>> Cc: "security-dev" >>> 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 >>>> wrote: >>>> >>>> >>>> >>>> Here is the benchmark results: >>>> >>>> Benchmark Mode CntScore 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" >>>>> To: "Weijun Wang" >>>>> Cc: "security-dev" >>>>> 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" >>>>>> To: "Tigran Mkrtchyan" >>>>>> Cc: "security-dev" >>>>>> 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 >>>>>> NullPointerExceptio
Re: NPE is used in javax.security.auth.Subject for flowcontrol
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" > To: "Tigran Mkrtchyan" > Cc: "security-dev" > 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 >> wrote: >> >> >> >> Here is the benchmark results: >> >> Benchmark Mode CntScore 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" >>> To: "Weijun Wang" >>> Cc: "security-dev" >>> 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" >>>> To: "Tigran Mkrtchyan" >>>> Cc: "security-dev" >>>> 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 >>>>> 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 el
Re: NPE is used in javax.security.auth.Subject for flowcontrol
Here is the benchmark results: Benchmark Mode CntScore 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" > To: "Weijun Wang" > Cc: "security-dev" > 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" >> To: "Tigran Mkrtchyan" >> Cc: "security-dev" >> 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 >>> 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" >>>> To: "Tigran Mkrtchyan" >>>> Cc: "security-dev" >>>> 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 >>>>* (optional) >>>>*/ >>>> 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, &g
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"
> To: "Tigran Mkrtchyan"
> Cc: "security-dev"
> 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
>> 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"
>>> To: "Tigran Mkrtchyan"
>>> Cc: "security-dev"
>>> 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
>>>* (optional)
>>>*/
>>> 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
>>>> 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.
Re: NPE is used in javax.security.auth.Subject for flowcontrol
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"
> To: "Tigran Mkrtchyan"
> Cc: "security-dev"
> 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
> * (optional)
> */
>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
>> 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.
NPE is used in javax.security.auth.Subject for flowcontrol
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.
