From 43b507ebac9d268b1ea3d908e296cc6e46795c00 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Fri, 1 Nov 2024 16:35:17 +0000 Subject: [PATCH] Fix inconsistent resource metadata with current GET and PUT/DELETE Origin: https://github.com/apache/tomcat/commit/43b507ebac9d268b1ea3d908e296cc6e46795c00 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. --- .../apache/catalina/WebResourceLockSet.java | 73 +++++++ .../catalina/webresources/DirResourceSet.java | 199 +++++++++++++++--- .../catalina/webresources/FileResource.java | 29 ++- .../webresources/LocalStrings.properties | 1 + 4 files changed, 273 insertions(+), 29 deletions(-) create mode 100644 java/org/apache/catalina/WebResourceLockSet.java diff --git a/java/org/apache/catalina/WebResourceLockSet.java b/java/org/apache/catalina/WebResourceLockSet.java new file mode 100644 index 000000000000..142472c33cd6 --- /dev/null +++ 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 index a221c3f1c33d..02ddc6ec887c 100644 --- 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 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 index 32ce5bfa62c3..354022f9096d 100644 --- 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 index 67d76d3b3b32..301c13eb8063 100644 --- 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