RFR: 8085903: New fix for memory leak in ProtectionDomain cache

Xuelei Fan xuelei.fan at oracle.com
Wed Jan 13 00:36:31 UTC 2016


Looks fine to me.

Thanks,
Xuelei

On 1/13/2016 3:24 AM, Sean Mullan wrote:
> New (simpler) fix using Map.put():
> 
> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.02/
> 
> This should be ok, as the JDK 8 code and the JDK 9 code prior to the fix
> for 8055753 [1] was using Map.put instead of putIfAbsent.
> 
> Thanks,
> Sean
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8055753
> 
> On 01/12/2016 11:02 AM, Sean Mullan wrote:
>> On 01/12/2016 10:26 AM, Xuelei Fan wrote:
>>> I think hashMap.put() may be similar or more effective than
>>> hashMap.putIfAbsent().
>>
>> Ok, I think I see what you are trying to say.
>>
>> The common case is that the putIfAbsent method is going to succeed (i.e.
>> the entry will be absent). It may not succeed in cases where there are
>> multiple threads simultaneously computing the initial permissions for
>> the same PD or the value is garbage collected due to memory demand.
>>
>> So, I think the question boils down to whether put is faster than
>> putIfAbsent in the common case (when there is not an entry). Probably
>> yes, since it doesn't have to check if there is an entry already.
>>
>>> Is there a case that the key/weakPd is the same,
>>> but the value/pc is different?
>>
>> I believe the pc should always be the same. If you change the policy
>> file(s), then you have to explicitly refresh it (using Policy.refresh())
>> which should cause a new cache to be created.
>>
>> Let me do some more testing around this, and see if I can check
>> performance and see what is better.
>>
>> Thanks,
>> Sean
>>
>>>
>>> Xuelei
>>>
>>> On 1/12/2016 11:01 PM, Sean Mullan wrote:
>>>> Hi Ivan,
>>>>
>>>> On 01/12/2016 09:38 AM, Ivan Gerasimov wrote:
>>>>> Hi Sean!
>>>>>
>>>>> On 12.01.2016 16:26, Sean Mullan wrote:
>>>>>> I received a private comment that there may be cases where the map's
>>>>>> value is reclaimed by the garbage collector, but the key still
>>>>>> exists.
>>>>>> If that ProtectionDomain is subsequently used for permission
>>>>>> checks, a
>>>>>> Map.putIfAbsent method will not replace the value.
>>>>>>
>>>>>> So, I have added an additional check for this case in the PDCache.put
>>>>>> method, and it instead uses the Map.replace method. Updated webrev:
>>>>>>
>>>>>
>>>>> Wouldn't it be a little bit more efficient to use merge() here?
>>>>>
>>>>> Something like:
>>>>> pdMap.merge(weakPd, v, (pv, nv) -> pv.get() != null ? pv : nv);
>>>>
>>>> Yes - good catch!
>>>>
>>>> Will update and send out new webrev after a round of testing ...
>>>>
>>>> Thanks,
>>>> Sean
>>>>
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>>>
>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.01/
>>>>>>
>>>>>> --Sean
>>>>>>
>>>>>> On 01/08/2016 03:06 PM, Sean Mullan wrote:
>>>>>>> Please review this fix for a memory leak in the ProtectionDomain
>>>>>>> cache
>>>>>>> (which maps each ProtectionDomain to their granted
>>>>>>> PermissionCollection). The memory leak only occurs if custom
>>>>>>> permissions
>>>>>>> are used in a policy file.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mullan/webrevs/8085903/webrev.00/
>>>>>>>
>>>>>>> Custom permissions cause a chain of strong references that link
>>>>>>> back to
>>>>>>> the ProtectionDomain key used in the map, preventing it from being
>>>>>>> removed (and also preventing the custom permission's URLClassLoader
>>>>>>> from
>>>>>>> being garbage collected). This fix wraps the PermissionCollection
>>>>>>> in a
>>>>>>> SoftReference which allows the objects to be garbage collected in
>>>>>>> response to memory demand.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sean
>>>>>>
>>>>>
>>>



More information about the security-dev mailing list