UnifiedTree.isRecursiveLink() links check may silently ignore some links

- Main fix: the child path created from the link should use
`realParentPath` to avoid silent `NoSuchFileException`'s if the
constructed link does not match actual (fully resolved) file system
state of the local file we are checking. Original code run into (silent)
exceptions and because of that was not able to detect some of possible
recursive links.
- Removed pattern checking for "../X" because backwards links can also
be created with absolute paths, as seen in
`Bug_185247_recursiveLinks.test5_linkParentDirectoyTwiceWithAbsolutePath(boolean)`
- Added regression test `SymlinkResourceTest.testGithubBug2220`.
- Added `UnifiedTree` methods to set/read
`disable_advanced_recursive_link_checks` flag in tests so we can test
both variants of link checking code.
- Updated existing tests to test both advanced / simple link checks
(where possible): `SymlinkResourceTest`, `Bug_185247_recursiveLinks` &
`Bug_185247_LinuxTests`
- Improved `WorkspaceResetExtension` to properly report test names (it
only reported `false` or `true` for parametrized tests).

Fixes #2220

Author: iloveeclipse

resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/UnifiedTree.java

@@ -438,9 +438,6 @@ private static class PatternHolder {
 		//Pattern: A UNIX or Windows relative path that just points backward
 		static final Pattern TRIVIAL_SYMLINK_PATTERN = Pattern.compile( //
 				Platform.OS.isWindows() ? "\\.[.\\\\]*" : "\\.[./]*"); //$NON-NLS-1$//$NON-NLS-2$
-
-		static final Pattern REPEATING_BACKWARDS_PATTERN = Pattern.compile( //
-				Platform.OS.isWindows() ? "(\\.\\.\\\\)+.*" : "(\\.\\./)+.*"); //$NON-NLS-1$//$NON-NLS-2$
 	}
 
 	/**
@@ -522,20 +519,10 @@ private boolean isRecursiveLink(IFileStore parentStore, IFileInfo localInfo) {
 			Path realParentPath = parent.toRealPath();
 			if (disable_advanced_recursive_link_checks) {
 				// Multiple ../ backwards links can go outside the project tree
-				if (linkTarget != null && PatternHolder.REPEATING_BACKWARDS_PATTERN.matcher(linkTarget).matches()) {
-					Path targetPath = parent.resolve(linkTarget).normalize();
-
-					// Recursive if literal target points to the literal parent of this tree
-					if (parent.normalize().startsWith(targetPath)) {
+				if (linkTarget != null) {
+					if (isRecursiveBackwardsLink(realParentPath, linkTarget)) {
 						return true;
 					}
-
-					// Recursive if resolved target points to the resolved parent of this tree
-					Path realTargetPath = targetPath.toRealPath();
-					if (realParentPath.startsWith(realTargetPath)) {
-						return true;
-					}
-
 					// If link is outside the project tree, consider as non recursive
 					// The link still can create recursion in the tree, but we can't detect it here.
 				}
@@ -573,6 +560,51 @@ private boolean isRecursiveLink(IFileStore parentStore, IFileInfo localInfo) {
 		return false;
 	}
 
+	/**
+	 * @param realParentPath    real parent path object obtained as a result
+	 *                          of @code{Path.toRealPath()}
+	 * @param linkTarget        the link target path as a string, may be relative or
+	 *                          absolute
+	 * @return true if the given target points backwards recursively to the given
+	 *         parent path
+	 * @throws IOException
+	 */
+	private static boolean isRecursiveBackwardsLink(Path realParentPath, String linkTarget)
+			throws IOException {
+		// Cheap test first: literal target points to the literal parent
+		Path normalizedLink = realParentPath.resolve(linkTarget).normalize();
+		if (realParentPath.startsWith(normalizedLink)) {
+			return true;
+		}
+		// Next check costs more time because it does real IO when resolving paths
+		Path realTarget = normalizedLink.toRealPath();
+		if (realParentPath.startsWith(realTarget)) {
+			return true;
+		}
+		return false;
+	}
+
+	/**
+	 * Disable or enable advanced recursive link checks. Advanced link checks may
+	 * hide valid directories in some cases, see bug 537449.
+	 *
+	 * @param enable <code>true</code> to enable advanced recursive link checks,
+	 *               <code>false</code> to disable them.
+	 */
+	public static void enableAdvancedRecursiveLinkChecks(boolean enable) {
+		disable_advanced_recursive_link_checks = !enable;
+	}
+
+	/**
+	 * Returns whether advanced recursive link checks are enabled.
+	 *
+	 * @return <code>true</code> if advanced recursive link checks are enabled,
+	 *         <code>false</code> otherwise.
+	 */
+	public static boolean isAdvancedRecursiveLinkChecksEnabled() {
+		return !disable_advanced_recursive_link_checks;
+	}
+
 	protected boolean isValidLevel(int currentLevel, int depth) {
 		return switch (depth) {
 		case IResource.DEPTH_INFINITE -> true;

resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/SymlinkResourceTest.java

@@ -21,28 +21,33 @@
 import static org.eclipse.core.tests.resources.ResourceTestUtil.createInWorkspace;
 import static org.eclipse.core.tests.resources.ResourceTestUtil.createTestMonitor;
 import static org.eclipse.core.tests.resources.ResourceTestUtil.waitForRefresh;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assume.assumeTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import org.eclipse.core.filesystem.EFS;
 import org.eclipse.core.filesystem.IFileStore;
+import org.eclipse.core.internal.localstore.UnifiedTree;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.core.resources.IResource;
 import org.eclipse.core.resources.IResourceVisitor;
 import org.eclipse.core.resources.IWorkspaceRunnable;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IPath;
-import org.eclipse.core.tests.resources.WorkspaceTestRule;
-import org.junit.Rule;
-import org.junit.Test;
-
+import org.eclipse.core.runtime.Platform.OS;
+import org.eclipse.core.tests.resources.util.WorkspaceResetExtension;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+@ExtendWith(WorkspaceResetExtension.class)
 public class SymlinkResourceTest {
 
-	@Rule
-	public WorkspaceTestRule workspaceRule = new WorkspaceTestRule();
-
 	private void mkLink(IFileStore dir, String src, String tgt, boolean isDir) throws CoreException, IOException {
 		createSymLink(dir.toLocalFile(EFS.NONE, createTestMonitor()), src, tgt, isDir);
 	}
@@ -70,6 +75,88 @@ protected void createBug358830Structure(IFileStore rootDir) throws CoreException
 		mkLink(folderA, "link", IPath.fromOSString("../").toOSString(), true);
 	}
 
+	/**
+	 * Test a case of both recursive and non recursive symbolic links that uncovered
+	 * issue described in <a href=
+	 * "https://github.com/eclipse-platform/eclipse.platform/issues/2220">GitHub bug
+	 * 2220</a>.
+	 *
+	 * <pre>{@code
+	 *      /A/B -> /X/Y/Z  	(B is symbolic link as precondition)
+	 *      /A/B/C/D -> ../../D (non-recursive)
+	 *      /A/B/C/E -> ../../Z (recursive)
+	 * }</pre>
+	 *
+	 * The starting path /A/B/C is already based on link. The real path of start is
+	 * /X/Y/Z/C so ../../Z points (recursive) to /X/Y/Z. /X/Y/D and /X/Y/Z is what
+	 * we expect to resolve but we fail in both cases with NoSuchFileException.
+	 */
+	@ParameterizedTest
+	@ValueSource(booleans = { false, true })
+	public void testGithubBug2220(boolean useAdvancedLinkCheck) throws Exception {
+		assumeTrue(canCreateSymLinks(), "only relevant for platforms supporting symbolic links");
+		assumeTrue(!OS.isWindows(), "Windows file system handles recursive links differently");
+		final boolean originalValue = UnifiedTree.isAdvancedRecursiveLinkChecksEnabled();
+		try {
+			UnifiedTree.enableAdvancedRecursiveLinkChecks(useAdvancedLinkCheck);
+			IProject project = getWorkspace().getRoot().getProject("testGithubBug2220");
+			createInWorkspace(project);
+
+			/* Re-use projects which are cleaned up automatically */
+			getWorkspace().run((IWorkspaceRunnable) monitor -> {
+				/* delete open project because we must re-open with BACKGROUND_REFRESH */
+				project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
+				project.create(null);
+				try {
+					createGithubBug2220Structure(EFS.getStore(project.getLocationURI()));
+				} catch (IOException e) {
+					throw new IllegalStateException("unexpected IOException occurred", e);
+				}
+				// Bug only happens with BACKGROUND_REFRESH.
+				project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
+			}, null);
+
+			// wait for BACKGROUND_REFRESH to complete.
+			waitForRefresh();
+			project.accept(new IResourceVisitor() {
+				int resourceCount = 0;
+
+				@Override
+				public boolean visit(IResource resource) {
+					resourceCount++;
+					// We have 1 root + .settings + prefs + + .project + 10 elements --> 14 elements
+					// to visit at most
+					System.out.println(resourceCount + " visited: " + resource.getFullPath());
+					assertTrue(resourceCount <= 15, "Expected max 15 elements to visit, got: " + resourceCount);
+					return true;
+				}
+			});
+		} finally {
+			UnifiedTree.enableAdvancedRecursiveLinkChecks(originalValue);
+		}
+	}
+
+	/**
+	 * <pre>{@code
+	 *      /A/B -> /X/Y/Z  	(B is symbolic link as precondition)
+	 *      /A/B/C/D -> ../../D (non-recursive)
+	 *      /A/B/C/E -> ../../Z (recursive)
+	 * }</pre>
+	 *
+	 * The starting path /A/B/C is already based on link. The real path of start is
+	 * /X/Y/Z/C so ../../Z points (recursive) to /X/Y/Z.
+	 */
+	protected void createGithubBug2220Structure(IFileStore rootDir) throws CoreException, IOException {
+		Path root = rootDir.toLocalFile(EFS.NONE, createTestMonitor()).toPath();
+		Files.createDirectories(root.resolve("A"));
+		Files.createDirectories(root.resolve("X/Y/Z"));
+		Files.createDirectories(root.resolve("X/Y/D"));
+		Files.createSymbolicLink(root.resolve("A/B"), root.resolve("X/Y/Z"));
+		Files.createDirectories(root.resolve("A/B/C"));
+		Files.createSymbolicLink(root.resolve("A/B/C/D"), Paths.get("../../D"));
+		Files.createSymbolicLink(root.resolve("A/B/C/E"), Paths.get("../../Z"));
+	}
+
 	/**
 	 * Test a very specific case of mutually recursive symbolic links:
 	 * <pre> {@code
@@ -85,7 +172,7 @@ protected void createBug358830Structure(IFileStore rootDir) throws CoreException
 	 */
 	@Test
 	public void testBug232426() throws Exception {
-		assumeTrue("only relevant for platforms supporting symbolic links", canCreateSymLinks());
+		assumeTrue(canCreateSymLinks(), "only relevant for platforms supporting symbolic links");
 
 		IProject project = getWorkspace().getRoot().getProject("Project");
 		createInWorkspace(project);
@@ -118,36 +205,42 @@ public boolean visit(IResource resource) {
 		});
 	}
 
-	@Test
-	public void testBug358830() throws Exception {
-		assumeTrue("only relevant for platforms supporting symbolic links", canCreateSymLinks());
-
-		IProject project = getWorkspace().getRoot().getProject("Project");
-		createInWorkspace(project);
-		/* Re-use projects which are cleaned up automatically */
-		getWorkspace().run((IWorkspaceRunnable) monitor -> {
-			/* delete open project because we must re-open with BACKGROUND_REFRESH */
-			project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
-			project.create(null);
-			try {
-				createBug358830Structure(EFS.getStore(project.getLocationURI()));
-			} catch (IOException e) {
-				throw new IllegalStateException("unexpected IOException occurred", e);
-			}
-			project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
-		}, null);
-
-		//wait for BACKGROUND_REFRESH to complete.
-		waitForRefresh();
-		final int resourceCount[] = new int[] {0};
-		project.accept(resource -> {
-			resourceCount[0]++;
-			return true;
-		});
-		// We have 1 root + 1 folder + 1 file (.project)
-		// + .settings / resources prefs
-		// --> 5 elements to visit
-		assertEquals(5, resourceCount[0]);
+	@ParameterizedTest
+	@ValueSource(booleans = { false, true })
+	public void testBug358830(boolean useAdvancedLinkCheck) throws Exception {
+		assumeTrue(canCreateSymLinks(), "only relevant for platforms supporting symbolic links");
+		final boolean originalValue = UnifiedTree.isAdvancedRecursiveLinkChecksEnabled();
+		try {
+			UnifiedTree.enableAdvancedRecursiveLinkChecks(useAdvancedLinkCheck);
+			IProject project = getWorkspace().getRoot().getProject("Project");
+			createInWorkspace(project);
+			/* Re-use projects which are cleaned up automatically */
+			getWorkspace().run((IWorkspaceRunnable) monitor -> {
+				/* delete open project because we must re-open with BACKGROUND_REFRESH */
+				project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
+				project.create(null);
+				try {
+					createBug358830Structure(EFS.getStore(project.getLocationURI()));
+				} catch (IOException e) {
+					throw new IllegalStateException("unexpected IOException occurred", e);
+				}
+				project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
+			}, null);
+
+			// wait for BACKGROUND_REFRESH to complete.
+			waitForRefresh();
+			final int resourceCount[] = new int[] { 0 };
+			project.accept(resource -> {
+				resourceCount[0]++;
+				return true;
+			});
+			// We have 1 root + 1 folder + 1 file (.project)
+			// + .settings / resources prefs
+			// --> 5 elements to visit
+			assertEquals(5, resourceCount[0]);
+		} finally {
+			UnifiedTree.enableAdvancedRecursiveLinkChecks(originalValue);
+		}
 	}
 
 }