code review request: 7118809: rcache deadlock

[Weijun Wang]

How about this? We remove the empty ReplayCache in JDK 8 but keep it in 
7u4 (I want to backport it). I just feel there is a hidden monster 
somewhere.

Thanks
Max

On 01/13/2012 07:20 AM, Valerie (Yu-Ching) Peng wrote:
>
> Secret use?
> As far as I can see, the CacheTable.put(...) method handles the case
> where an ReplayCache object isn't found and I can't think of any secret
> use for this.
> It looks to me that cleaning this up is only an internal impl of
> CacheTable class and should not affect its behavior.
> Removing an empty Cache seems more correct than leaving it there.
> Thanks,
> Valerie
>
> On 01/11/12 22:11, Weijun Wang wrote:
>>> In addition, I think the CacheTable.put(...) method should check if the
>>> ReplayCache object is empty and if yes, do the removal itself.
>>> So, the cleanup of empty ReplayCache object is still done, except it's
>>> now in CacheTable class instead of ReplayCache class which leads to the
>>> deadlock.
>>
>> That was my original plan. But I have no idea why in CacheTable there is
>>
>> // re-insert the entry, since rc.put could have removed the entry
>>
>> Does it really have a secret use?
>>
>>>
>>> Lastly, not really relevant to your changes here, but the code in both
>>> classes doesn't seem too efficient and some even looks not quite
>>> correct.
>>> For example, the impl of ReplayCache.put(...) method first goes through
>>> the LinkList and insert authenticator timestamp in descending order and
>>> then going through it again removing the expired timestamps. Wouldn't it
>>> be better to revert the order or doing both together, i.e. check for
>>> expiration and find the indexing while going through the list once?
>>
>> Maybe.
>>
>> Thanks
>> Max
>>
>>>
>>> Thanks,
>>> Valerie
>>>
>>> On 01/09/12 21:41, Weijun Wang wrote:
>>>> Hi Valerie
>>>>
>>>> Please review my fix:
>>>>
>>>> http://cr.openjdk.java.net/~weijun/7118809/webrev.00/
>>>>
>>>> Basically, CacheTable.put() operates on ReplayCache inside it, and
>>>> ReplayCache.put() operates on CacheTable containing it, and both
>>>> methods are synchronized and using thread-safe Hashtable.
>>>>
>>>> My solution is to move the "table.remove(principal)" line in
>>>> ReplayCache.put() outside the method. I search thru JDK and
>>>> CacheTable.put() is the only place the method is called:
>>>>
>>>> public synchronized void put(String principal, AuthTime time, long
>>>> currTime) {
>>>> ReplayCache rc = super.get(principal);
>>>> if (rc == null) {
>>>> if (DEBUG) {
>>>> System.out.println("replay cache for " + principal + "
>>>> is null.");
>>>> }
>>>> rc = new ReplayCache(principal, this);
>>>> rc.put(time, currTime);
>>>> super.put(principal, rc);
>>>> }
>>>> else {
>>>> rc.put(time, currTime);
>>>> // re-insert the entry, since rc.put could have removed
>>>> the entry
>>>> super.put(principal, rc);
>>>> if (DEBUG) {
>>>> System.out.println("replay cache found.");
>>>> }
>>>> }
>>>> }
>>>>
>>>> Curiously, you can see after each call, the ReplayCache object is
>>>> added back into the CacheTable. Therefore, the remove action is
>>>> useless.
>>>>
>>>> Maybe the most correct way is to remove a ReplayCache from a
>>>> CacheTable if it's empty, but that re-insert line was added in a
>>>> security fix some years ago which I cannot decipher.
>>>>
>>>> So my fix simply removes the "remove" call in ReplayCache.
>>>>
>>>> No regression test, hard to reproduce failure.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>> -------- Original Message --------
>>>> *Change Request ID*: 7118809
>>>> *Synopsis*: rcache deadlock
>>>>
>>>> === *Description*
>>>> ============================================================
>>>> A DESCRIPTION OF THE PROBLEM :
>>>> The program as below:
>>>> --------------------------------------------------
>>>> import sun.security.krb5.internal.rcache.*;
>>>> import sun.security.krb5.internal.*;
>>>> import java.util.*;
>>>>
>>>> public class KTest2 {
>>>> public static void main(String [] a) throws Exception{
>>>> final CacheTable ct = new CacheTable();
>>>> final long time = System.currentTimeMillis();
>>>> ct.put("TheOnlyOne", new AuthTime( time -
>>>> Krb5.DEFAULT_ALLOWABLE_CLOCKSKEW * 1000L, 0), time);
>>>> final ReplayCache rc = (ReplayCache) ct.get("TheOnlyOne");
>>>> new Thread() {
>>>> public void run() {
>>>> rc.put(new AuthTime( time - Krb5.DEFAULT_ALLOWABLE_CLOCKSKEW *
>>>> 1000L, 0), time + 300*1000);
>>>> }
>>>> }.start();
>>>> ct.put("TheOnlyOne", new AuthTime( time -
>>>> Krb5.DEFAULT_ALLOWABLE_CLOCKSKEW * 1000L, 0), time);
>>>> }
>>>> }
>>>> --------------------------------------------------
>>>> When compiles as in: javac KTest2.java
>>>> and executed as in: java KTest2
>>>> can deadlock the hosting JVM as is reproduced by the stack-trace dump,
>>>> below:
>>>> -------------------------------------------------
>>>> Found one Java-level deadlock:
>>>> =============================
>>>> "Thread-0":
>>>> waiting to lock monitor 0x0921d720 (object 0xa5621b80, a
>>>> sun.security.krb5.internal.rcache.CacheTable),
>>>> which is held by "main"
>>>> "main":
>>>> waiting to lock monitor 0x0921caa0 (object 0xa5622fb8, a
>>>> sun.security.krb5.internal.rcache.ReplayCache),
>>>> which is held by "Thread-0"
>>>>
>>>> Java stack information for the threads listed above:
>>>> ===================================================
>>>> "Thread-0":
>>>> at java.util.Hashtable.remove(Hashtable.java:435)
>>>> - waiting to lock<0xa5621b80> (a
>>>> sun.security.krb5.internal.rcache.CacheTable)
>>>> at
>>>> sun.security.krb5.internal.rcache.ReplayCache.put(ReplayCache.java:123)
>>>> - locked<0xa5622fb8> (a
>>>> sun.security.krb5.internal.rcache.ReplayCache)
>>>> at KTest2$1.run(KTest2.java:13)
>>>> "main":
>>>> at
>>>> sun.security.krb5.internal.rcache.ReplayCache.put(ReplayCache.java:62)
>>>> - waiting to lock<0xa5622fb8> (a
>>>> sun.security.krb5.internal.rcache.ReplayCache)
>>>> at
>>>> sun.security.krb5.internal.rcache.CacheTable.put(CacheTable.java:59)
>>>> - locked<0xa5621b80> (a
>>>> sun.security.krb5.internal.rcache.CacheTable)
>>>> at KTest2.main(KTest2.java:16)
>>>>
>>>> Found 1 deadlock.
>>>> ...
>>>>
>>>
>
>