Fix inconsistent resource metadata with current GET and PUT/DELETE

markt-asf · markt-asf · commit 43b507ebac9d · 2024-11-01T16:35:47.000Z
Concurrent reads and writes (e.g. HTTP GET and PUT / DELETE) for the
same path cause corruption of the FileResource where some of the fields
are set as if the file exists and some as set as if it does not.
diff --git a/java/org/apache/catalina/WebResourceLockSet.java b/java/org/apache/catalina/WebResourceLockSet.java
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * Interface implemented by {@link WebResourceSet} implementations that wish to provide locking functionality.
+ */
+public interface WebResourceLockSet {
+
+    /**
+     * Lock the resource at the provided path for reading. The resource is not required to exist. Read locks are not
+     * exclusive.
+     *
+     * @param path The path to the resource to be locked for reading
+     *
+     * @return The {@link ResourceLock} that must be passed to {@link #unlockForRead(ResourceLock)} to release the lock
+     */
+    ResourceLock lockForRead(String path);
+
+    /**
+     * Release a read lock from the resource associated with the given {@link ResourceLock}.
+     *
+     * @param resourceLock The {@link ResourceLock} associated with the resource for which a read lock should be
+     *                         released
+     */
+    void unlockForRead(ResourceLock resourceLock);
+
+    /**
+     * Lock the resource at the provided path for writing. The resource is not required to exist. Write locks are
+     * exclusive.
+     *
+     * @param path The path to the resource to be locked for writing
+     *
+     * @return The {@link ResourceLock} that must be passed to {@link #unlockForWrite(ResourceLock)} to release the lock
+     */
+    ResourceLock lockForWrite(String path);
+
+    /**
+     * Release the write lock from the resource associated with the given {@link ResourceLock}.
+     *
+     * @param resourceLock The {@link ResourceLock} associated with the resource for which the write lock should be
+     *                         released
+     */
+    void unlockForWrite(ResourceLock resourceLock);
+
+
+    class ResourceLock {
+        public final AtomicInteger count = new AtomicInteger(0);
+        public final ReentrantReadWriteLock reentrantLock = new ReentrantReadWriteLock();
+        public final String key;
+
+        public ResourceLock(String key) {
+            this.key = key;
+        }
+    }
+}
diff --git a/java/org/apache/catalina/webresources/DirResourceSet.java b/java/org/apache/catalina/webresources/DirResourceSet.java
@@ -22,24 +22,35 @@
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.StandardCopyOption;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
 import java.util.Set;
 import java.util.jar.Manifest;
 
 import org.apache.catalina.LifecycleException;
 import org.apache.catalina.WebResource;
+import org.apache.catalina.WebResourceLockSet;
 import org.apache.catalina.WebResourceRoot;
 import org.apache.catalina.WebResourceRoot.ResourceSetType;
 import org.apache.catalina.util.ResourceSet;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.http.RequestUtil;
 
 /**
  * Represents a {@link org.apache.catalina.WebResourceSet} based on a directory.
  */
