Bug 59859

Summary: Tomcat WebDAV MOVE operation fails on Windows
Product: Tomcat 8 Reporter: Coty Sutherland <csutherl>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 8.0.36   
Target Milestone: ----   
Hardware: PC   
OS: All   
Attachments: reproducer app
FIS.close() patch
Use try-with and don't throw IOException while closing InputStream

Description Coty Sutherland 2016-07-14 19:21:27 UTC
Created attachment 34040 [details]
reproducer app

The WebDAV MOVE operation doesn't seem to work in tomcat8 or tomcat8.5 (it works fine in tomcat7) on Windows. Here are some reproduction steps and a reproducer to show the behavior. Note that this only occurs on Windows; I also tested on Linux and it works fine there, so I think something is wrong at the OS/java level here.

Reproduction steps:

1) Install tomcat 8 on Windows
2) Add the webdav role and a user with that role to your conf/tomcat-users.xml
3) Deploy the attached webdav test application (webdavapp.war)
4) Start tomcat and make the following MOVE request:

curl -i -X MOVE --header "Destination: http://localhost:8080/webdavapp/webdav/hello2.txt" --user "tomcat:tomcat" http://localhost:8080/webdavapp/webdav/hello.txt

5) Observe the 500 response

Actual Results:

HTTP/1.1 500 Internal Server Error
Server: Apache-Coyote/1.1
Cache-Control: private
Expires: Wed, 31 Dec 1969 16:00:00 PST
Content-Type: text/html;charset=utf-8
Content-Language: en
Content-Length: 1040
Date: Thu, 14 Jul 2016 22:35:32 GMT
Connection: close

Expected Results (as seen on Linux):

HTTP/1.1 201 Created
Server: Apache-Coyote/1.1
Cache-Control: private
Expires: Wed, 31 Dec 1969 19:00:00 EST
Content-Length: 0
Date: Thu, 14 Jul 2016 18:42:30 GMT

Additional Information:

I did spend some time tracing this in jdb and found that the MOVE is partially working...it successfully copies the resource to the destination and but it is failing to delete the old resource. I think I tracked the failure down to the following block within o.a.c.servlets.WebdavServlet.doMove():

