From 2b3d811ea99830629c9012cebf67ca65443f69dd Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 15 Nov 2023 07:26:29 -0600 Subject: [PATCH] Don't track mounts for `newResource()` that doesn't exist (#10886) * Don't track mounts for newResource() that doesn't exist. * Cannot dereference if not tracking both resources that use the JAR. * Only mount the root. Check if root already mounted in mountIfNeeded --------- Co-authored-by: gregw --- .../jetty/util/resource/FileSystemPool.java | 20 ++- .../resource/ResourceFactoryInternals.java | 30 +++- .../resource/MountedPathResourceTest.java | 9 +- .../jetty/util/resource/PathResourceTest.java | 144 ++++++++++++++++++ 4 files changed, 189 insertions(+), 14 deletions(-) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java index 2908a0833773..81db00791eef 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileSystemPool.java @@ -108,27 +108,28 @@ Mount mount(URI uri) throws IOException throw new IllegalArgumentException("not an supported scheme: " + uri); FileSystem fileSystem = null; + URI jarURIRoot = toJarURIRoot(uri); try (AutoLock ignore = poolLock.lock()) { try { - fileSystem = FileSystems.newFileSystem(uri, ENV_MULTIRELEASE_RUNTIME); + fileSystem = FileSystems.newFileSystem(jarURIRoot, ENV_MULTIRELEASE_RUNTIME); if (LOG.isDebugEnabled()) - LOG.debug("Mounted new FS {}", uri); + LOG.debug("Mounted new FS {}", jarURIRoot); } catch (FileSystemAlreadyExistsException fsaee) { - fileSystem = Paths.get(uri).getFileSystem(); + fileSystem = Paths.get(jarURIRoot).getFileSystem(); if (LOG.isDebugEnabled()) - LOG.debug("Using existing FS {}", uri); + LOG.debug("Using existing FS {}", jarURIRoot); } catch (ProviderNotFoundException pnfe) { - throw new IllegalArgumentException("Unable to mount FileSystem from unsupported URI: " + uri, pnfe); + throw new IllegalArgumentException("Unable to mount FileSystem from unsupported URI: " + jarURIRoot, pnfe); } // use root FS URI so that pool key/release/sweep is sane URI rootURI = fileSystem.getPath("/").toUri(); - Mount mount = new Mount(rootURI, new MountedPathResource(uri)); + Mount mount = new Mount(rootURI, new MountedPathResource(jarURIRoot)); retain(rootURI, fileSystem, mount); return mount; } @@ -139,6 +140,13 @@ Mount mount(URI uri) throws IOException } } + private URI toJarURIRoot(URI uri) + { + String rawURI = uri.toASCIIString(); + int idx = rawURI.indexOf("!/"); + return URI.create(rawURI.substring(0, idx + 2)); + } + private void unmount(URI fsUri) { try (AutoLock ignore = poolLock.lock()) diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java index 1bc7728382f1..a870ae70596c 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactoryInternals.java @@ -127,23 +127,37 @@ static boolean isSupported(String str) return RESOURCE_FACTORIES.getBest(str) != null; } - static class Closeable implements ResourceFactory.Closeable + interface Tracking { + int getTrackingCount(); + } + + static class Closeable implements ResourceFactory.Closeable, Tracking + { + private boolean closed = false; private final CompositeResourceFactory _compositeResourceFactory = new CompositeResourceFactory(); @Override public Resource newResource(URI uri) { + if (closed) + throw new IllegalStateException("Unable to create new Resource on closed ResourceFactory"); return _compositeResourceFactory.newResource(uri); } @Override public void close() { + closed = true; for (FileSystemPool.Mount mount : _compositeResourceFactory.getMounts()) IO.close(mount); _compositeResourceFactory.clearMounts(); } + + public int getTrackingCount() + { + return _compositeResourceFactory.getMounts().size(); + } } static class LifeCycle extends AbstractLifeCycle implements ResourceFactory.LifeCycle @@ -153,6 +167,7 @@ static class LifeCycle extends AbstractLifeCycle implements ResourceFactory.Life @Override public Resource newResource(URI uri) { + // TODO: add check that LifeCycle is started before allowing this method to be used? return _compositeResourceFactory.newResource(uri); } @@ -227,14 +242,25 @@ public Resource newResource(URI uri) * * @param uri The URI to mount that may require a FileSystem (e.g. "jar:file://tmp/some.jar!/directory/file.txt") * @return A reference counted {@link FileSystemPool.Mount} for that file system or null. Callers should call - * {@link FileSystemPool.Mount#close()} once they no longer require any resources from a mounted resource. + * {@link FileSystemPool.Mount#close()} once they no longer require this specific Mount. * @throws IllegalArgumentException If the uri could not be mounted. */ private FileSystemPool.Mount mountIfNeeded(URI uri) { + // do not mount if it is not a jar URI String scheme = uri.getScheme(); if (!"jar".equalsIgnoreCase(scheme)) return null; + + // Do not mount if we have already mounted + // TODO there is probably a better way of doing this other than string comparisons + String uriString = uri.toASCIIString(); + for (FileSystemPool.Mount mount : _mounts) + { + if (uriString.startsWith(mount.root().toString())) + return null; + } + try { return FileSystemPool.INSTANCE.mount(uri); diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java index 559791764192..10631a6d6cf9 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MountedPathResourceTest.java @@ -397,7 +397,7 @@ public void testMountByJarNameClosable() assertThat(FileSystemPool.INSTANCE.mounts().size(), is(1)); int mountCount = FileSystemPool.INSTANCE.getReferenceCount(uriRoot); - assertThat(mountCount, is(4)); + assertThat(mountCount, is(1)); } assertThat(FileSystemPool.INSTANCE.mounts().size(), is(0)); @@ -433,13 +433,10 @@ public void testMountByJarNameLifeCycle() throws Exception assertThat(FileSystemPool.INSTANCE.mounts().size(), is(1)); int mountCount = FileSystemPool.INSTANCE.getReferenceCount(uriRoot); - assertThat(mountCount, is(4)); + assertThat(mountCount, is(1)); String dump = resourceFactory.dump(); - assertThat(dump, containsString("newResourceReferences size=4")); + assertThat(dump, containsString("newResourceReferences size=1")); assertThat(dump, containsString(uriRoot.toASCIIString())); - assertThat(dump, containsString(uriRez.toASCIIString())); - assertThat(dump, containsString(uriDeep.toASCIIString())); - assertThat(dump, containsString(uriZzz.toASCIIString())); } finally { diff --git a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java index 46a7d3709bf5..b3ea77bb4ee5 100644 --- a/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java +++ b/jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/PathResourceTest.java @@ -18,6 +18,7 @@ import java.net.URI; import java.nio.channels.ReadableByteChannel; import java.nio.charset.StandardCharsets; +import java.nio.file.ClosedFileSystemException; import java.nio.file.FileSystem; import java.nio.file.FileSystemException; import java.nio.file.FileSystems; @@ -54,6 +55,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeTrue; @@ -236,6 +238,148 @@ public void testGetPathTo(WorkDir workDir) throws IOException } } + @Test + public void testJarMountNonExistent(WorkDir workDir) throws IOException + { + Path tmpPath = workDir.getEmptyPathDir(); + Path testJar = tmpPath.resolve("test.jar"); + + URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/"); + + Map env = new HashMap<>(); + env.put("create", "true"); + try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env)) + { + zipfs.getPath("/"); + } + try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable()) + { + Resource resBadDir = resourceFactory.newResource(jarUri.toASCIIString() + "does-not-exist/"); + assertNull(resBadDir); + Resource resBadFile = resourceFactory.newResource(jarUri.toASCIIString() + "bad/file.txt"); + assertNull(resBadFile); + + if (resourceFactory instanceof ResourceFactoryInternals.Tracking tracking) + { + assertThat(tracking.getTrackingCount(), is(1)); + } + } + } + + @Test + public void testMountsForSameJar(WorkDir workDir) throws IOException + { + Path tmpPath = workDir.getEmptyPathDir(); + Path testJar = tmpPath.resolve("test.jar"); + + Map env = new HashMap<>(); + env.put("create", "true"); + + URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/"); + try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env)) + { + Path root = zipfs.getPath("/"); + Files.writeString(root.resolve("one.txt"), "Contents of one.txt", StandardCharsets.UTF_8); + + Path dir = root.resolve("datainf"); + Files.createDirectory(dir); + Files.writeString(dir.resolve("two.txt"), "Contents of two.txt", StandardCharsets.UTF_8); + } + + try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable()) + { + Resource oneTxt = resourceFactory.newResource(jarUri.toASCIIString() + "one.txt"); + assertTrue(Resources.isReadableFile(oneTxt)); + Resource twoTxt = resourceFactory.newResource(jarUri.toASCIIString() + "datainf/two.txt"); + assertTrue(Resources.isReadableFile(twoTxt)); + + if (resourceFactory instanceof ResourceFactoryInternals.Tracking tracking) + { + assertThat(tracking.getTrackingCount(), is(1)); + } + } + } + + @Test + public void testMountsForSameJarDifferentResourceFactories(WorkDir workDir) throws IOException + { + Path tmpPath = workDir.getEmptyPathDir(); + Path testJar = tmpPath.resolve("test.jar"); + + Map env = new HashMap<>(); + env.put("create", "true"); + + URI jarUri = URIUtil.uriJarPrefix(testJar.toUri(), "!/"); + try (FileSystem zipfs = FileSystems.newFileSystem(jarUri, env)) + { + Path root = zipfs.getPath("/"); + Files.writeString(root.resolve("one.txt"), "Contents of one.txt", StandardCharsets.UTF_8); + + Path dir = root.resolve("datainf"); + Files.createDirectory(dir); + Files.writeString(dir.resolve("two.txt"), "Contents of two.txt", StandardCharsets.UTF_8); + } + + assertThat(FileSystemPool.INSTANCE.mounts(), is(empty())); + + try (ResourceFactory.Closeable resourceFactory1 = ResourceFactory.closeable(); + ResourceFactory.Closeable resourceFactory2 = ResourceFactory.closeable()) + { + Resource oneTxt = resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt"); + assertTrue(Resources.isReadableFile(oneTxt)); + + Resource oneTxt2 = resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt"); + assertTrue(Resources.isReadableFile(oneTxt)); + + Resource twoTxt = resourceFactory2.newResource(jarUri.toASCIIString() + "datainf/two.txt"); + assertTrue(Resources.isReadableFile(twoTxt)); + + assertThat("Should see only 1 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(1)); + + if (resourceFactory1 instanceof ResourceFactoryInternals.Tracking tracking) + { + assertThat(tracking.getTrackingCount(), is(1)); + } + + if (resourceFactory2 instanceof ResourceFactoryInternals.Tracking tracking) + { + assertThat(tracking.getTrackingCount(), is(1)); + } + + // Close Resource Factory 1 + resourceFactory1.close(); + + if (resourceFactory1 instanceof ResourceFactoryInternals.Tracking tracking) + { + assertThat(tracking.getTrackingCount(), is(0)); + } + + // Resource one still works because factory 2 is holding filesystem open + assertThat(IO.toString(oneTxt.newInputStream()), is("Contents of one.txt")); + + // should not be able to use closed ResourceFactory.Closable + assertThrows(IllegalStateException.class, () -> resourceFactory1.newResource(jarUri.toASCIIString() + "one.txt")); + + assertThat("Should see only 1 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(1)); + + Resource oneAlt = resourceFactory2.newResource(jarUri.toASCIIString() + "one.txt"); + assertTrue(Resources.isReadableFile(oneAlt)); + + // Close Resource Factory 2 + resourceFactory2.close(); + + // Neither Resource one nor two works because filesystem is now closed + assertThrows(ClosedFileSystemException.class, oneTxt::newInputStream); + assertThrows(ClosedFileSystemException.class, oneTxt2::newInputStream); + + assertThat("Should see only 0 FS Mount", FileSystemPool.INSTANCE.mounts().size(), is(0)); + } + finally + { + assertThat(FileSystemPool.INSTANCE.mounts(), is(empty())); + } + } + @Test public void testJarFileIsAliasFile(WorkDir workDir) throws IOException {