-public class DirResourceSet extends AbstractFileResourceSet {
+public class DirResourceSet extends AbstractFileResourceSet implements WebResourceLockSet {
 
     private static final Log log = LogFactory.getLog(DirResourceSet.class);
 
+    private boolean caseSensitive = true;
+
+    private Map<String,ResourceLock> resourceLocksByPath = new HashMap<>();
+    private Object resourceLocksByPathLock = new Object();
+
+
     /**
      * A no argument constructor is required for this to work with the digester.
      */
@@ -91,22 +102,33 @@ public WebResource getResource(String path) {
         String webAppMount = getWebAppMount();
         WebResourceRoot root = getRoot();
         if (path.startsWith(webAppMount)) {
-            File f = file(path.substring(webAppMount.length()), false);
-            if (f == null) {
-                return new EmptyResource(root, path);
-            }
-            if (!f.exists()) {
-                return new EmptyResource(root, path, f);
-            }
-            if (f.isDirectory() && path.charAt(path.length() - 1) != '/') {
-                path = path + '/';
+            /*
+             * Lock the path for reading until the WebResource has been constructed. The lock prevents concurrent reads
+             * and writes (e.g. HTTP GET and PUT / DELETE) for the same path causing corruption of the FileResource
+             * where some of the fields are set as if the file exists and some as set as if it does not.
+             */
+            ResourceLock lock = lockForRead(path);
+            try {
+                File f = file(path.substring(webAppMount.length()), false);
+                if (f == null) {
+                    return new EmptyResource(root, path);
+                }
+                if (!f.exists()) {
+                    return new EmptyResource(root, path, f);
+                }
+                if (f.isDirectory() && path.charAt(path.length() - 1) != '/') {
+                    path = path + '/';
+                }
+                return new FileResource(root, path, f, isReadOnly(), getManifest(), this, lock.key);
+            } finally {
+                unlockForRead(lock);
             }
-            return new FileResource(root, path, f, isReadOnly(), getManifest());
         } else {
             return new EmptyResource(root, path);
         }
     }
 
+
     @Override
     public String[] list(String path) {
         checkPath(path);
@@ -246,32 +268,42 @@ public boolean write(String path, InputStream is, boolean overwrite) {
             return false;
         }
 
-        File dest = null;
         String webAppMount = getWebAppMount();
-        if (path.startsWith(webAppMount)) {
+        if (!path.startsWith(webAppMount)) {
+            return false;
+        }
+
+        File dest = null;
+        /*
+         * Lock the path for writing until the write is complete. The lock prevents concurrent reads and writes (e.g.
+         * HTTP GET and PUT / DELETE) for the same path causing corruption of the FileResource where some of the fields
+         * are set as if the file exists and some as set as if it does not.
+         */
+        ResourceLock lock = lockForWrite(path);
+        try {
             dest = file(path.substring(webAppMount.length()), false);
             if (dest == null) {
                 return false;
             }
-        } else {
-            return false;
-        }
 
-        if (dest.exists() && !overwrite) {
-            return false;
-        }
+            if (dest.exists() && !overwrite) {
+                return false;
+            }
 
-        try {
-            if (overwrite) {
-                Files.copy(is, dest.toPath(), StandardCopyOption.REPLACE_EXISTING);
-            } else {
-                Files.copy(is, dest.toPath());
+            try {
+                if (overwrite) {
+                    Files.copy(is, dest.toPath(), StandardCopyOption.REPLACE_EXISTING);
+                } else {
+                    Files.copy(is, dest.toPath());
+                }
+            } catch (IOException ioe) {
+                return false;
             }
-        } catch (IOException ioe) {
-            return false;
-        }
 
-        return true;
+            return true;
+        } finally {
+            unlockForWrite(lock);
+        }
     }
 
     @Override
@@ -286,6 +318,7 @@ protected void checkType(File file) {
     @Override
     protected void initInternal() throws LifecycleException {
         super.initInternal();
+        caseSensitive = isCaseSensitive();
         // Is this an exploded web application?
         if (getWebAppMount().equals("")) {
             // Look for a manifest
@@ -299,4 +332,114 @@ protected void initInternal() throws LifecycleException {
             }
         }
     }
+
+
+    /*
+     * Determines if this ResourceSet is based on a case sensitive file system or not.
+     */
+    private boolean isCaseSensitive() {
+        try {
+            String canonicalPath = getFileBase().getCanonicalPath();
+            File upper = new File(canonicalPath.toUpperCase(Locale.ENGLISH));
+            if (!canonicalPath.equals(upper.getCanonicalPath())) {
+                return true;
+            }
+            File lower = new File(canonicalPath.toLowerCase(Locale.ENGLISH));
+            if (!canonicalPath.equals(lower.getCanonicalPath())) {
+                return true;
+            }
+            /*
+             * Both upper and lower case versions of the current fileBase have the same canonical path so the file
+             * system must be case insensitive.
+             */
+        } catch (IOException ioe) {
+            log.warn(sm.getString("dirResourceSet.isCaseSensitive.fail", getFileBase().getAbsolutePath()), ioe);
+        }
+
+        return false;
+    }
+
+
+    private String getLockKey(String path) {
+        // Normalize path to ensure that the same key is used for the same path.
+        String normalisedPath = RequestUtil.normalize(path);
+        if (caseSensitive) {
+            return normalisedPath;
+        }
+        return normalisedPath.toLowerCase(Locale.ENGLISH);
+    }
+
+
+    @Override
+    public ResourceLock lockForRead(String path) {
+        String key = getLockKey(path);
+        ResourceLock resourceLock = null;
+        synchronized (resourceLocksByPathLock) {
+            /*
+             * Obtain the ResourceLock and increment the usage count inside the sync to ensure that that map always has
+             * a consistent view of the currently "in-use" ResourceLocks.
+             */
+            resourceLock = resourceLocksByPath.get(key);
+            if (resourceLock == null) {
+                resourceLock = new ResourceLock(key);
+            }
+            resourceLock.count.incrementAndGet();
+        }
+        // Obtain the lock outside the sync as it will block if there is a current write lock.
+        resourceLock.reentrantLock.readLock().lock();
+        return resourceLock;
+    }
+
+
+    @Override
+    public void unlockForRead(ResourceLock resourceLock) {
+        // Unlock outside the sync as there is no need to do it inside.
+        resourceLock.reentrantLock.readLock().unlock();
+        synchronized (resourceLocksByPathLock) {
+            /*
+             * Decrement the usage count and remove ResourceLocks no longer required inside the sync to ensure that that
+             * map always has a consistent view of the currently "in-use" ResourceLocks.
+             */
+            if (resourceLock.count.decrementAndGet() == 0) {
+                resourceLocksByPath.remove(resourceLock.key);
+            }
+        }
+    }
+
+
+    @Override
+    public ResourceLock lockForWrite(String path) {
+        String key = getLockKey(path);
+        ResourceLock resourceLock = null;
+        synchronized (resourceLocksByPathLock) {
+            /*
+             * Obtain the ResourceLock and increment the usage count inside the sync to ensure that that map always has
+             * a consistent view of the currently "in-use" ResourceLocks.
+             */
+            resourceLock = resourceLocksByPath.get(key);
+            if (resourceLock == null) {
+                resourceLock = new ResourceLock(key);
+            }
+            resourceLock.count.incrementAndGet();
+        }
+        // Obtain the lock outside the sync as it will block if there are any other current locks.
+        resourceLock.reentrantLock.writeLock().lock();
+        return resourceLock;
+    }
+
+
+    @Override
+    public void unlockForWrite(ResourceLock resourceLock) {
+        // Unlock outside the sync as there is no need to do it inside.
+        resourceLock.reentrantLock.writeLock().unlock();
+        synchronized (resourceLocksByPathLock) {
+            /*
+             * Decrement the usage count and remove ResourceLocks no longer required inside the sync to ensure that that
+             * map always has a consistent view of the currently "in-use" ResourceLocks.
+             */
+            if (resourceLock.count.decrementAndGet() == 0) {
+                resourceLocksByPath.remove(resourceLock.key);
+            }
+        }
+    }
 }
diff --git a/java/org/apache/catalina/webresources/FileResource.java b/java/org/apache/catalina/webresources/FileResource.java
@@ -31,6 +31,8 @@
 import java.security.cert.Certificate;
 import java.util.jar.Manifest;
 
+import org.apache.catalina.WebResourceLockSet;
+import org.apache.catalina.WebResourceLockSet.ResourceLock;
 import org.apache.catalina.WebResourceRoot;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
@@ -62,10 +64,20 @@ public class FileResource extends AbstractResource {
     private final boolean readOnly;
     private final Manifest manifest;
     private final boolean needConvert;
+    private final WebResourceLockSet lockSet;
+    private final String lockKey;
 
     public FileResource(WebResourceRoot root, String webAppPath, File resource, boolean readOnly, Manifest manifest) {
+        this(root, webAppPath, resource, readOnly, manifest, null, null);
+    }
+
+
+    public FileResource(WebResourceRoot root, String webAppPath, File resource, boolean readOnly, Manifest manifest,
+            WebResourceLockSet lockSet, String lockKey) {
         super(root, webAppPath);
         this.resource = resource;
+        this.lockSet = lockSet;
+        this.lockKey = lockKey;
 
         if (webAppPath.charAt(webAppPath.length() - 1) == '/') {
             String realName = resource.getName() + '/';
@@ -117,7 +129,22 @@ public boolean delete() {
         if (readOnly) {
             return false;
         }
-        return resource.delete();
+        /*
+         * Lock the path for writing until the delete is complete. The lock prevents concurrent reads and writes (e.g.
+         * HTTP GET and PUT / DELETE) for the same path causing corruption of the FileResource where some of the fields
+         * are set as if the file exists and some as set as if it does not.
+         */
+        ResourceLock lock = null;
+        if (lockSet != null) {
+            lock = lockSet.lockForWrite(lockKey);
+        }
+        try {
+            return resource.delete();
+        } finally {
+            if (lockSet != null) {
+                lockSet.unlockForWrite(lock);
+            }
+        }
     }
 
     @Override
diff --git a/java/org/apache/catalina/webresources/LocalStrings.properties b/java/org/apache/catalina/webresources/LocalStrings.properties
@@ -36,6 +36,7 @@ cachedResource.invalidURL=Unable to create an instance of CachedResourceURLStrea
 
 classpathUrlStreamHandler.notFound=Unable to load the resource [{0}] using the thread context class loader or the current class''s class loader
 
+dirResourceSet.isCaseSensitive.fail=Error trying to determine if file system at [{0}] is case sensitive so assuming it is not case sensitive
 dirResourceSet.manifestFail=Failed to read manifest from [{0}]
 dirResourceSet.notDirectory=The directory specified by base and internal path [{0}]{1}[{2}] does not exist.
 dirResourceSet.writeNpe=The input stream may not be null
