Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make UnionPath behave as paths from ZipFileSystem do. #2

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Make UnionPath behave as paths from ZipFileSystem do. #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
82862aa
Make tests work with jar paths
noeppi-noeppi Mar 15, 2022
3494f50
Move tests for relativize to TestUnionPath and fix path and fs so the…
noeppi-noeppi Mar 15, 2022
e89c092
Some jar fixes
noeppi-noeppi Mar 15, 2022
e14f126
Merge branch 'main' of https://github.com/McModLauncher/securejarhand…
noeppi-noeppi Jul 8, 2022
4d8cf80
Fix UnionPath
noeppi-noeppi Jul 8, 2022
0cd3f12
Merge branch 'main' of https://github.com/McModlauncher/securejarhand…
noeppi-noeppi Jun 14, 2023
bf8c1c0
Merge remote-tracking branch 'upstream-forge/master' into pathfix-forge
noeppi-noeppi Jan 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 1 addition & 53 deletions sm-test/src/test/java/net/minecraftforge/securemodules/test/TestUnionFS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,65 +78,13 @@ void testUnionFileSystemJar() throws Throwable {
var doexist = List.of("cpw/mods/niofs/union/UnionPath.class", "net/minecraftforge/client/event/GuiOpenEvent.class", "cpw/mods/modlauncher/Launcher.class"); //jar 3
var dontexist = List.of("cpw/mods/modlauncher/api/NoIDontExist.class", "net/minecraftforge/client/nonexistent/Nope.class", "Missing.class");
assertAll(
doexist.stream().map(ufs::getPath).map(p->()->assertTrue(Files.exists(p)))
doexist.stream().map(ufs::getPath).map(p->()->assertTrue(Files.exists(p)))
);
assertAll(
dontexist.stream().map(ufs::getPath).map(p->()->assertTrue(Files.notExists(p)))
);
}

@Test
void testRelativize() {
final var dir1 = Paths.get("src", "test", "resources", "dir1").toAbsolutePath().normalize();
final var dir2 = Paths.get("src", "test", "resources", "dir2").toAbsolutePath().normalize();

var fsp = (UnionFileSystemProvider)FileSystemProvider.installedProviders().stream().filter(fs-> fs.getScheme().equals("union")).findFirst().orElseThrow();
var ufs = fsp.newFileSystem((path, base) -> true, dir1, dir2);
var p1 = ufs.getPath("path1");
var p123 = ufs.getPath("path1/path2/path3");
var p11 = ufs.getPath("path1/path1");
var p12 = ufs.getPath("path1/path2");
var p13 = ufs.getPath("path1/path3");
var p23 = ufs.getPath("path2/path3");
var p13plus = ufs.getPath("path1/path3");
assertAll(
()->assertEquals("path2/path3", p1.relativize(p123).toString()),
()->assertEquals("../..", p123.relativize(p1).toString()),
()->assertEquals("path1", p1.relativize(p11).toString()),
()->assertEquals("path2", p1.relativize(p12).toString()),
()->assertEquals("path3", p1.relativize(p13).toString()),
()->assertEquals("../../path1/path1", p23.relativize(p11).toString()),
()->assertEquals("../../path1", p123.relativize(p11).toString()),
()->assertEquals(0, p13.relativize(p13plus).getNameCount())
);
}

@Test
void testRelativizeAbsolute() {
final var dir1 = Paths.get("src", "test", "resources", "dir1").toAbsolutePath().normalize();
final var dir2 = Paths.get("src", "test", "resources", "dir2").toAbsolutePath().normalize();

var fsp = (UnionFileSystemProvider)FileSystemProvider.installedProviders().stream().filter(fs-> fs.getScheme().equals("union")).findFirst().orElseThrow();
var ufs = fsp.newFileSystem((path, base) -> true, dir1, dir2);
var p1 = ufs.getPath("/path1");
var p123 = ufs.getPath("/path1/path2/path3");
var p11 = ufs.getPath("/path1/path1");
var p12 = ufs.getPath("/path1/path2");
var p13 = ufs.getPath("/path1/path3");
var p23 = ufs.getPath("/path2/path3");
var p13plus = ufs.getPath("/path1/path3");
assertAll(
()->assertEquals("path2/path3", p1.relativize(p123).toString()),
()->assertEquals("../..", p123.relativize(p1).toString()),
()->assertEquals("path1", p1.relativize(p11).toString()),
()->assertEquals("path2", p1.relativize(p12).toString()),
()->assertEquals("path3", p1.relativize(p13).toString()),
()->assertEquals("../../path1/path1", p23.relativize(p11).toString()),
()->assertEquals("../../path1", p123.relativize(p11).toString()),
()->assertEquals(0, p13.relativize(p13plus).getNameCount())
);
}

