Re: Possible deadlocking between jcifs.smb.Dfs and jcifs.smb.SmbTransport [PATCH]

Xavier Roche <[email protected]> Thu, 01 Mar 2012 11:24:31 +0100

Newsgroups	gmane.network.samba.java
Organization	Exalead
Message-ID	<[email protected]>

Hi again,

On 02/29/2012 05:42 PM, Xavier Roche wrote:
> [ Issue found in jcifs 1.3.17 ]
> jcifs.smb.Dfs and jcifs.smb.SmbTransport seems to be deadlocking due to
> Mutex ordering mismatch in the code.

The attached patch seems to solve the issue.

Basically, the Dfs.resolve() and Dfs.insert() methods do not need to be 
synchronized, only the "referrals" member needs to be locked (using an 
additional lock, because the member itself can be replaced). This way, 
calls such as getDc() in Dfs.resolve() won't deadlock.

I successfully ran a stress test with 32 threads enumerating a DFS, and 
it seems to work fine.

Could this fix me merged in the next release, if it is good enough ?

Regards,
Xavier

jcifs-1.3.17-Dfs_deadlock_fix.patch (text/x-patch, 2.6 KB)

diff -rudb /ng/sdk/jcifs/1.3.17/src/jcifs/smb/Dfs.java jcifs/smb/Dfs.java
--- /ng/sdk/jcifs/1.3.17/src/jcifs/smb/Dfs.java	2011-10-18 21:26:25.000000000 +0200
+++ jcifs/smb/Dfs.java	2012-03-01 10:23:49.353867365 +0100
@@ -47,7 +47,9 @@
     protected static CacheEntry FALSE_ENTRY = new Dfs.CacheEntry(0L);
 
     protected CacheEntry _domains = null; /* aka trusted domains cache */
+    
     protected CacheEntry referrals = null;
+    protected final Object referralsLock = new Object();
 
     public HashMap getTrustedDomains(NtlmPasswordAuthentication auth) throws SmbAuthException {
         if (DISABLED || auth.domain == "?")
@@ -152,7 +154,7 @@
         }
         return null;
     }
-    public synchronized DfsReferral resolve(String domain,
+    public DfsReferral resolve(String domain,
                 String root,
                 String path,
                 NtlmPasswordAuthentication auth) throws SmbAuthException {
@@ -252,12 +254,14 @@
             /* We did not match a domain based root. Now try to match the
              * longest path in the list of stand-alone referrals.
              */
+            synchronized (referralsLock) {
             if (referrals != null && now > referrals.expiration) {
                 referrals = null;
             }
             if (referrals == null) {
                 referrals = new CacheEntry(0);
             }
+
             String key = "\\" + domain + "\\" + root;
             if (path.equals("\\") == false)
                 key += path;
@@ -265,7 +269,7 @@
 
             Iterator iter = referrals.map.keySet().iterator();
             while (iter.hasNext()) {
-                String _key = (String)iter.next();
+                    String _key = (String) iter.next();
                 int _klen = _key.length();
                 boolean match = false;
 
@@ -276,13 +280,15 @@
                 }
 
                 if (match)
-                    dr = (DfsReferral)referrals.map.get(_key);
+                        dr = (DfsReferral) referrals.map.get(_key);
+                }
             }
         }
 
         return dr;
     }
-    synchronized void insert(String path, DfsReferral dr) {
+    
+    void insert(String path, DfsReferral dr) {
         int s1, s2;
         String server, share, key;
 
@@ -316,6 +322,7 @@
          */
         dr.pathConsumed -= 1 + server.length() + 1 + share.length();
 
+        synchronized (referralsLock) {
         if (referrals != null && (System.currentTimeMillis() + 10000) > referrals.expiration) {
             referrals = null;
         }
@@ -324,4 +331,5 @@
         }
         referrals.map.put(key, dr);
     }
+    }
 }