[jcifs] Issues with closing destination file in SmbFile.copyTo0()

[Vella, Shon]

Follow up on what was happening at my customers sights and how both of
the issues I noticed contributed. Randomly issue #1 would close the
destination file before the last block had been written by the writer
thread. When the writer thread did the write, it would fail with an
Invalid Handle and exit the writer thread leaving a record of the
exception. Subsequent writes to the writer thread (for all the rest of
the files in a deep copy, which in this case was a lot) would fail
because of the remembered exception, and because that remembered
exception would be thrown each time, the close of the file would be
skipped and left open. My patch along with setting
ignoreCopyToException to false completely fixes the problem I was
happen, but there is still one issue remaining which is that if
ignoreCopyToException is left at the default, an exception in the
writer thread will not abort a deep copy, but rather continue to open
destination files, fail to write to them and then close them leaving
them empty.

Shon Vella
Product Engineer
iDENTiTYAUTOMATiON
www.identityautomation.com


On Thu, Jun 19, 2014 at 7:56 AM, Vella, Shon <svella at idauto.net> wrote:
> I have a customer that's been having issues with files being left open
> when copying directory hierarchies (i.e. moving home directories). I
> don't know for sure that this is the issue the customer is seeing, but
> looking over the code for copyTo0() it is apparent that there are two
> issues related to closing the destination file.
>
> 1. There are no guarantees that the last block has been written by
> writer thread before Trans2SetFileInformation is sent or dest.close()
> is called.
> 2. If there are any exceptions thrown during the read/write loop or
> when sending Trans2SetFileInformation, dest.close() will not be called
> at all, leaving it open until the session is dropped.
>
> I've created a patch that addresses both problems. Note that the patch
> builds depends on a previous patch
> (http://article.gmane.org/gmane.network.samba.java/9348) that I've
> submitted.
>
> --- src/main/java/jcifs/smb/SmbFile.java (revision 5164)
> +++ src/main/java/jcifs/smb/SmbFile.java (revision )
> @@ -36,8 +36,6 @@
>  import jcifs.dcerpc.*;
>  import jcifs.dcerpc.msrpc.*;
>
> -import java.util.Date;
> -
>  /**
>   * This class represents a resource on an SMB network. Mainly these
>   * resources are files and directories however an <code>SmbFile</code>
> @@ -2105,13 +2103,7 @@
>              if( e != null ) {
>                  throw e;
>              }
> -            while( !ready ) {
> -                try {
> -                    wait();
> -                } catch( InterruptedException ie ) {
> -                    throw new SmbException( dest.url.toString(), ie );
> -                }
> -            }
> +            waitUntilReady();
>              if( e != null ) {
>                  throw e;
>              }
> @@ -2123,6 +2115,16 @@
>              notify();
>          }
>
> +        synchronized void waitUntilReady() throws SmbException {
> +            while( !ready ) {
> +                try {
> +                    wait();
> +                } catch( InterruptedException ie ) {
> +                    throw new SmbException( dest.url.toString(), ie );
> +                }
> +            }
> +        }
> +
>          public void run() {
>              synchronized( this ) {
>                  try {
> @@ -2231,27 +2233,33 @@
>                      }
>                  }
>
> +                try {
> -                i = 0;
> -                off = 0L;
> +                    i = 0;
> +                    off = 0L;
> -                for( ;; ) {
> +                    for (; ; ) {
> -                    req.setParam( fid, off, bsize );
> +                        req.setParam(fid, off, bsize);
> -                    resp.setParam( b[i], 0 );
> +                        resp.setParam(b[i], 0);
> -                    send( req, resp );
> +                        send(req, resp);
>
> -                    if( resp.dataLength <= 0 ) {
> +                        if (resp.dataLength <= 0) {
> -                        break;
> -                    }
> +                            break;
> +                        }
>
> -                    w.write( b[i], resp.dataLength, dest, off );
> +                        w.write(b[i], resp.dataLength, dest, off);
>
> -                    i = i == 1 ? 0 : 1;
> -                    off += resp.dataLength;
> -                }
> +                        i = i == 1 ? 0 : 1;
> +                        off += resp.dataLength;
> +                    }
> -
> +                } finally {
> +                    try {
> +                        w.waitUntilReady();
> -                dest.send( new Trans2SetFileInformation(
> +                        dest.send(new Trans2SetFileInformation(
> -                        dest.fid, attributes, createTime, lastModified ),
> +                                        dest.fid, attributes,
> createTime, lastModified),
> -                        new Trans2SetFileInformationResponse() );
> +                                new Trans2SetFileInformationResponse());
> +                    } finally {
> -                dest.close( 0L );
> +                        dest.close(0L);
> +                    }
> +                }
>              } catch( SmbException se ) {
>
>                  if (ignoreCopyToException == false)
>
>
> Shon Vella
> Product Engineer
> iDENTiTYAUTOMATiON
> www.identityautomation.com