@Test
void testPathFiltering() {
final var dir1 = Paths.get("src", "test", "resources", "dir1").toAbsolutePath().normalize();
Expand Down
72 changes: 59 additions & 13 deletions sm-test/src/test/java/net/minecraftforge/securemodules/test/TestUnionPath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,32 @@
import cpw.mods.niofs.union.UnionFileSystem;
import cpw.mods.niofs.union.UnionFileSystemProvider;

import java.io.IOException;
import java.net.URI;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.spi.FileSystemProvider;
import java.util.Arrays;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.*;

public class TestUnionPath {
@Test
void testUnionPath() {
void testUnionPath() throws IOException {
var jarFs = FileSystems.newFileSystem(URI.create("jar:" + Paths.get("src", "test", "resources", "dir1.zip").toUri()), Map.of());
// First test that our tests succed on jar paths:
assertAll("Tests failed for jar file system, this should not happen, tests need to be adjusted", () -> runPathTests(jarFs));

var fsp = (UnionFileSystemProvider)FileSystemProvider.installedProviders().stream().filter(fs-> fs.getScheme().equals("union")).findFirst().orElseThrow();
// Actual base directory is not relevant as we don't access the file system in these tests
var fs = fsp.newFileSystem((path, base) -> true, Paths.get("src").toAbsolutePath().normalize());
runPathTests(fs);
}

private static void runPathTests(FileSystem fs) {
var relUp = fs.getPath("..");
var rel0 = fs.getPath("");
var rel1 = fs.getPath("one");
Expand Down Expand Up @@ -132,7 +144,7 @@ void testUnionPath() {
assertEquals(rel1, rel1223.getParent().getParent().getParent());
assertEquals(rel1, rel12up3.getParent().getParent().getParent());
assertEquals(rel1, rel13.getParent());
assertEquals(rel0, rel13.getParent().getParent());
assertNull(rel13.getParent().getParent());
assertEquals(rel1, rel13slash.getParent());
assertEquals(rel1, rel1slash3.getParent());
assertEquals(abs3, abs32.getParent());
Expand All @@ -146,7 +158,7 @@ void testUnionPath() {

// getNameCount, getName, getFileName, subpath
testNameParts(fs, relUp, "..");
testNameParts(fs, rel0);
testNameParts(fs, rel0, "");
testNameParts(fs, rel1, "one");
testNameParts(fs, rel2, "two");
testNameParts(fs, rel3, "three");
Expand Down Expand Up @@ -186,8 +198,8 @@ void testUnionPath() {
assertEquals(abs13, abs13.normalize());
assertEquals(abs123, abs1223.normalize());
assertEquals(abs13, abs12up3.normalize());
assertEquals(absUpUp1, absUpUp1.normalize());
assertEquals(absUpUp123, absUpUp123.normalize());
assertEquals(abs1, absUpUp1.normalize());
assertEquals(abs123, absUpUp123.normalize());

// resolve
assertEquals(abs32, rel0.resolve(abs32));
Expand All @@ -200,25 +212,59 @@ void testUnionPath() {
assertEquals(abs123, abs1.resolve(rel2).resolve(rel3));
assertEquals(abs32, abs3.resolve(rel0).resolve(rel2));

// relativize is tested in TestUnionFS
assertThrows(IllegalArgumentException.class, () -> rel1.relativize(abs123));
assertThrows(IllegalArgumentException.class, () -> abs1.relativize(rel123));

assertEquals(fs.getPath("two/three"), rel1.relativize(rel123));
assertEquals(fs.getPath("../.."), rel123.relativize(rel1));
assertEquals(rel1, rel1.relativize(fs.getPath("one/one")));
assertEquals(rel3, rel1.relativize(rel13));
assertEquals(fs.getPath("../../one/one"), rel32.relativize(fs.getPath("one/one")));
assertEquals(fs.getPath("../../one"), rel123.relativize(fs.getPath("one/one")));

assertEquals(fs.getPath("two/three"), abs1.relativize(abs123));
assertEquals(fs.getPath("../.."), abs123.relativize(abs1));
assertEquals(rel1, abs1.relativize(fs.getPath("/one/one")));
assertEquals(rel3, abs1.relativize(abs13));
assertEquals(fs.getPath("../../one/one"), abs32.relativize(fs.getPath("/one/one")));
assertEquals(fs.getPath("../../one"), abs123.relativize(fs.getPath("/one/one")));

// getRoot
assertNull(rel0.getRoot());
assertNull(rel123.getRoot());
assertNull(relUpUp1.getRoot());

assertEquals(abs0, abs0.getRoot());
assertEquals(abs0, abs123.getRoot());
assertEquals(abs0, absUpUp1.getRoot());
}

private static void testNameParts(UnionFileSystem fs, Path path, String... names) {
private static void testNameParts(FileSystem fs, Path path, String... names) {
// getNameCount
assertEquals(names.length, path.getNameCount());
if (names.length == 0) {
if (path.isAbsolute()) {
assertEquals(0, path.getNameCount());
} else {
assertEquals(1, path.getNameCount());
assertEquals(fs.getPath(""), path.getName(0));
}
} else {
assertEquals(names.length, path.getNameCount());
}
int nameCount = path.getNameCount();

// getName
assertThrows(IllegalArgumentException.class, () -> path.getName(-1));
assertThrows(IllegalArgumentException.class, () -> path.getName(names.length));
assertThrows(IllegalArgumentException.class, () -> path.getName(nameCount));
for (int i = 0; i < names.length; i++) {
assertEquals(fs.getPath(names[i]), path.getName(i));
}

// getFileName
if (names.length > 0) {
assertEquals(fs.getPath(names[names.length - 1]), path.getFileName());
if (names.length == 0 || (names.length == 1 && names[0].isEmpty())) {
assertNull(path.getFileName());
} else {
assertEquals(fs.getPath(""), path.getFileName());
assertEquals(fs.getPath(names[names.length - 1]), path.getFileName());
}

// subpath, startsWith, endsWith
Expand All @@ -235,7 +281,7 @@ private static void testNameParts(UnionFileSystem fs, Path path, String... names
String absStr = path.isAbsolute() ? "/" : "";
String oppositeAbsStr = path.isAbsolute() ? "" : "/";
assertTrue(path.startsWith(fs.getPath(absStr, fromStart)));
assertTrue(path.endsWith(fs.getPath(absStr, fromEnd)));
assertTrue(path.endsWith(fs.getPath("", fromEnd)));
assertFalse(path.startsWith(fs.getPath(oppositeAbsStr, fromStart)));
if (path.isAbsolute()) {
// Absolute paths can end on non-absolute paths
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/cpw/mods/niofs/union/UnionFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ public DirectoryStream<Path> newDirStream(final UnionPath path, final DirectoryS
try (final var ds = Files.newDirectoryStream(dir, filter)) {
StreamSupport.stream(ds.spliterator(), false)
.filter(p->testFilter(p, bp))
.map(other -> StreamSupport.stream(Spliterators.spliteratorUnknownSize((isSimple ? other : bp.relativize(other)).iterator(), Spliterator.ORDERED), false)
.map(other -> StreamSupport.stream(Spliterators.spliteratorUnknownSize((isSimple ? other : bp.toAbsolutePath().relativize(other.toAbsolutePath())).iterator(), Spliterator.ORDERED), false)
.map(Path::getFileName).map(Path::toString).toArray(String[]::new))
.map(this::fastPath)
.forEachOrdered(allpaths::add);
Expand Down Expand Up @@ -389,7 +389,7 @@ private boolean testFilter(final Path path, final Path basePath) {

var sPath = path.toString();
if (path.getFileSystem() == basePath.getFileSystem()) // Directories, zips will be different file systems.
sPath = basePath.relativize(path).toString().replace('\\', '/');
sPath = basePath.toAbsolutePath().relativize(path.toAbsolutePath()).toString().replace('\\', '/');
if (Files.isDirectory(path))
sPath += '/';
if (sPath.length() > 1 && sPath.startsWith("/"))
Expand Down
68 changes: 39 additions & 29 deletions src/main/java/cpw/mods/niofs/union/UnionPath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
public class UnionPath implements Path {
private final UnionFileSystem fileSystem;
private final boolean absolute;
private final boolean empty;
private final String[] pathParts;

// Store the normalized path after it has been created first
Expand All @@ -35,7 +36,8 @@ public class UnionPath implements Path {
this.fileSystem = fileSystem;
if (pathParts.length == 0) {
this.absolute = false;
this.pathParts = new String[0];
this.pathParts = new String[]{ "" };
this.empty = true;
} else {
StringBuilder joiner = new StringBuilder();
for (int i = 0; i < pathParts.length; i++) {
Expand All @@ -47,7 +49,8 @@ public class UnionPath implements Path {
}
final var longstring = joiner.toString();
this.absolute = longstring.startsWith(UnionFileSystem.SEP_STRING);
this.pathParts = getPathParts(longstring);
this.pathParts = getPathParts(this.absolute, longstring);
this.empty = !this.absolute && this.pathParts.length == 1 && this.pathParts[0].isEmpty();
}
this.normalized = null;
}
Expand All @@ -59,15 +62,22 @@ public class UnionPath implements Path {

private UnionPath(final UnionFileSystem fileSystem, boolean absolute, boolean isNormalized, final String... pathParts) {
this.fileSystem = fileSystem;
this.absolute = absolute;
this.pathParts = pathParts;
if (isNormalized)
this.normalized = this;
else
this.normalized = null;
if (!absolute && (pathParts.length == 0 || (pathParts.length == 1 && pathParts[0].isEmpty()))) {
this.absolute = false;
this.pathParts = new String[]{ "" };
this.empty = true;
} else {
this.absolute = absolute;
this.empty = false;
this.pathParts = pathParts;
if (isNormalized)
this.normalized = this;
else
this.normalized = null;
}
}

private String[] getPathParts(final String longstring) {
private String[] getPathParts(final boolean isAbsolute, final String longstring) {
var clean = longstring.replace('\\', '/');
int startIndex = 0;
List<String> parts = new ArrayList<>();
Expand All @@ -83,7 +93,11 @@ private String[] getPathParts(final String longstring) {
}
startIndex = (index + 1);
}
return parts.toArray(String[]::new);
if (parts.isEmpty() && !isAbsolute) {
return new String[]{ "" };
} else {
return parts.toArray(String[]::new);
}
}

@Override
Expand All @@ -98,28 +112,25 @@ public boolean isAbsolute() {

@Override
public Path getRoot() {
// Found nothing in the docs that say a non-absolute path can't have a root
// although this is uncommon. However, other stuff relies on it so leave it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the other stuff that relies on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment got added in the first PR to fix issues with UnionPath. The basic issue was, that at the time, a lot of projects treated relative paths as if they were absolute, relativizing them with other absolute paths and calling getRoot() on it. iirc, when I was playing around with it, I had to make some changes to modlauncher to get forge starting and then it crashed somewhere in FMLLoader. However that was almost 3 years ago and I don't know, how up-to-date that comment still is.

I would assume, it shouldn't really be much af a problem by now because Jar now uses regular JarFileSystems when there's only one path (that wasn't the case back then) and code seems to work with them as well (which it didn't when that comment was written).

//if (!this.absolute)
// return null;
if (!this.absolute)
return null;
return this.fileSystem.getRoot();
}

@Override
public Path getFileName() {
if (this.pathParts.length > 0) {
if (this.empty) {
return null;
} else if (this.pathParts.length > 0) {
return new UnionPath(this.getFileSystem(), false, this.pathParts[this.pathParts.length - 1]);
} else {
// normally would be null for the empty absolute path and empty string for the empty relative
// path. But again, very much stuff relies on it and there's no current directory for union
// paths, so it does not really matter.
return new UnionPath(this.fileSystem, false);
return this.absolute ? null : new UnionPath(this.fileSystem, false);
}
}

@Override
public Path getParent() {
if (this.pathParts.length > 0) {
if (this.pathParts.length > 1 || (this.absolute && this.pathParts.length == 1)) {
return new UnionPath(this.fileSystem, this.absolute, Arrays.copyOf(this.pathParts,this.pathParts.length - 1));
} else {
return null;
Expand Down Expand Up @@ -197,10 +208,10 @@ public Path normalize() {
case ".":
break;
case "..":
if (normpath.isEmpty() || normpath.getLast().equals("..")) {
// .. on an empty path is allowed, so keep it
if (!this.absolute && (normpath.isEmpty() || normpath.getLast().equals(".."))) {
// .. on an empty path is allowed as long as it is not absolute, so keep it
normpath.addLast(pathPart);
} else {
} else if (!normpath.isEmpty()) {
normpath.removeLast();
}
break;
Expand All @@ -216,9 +227,12 @@ public Path normalize() {
@Override
public Path resolve(final Path other) {
if (other instanceof UnionPath path) {
if (path.isAbsolute()) {
if (path.isAbsolute() || this.empty) {
return path;
}
if (path.empty) {
return this;
}
String[] mergedParts = new String[this.pathParts.length + path.pathParts.length];
System.arraycopy(this.pathParts, 0, mergedParts, 0, this.pathParts.length);
System.arraycopy(path.pathParts, 0, mergedParts, this.pathParts.length, path.pathParts.length);
Expand All @@ -232,11 +246,7 @@ public Path relativize(final Path other) {
if (other.getFileSystem()!=this.getFileSystem()) throw new IllegalArgumentException("Wrong filesystem");
if (other instanceof UnionPath p) {
if (this.absolute != p.absolute) {
// Should not be allowed but union fs relies on it
// also there is no such concept of a current directory for union paths
// meaning absolute and relative paths should have the same effect,
// so we just allow this.
//throw new IllegalArgumentException("Different types of path");
throw new IllegalArgumentException("Different types of path");
}
var length = Math.min(this.pathParts.length, p.pathParts.length);
int i = 0;
Expand Down