This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new cc7a98b57c Fix inconsistent resource metadata with current GET and
PUT/DELETE
cc7a98b57c is described below
commit cc7a98b57c6dc1df21979fcff94a36e068f4456c
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Nov 1 16:35:17 2024 +0000
Fix inconsistent resource metadata with current GET and PUT/DELETE
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.
---
java/org/apache/catalina/WebResourceLockSet.java | 73 ++++++++
.../catalina/webresources/DirResourceSet.java | 199 ++++++++++++++++++---
.../apache/catalina/webresources/FileResource.java | 29 ++-
.../catalina/webresources/LocalStrings.properties | 1 +
4 files changed, 273 insertions(+), 29 deletions(-)
diff --git a/java/org/apache/catalina/WebResourceLockSet.java
b/java/org/apache/catalina/WebResourceLockSet.java
new file mode 100644
index 0000000000..142472c33c
--- /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 a221c3f1c3..02ddc6ec88 100644
--- a/java/org/apache/catalina/webresources/DirResourceSet.java
+++ b/java/org/apache/catalina/webresources/DirResourceSet.java
@@ -22,24 +22,35 @@ import java.io.IOException;
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 class DirResourceSet extends
AbstractFileResourceSet {
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 class DirResourceSet extends
AbstractFileResourceSet {
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 @@ public class DirResourceSet extends AbstractFileResourceSet
{
@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 @@ public class DirResourceSet extends
AbstractFileResourceSet {
}
}
}
+
+
+ /*
+ * 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 f1b674450b..f109893d9e 100644
--- a/java/org/apache/catalina/webresources/FileResource.java
+++ b/java/org/apache/catalina/webresources/FileResource.java
@@ -31,6 +31,8 @@ import java.nio.file.attribute.BasicFileAttributes;
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 class FileResource extends AbstractResource {
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 c964d31291..70d4847faa 100644
--- a/java/org/apache/catalina/webresources/LocalStrings.properties
+++ b/java/org/apache/catalina/webresources/LocalStrings.properties
@@ -33,6 +33,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
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
