code review request: 7118809: rcache deadlock

[Weijun Wang]

On 01/12/2012 07:16 AM, Valerie (Yu-Ching) Peng wrote:
> Max,
>
> Well, now that you removed the call of "table.remove(principal)", the
> "table" field in ReplayCache is useless.
> Given that both classes are serializable, I understand that we may not
> able to do as much clean up as we want.

Correct.

>
> Now that you removed the "table.remove(principal)" call from the
> ReplayCache class, I think we should update the CacheTable.put(...)
> method as well since Its "super.put(principal, rc)" call is now
> redundant given that there is no chance for ReplayCache to remove the entry.

Correct.

>
> 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.
>> ...
>>
>