436 lines
17 KiB
Diff
436 lines
17 KiB
Diff
From 43b507ebac9d268b1ea3d908e296cc6e46795c00 Mon Sep 17 00:00:00 2001
|
|
From: Mark Thomas <markt@apache.org>
|
|
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<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
|
|
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
|