1745                if(!resource.isDirectory()) {
1746                    if(!resource.delete()) {
1747                        resp.sendError(WebdavStatus.SC_INTERNAL_SERVER_ERROR);
1748                        return false;

The problem occurs in the (o.a.c.webresources.CachedResource) resource.delete() call; it returns false and doesn't delete the resource for some reason (no exception is thrown, but the errorState flag is set to 1 on the response). Furthermore, if you make the same exact call again it will successfully delete the previously copied destination, copy the original to the destination again, and then fail to delete the original throwing the 500. I can't understand why the resource.delete() can fail because it passes through to what ends up being a o.a.c.webresources.FileResource.delete(). I compared the successful deletion's FileResource instance to the unsuccessful deletion and the instances match almost exactly (path's differ which is expected). I'm not sure what could be at play here unless its at the JVM level.
Comment 1 Michael Osipov 2016-07-15 07:22:35 UTC
Windows file locking?
Comment 2 Coty Sutherland 2016-07-15 12:11:54 UTC
> Windows file locking?

I'm no Windows expert, but I don't see how the issue could be file locking because it works in tomcat7 on the same machine with the same configuration/test application. I tested tomcat7 right before I tested tomcat8.

Additionally, I tested this with java 1.7.0u80 and 1.8.0u92 (both are the latest from jdk releases from Oracle) and the issue remains.
Comment 3 Christopher Schultz 2016-07-15 13:52:39 UTC
(In reply to Coty Sutherland from comment #2)
> > Windows file locking?
> 
> I'm no Windows expert, but I don't see how the issue could be file locking
> because it works in tomcat7 on the same machine with the same
> configuration/test application. I tested tomcat7 right before I tested
> tomcat8.
> 
> Additionally, I tested this with java 1.7.0u80 and 1.8.0u92 (both are the
> latest from jdk releases from Oracle) and the issue remains.

Just to be sure: you tested the same setup (including the exact same DAV-mounted directory structure) on both Tomcat 7 and Tomcat 8 and only Tomcat 8 fails? So, no file-permissions issues or anything like that?

The Resources implementation has been changed quite a bit between Tomcat 7 -> 8, so it's almost certainly a bug buried in there.
Comment 4 Coty Sutherland 2016-07-15 14:12:47 UTC
> Just to be sure: you tested the same setup (including the exact same
> DAV-mounted directory structure) on both Tomcat 7 and Tomcat 8 and only
> Tomcat 8 fails? So, no file-permissions issues or anything like that?

Correct. I downloaded both versions, unzipped them in the same folder, made the same configuration changes to add the role/user, deployed the same application, and made the exact same request from my hypervisor to both (I started and stopped each in between, they weren't running at the same time).
Comment 5 Coty Sutherland 2016-07-15 19:43:03 UTC
I updated the o.a.c.webresources.FileResource class and replaced the resource.delete() (because File.delete() throws no exceptions when it fails) with the following block:

110         try {
111             Path p = resource.toPath();
112             Files.delete(p);
113             return true;
114         } catch (Exception e) {
115             log.error("There was an exception! " + e.getMessage());
116             return false;
117         }

Trying my test with this in place yields an the following message in the catalina.log:

org.apache.catalina.webresources.FileResource.delete There was an exception! C:\apache-tomcat-8.0.36\webapps\webdavapp\hello.txt: The process cannot access the file because it is being used by another process.

The process owning the file handle is the java process that tomcat is running in. It looks like tomcat doesn't grab the file handle until the request is made, but then never releases the handle whereas tomcat7 does (or never gets it).
Comment 6 Coty Sutherland 2016-07-28 18:18:58 UTC
OK, after a lot of poking around I think I found a solution :D

I figured out that the problem was with a FileInputStream. Tomcat7 doesn't use FIS' in the webresources package, but tomcat8+ does. I determined this by noticing that that the file handle was hanging around (via lsof) after the MOVE call is made. Then I took a look at a heap dump taken after the issue (before a GC removed the reference) and found the FIS object in a Finalizer. After that, I started in the move method and stepped through the method with jdb while periodically running lsof to see when the handle was created. This led me to java/org/apache/catalina/servlets/WebdavServlet.java:1679 where a FIS was being retrieved from the resource, but not closed. My patch drops the FIS into a variable and then adds a close call after the write method is invoked to remove the handle. I don't think the write method should close the FIS since the FIS din't originate within the write method, which is why I put the close after the write returned. 

I verified that this doesn't leave a handle open on linux and that the move operation from my reproduction steps works on both linux and windows after the patch is applied.

Please review and provide feedback.
Comment 7 Coty Sutherland 2016-07-28 18:19:28 UTC
Created attachment 34077 [details]
FIS.close() patch
Comment 8 Felix Schumacher 2016-07-28 20:05:34 UTC
Created attachment 34078 [details]
Use try-with and don't throw IOException while closing InputStream

Close InputStream, but don't throw IOException when close() fails. Log the exception (that hopefully never gets thrown).
Comment 9 Felix Schumacher 2016-07-28 20:06:30 UTC
This issue is #89595 in coverity :)
Comment 10 Coty Sutherland 2016-07-28 20:15:34 UTC
Good idea! I don't know why I didn't do that given that the FileResource class uses try-with-resources for it's FIS usage. I like your patch change much better :)

Thanks!!
Comment 11 Coty Sutherland 2016-07-28 20:34:13 UTC
By the way, I tested the amended patch and it looks to resolve the issue as expected.
Comment 12 Felix Schumacher 2016-07-31 09:58:48 UTC
Fixed in trunk, 8.5.x and 8.0.x. Should be released with 8.0.37, 8.5.5 and 9.0.0.M10.

Thanks for the work and the initial patch.