[jcifs] WriterThreads are leaking

[Shon Vella]

I've run into an issue in JCIFS 3.1.17 where WriterThreads are hanging around blocked on the call to wait(). It looks like this is happening in the case where the parent directory of the destination file/folder does not exist, but I'm guessing that it could happen under other error conditions as well. This appears to have been reported a long time ago:

https://lists.samba.org/archive/jcifs/2006-November/006804.html

This was reported as fixed, though the original reporter reported that he could still duplicate it and suggested a need for the a call to interrupt().

Looking at the code I can see why it is happening, and while I think a call to interrupt() would fix the problem, that really isn't the correct fix.

The problem is that the currently there is no guarantee that WriterThread will have reached the wait() before the call to w.write() in the finally block if no previous write was performed in copyTo0(). This is because copyTo0() has code to ensure that WriterThread is ready before calling write(), whereas the finally block does not. While the code could be duplicated in the finally block, I think it would make more sense to move the synchronization code inside of the write() method. It would also be a very good think to mark member variables that are accessed from both threads as volatile, since without that there is no guarantee that other threads will see updated values values immediately. Something like:

--- ../1.3.17/src/jcifs/smb/SmbFile.java	2013-10-21 11:40:10.000000000 -0600
+++ src/jcifs/smb/SmbFile.java	2013-10-21 12:27:45.000000000 -0600
@@ -2077,12 +2077,12 @@
     }
 
     class WriterThread extends Thread {
-        byte[] b;
-        int n;
-        long off;
-        boolean ready;
-        SmbFile dest;
-        SmbException e = null;
+        volatile byte[] b;
+        volatile int n;
+        volatile long off;
+        volatile boolean ready;
+        volatile SmbFile dest;
+        volatile SmbException e = null;
         boolean useNTSmbs;
         SmbComWriteAndX reqx;
         SmbComWrite req;
@@ -2101,7 +2101,20 @@
             ready = false;
         }
 
-        synchronized void write( byte[] b, int n, SmbFile dest, long off ) {
+        synchronized void write( byte[] b, int n, SmbFile dest, long off ) throws SmbException {
+            if( e != null ) {
+                throw e;
+            }
+            while( !ready ) {
+                try {
+                    wait();
+                } catch( InterruptedException ie ) {
+                    throw new SmbException( dest.url.toString(), ie );
+                }
+            }
+            if( e != null ) {
+                throw e;
+            }
             this.b = b;
             this.n = n;
             this.dest = dest;
@@ -2225,25 +2238,11 @@
                     resp.setParam( b[i], 0 );
                     send( req, resp );
     
-                    synchronized( w ) {
-                        if( w.e != null ) {
-                            throw w.e;
-                        }
-                        while( !w.ready ) {
-                            try {
-                                w.wait();
-                            } catch( InterruptedException ie ) {
-                                throw new SmbException( dest.url.toString(), ie );
-                            }
-                        }
-                        if( w.e != null ) {
-                            throw w.e;
-                        }
                         if( resp.dataLength <= 0 ) {
                             break;
                         }
+
                         w.write( b[i], resp.dataLength, dest, off );
-                    }
     
                     i = i == 1 ? 0 : 1;
                     off += resp.dataLength;


Shon Vella
Developer
iDENTiTYAUTOMATiON