Skip to content

Commit 043d728

Browse files
iloveeclipseiloveeclipse
committed
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
1 parent f394888 commit 043d728

File tree

5 files changed

+259
-100
lines changed

5 files changed

+259
-100
lines changed

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

Lines changed: 47 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -438,9 +438,6 @@ private static class PatternHolder {
438438
//Pattern: A UNIX or Windows relative path that just points backward
439439
static final Pattern TRIVIAL_SYMLINK_PATTERN = Pattern.compile( //
440440
Platform.OS.isWindows() ? "\\.[.\\\\]*" : "\\.[./]*"); //$NON-NLS-1$//$NON-NLS-2$
441-
442-
static final Pattern REPEATING_BACKWARDS_PATTERN = Pattern.compile( //
443-
Platform.OS.isWindows() ? "(\\.\\.\\\\)+.*" : "(\\.\\./)+.*"); //$NON-NLS-1$//$NON-NLS-2$
444441
}
445442

446443
/**
@@ -522,20 +519,10 @@ private boolean isRecursiveLink(IFileStore parentStore, IFileInfo localInfo) {
522519
Path realParentPath = parent.toRealPath();
523520
if (disable_advanced_recursive_link_checks) {
524521
// Multiple ../ backwards links can go outside the project tree
525-
if (linkTarget != null && PatternHolder.REPEATING_BACKWARDS_PATTERN.matcher(linkTarget).matches()) {
526-
Path targetPath = parent.resolve(linkTarget).normalize();
527-
528-
// Recursive if literal target points to the literal parent of this tree
529-
if (parent.normalize().startsWith(targetPath)) {
522+
if (linkTarget != null) {
523+
if (isRecursiveBackwardsLink(realParentPath, linkTarget)) {
530524
return true;
531525
}
532-
533-
// Recursive if resolved target points to the resolved parent of this tree
534-
Path realTargetPath = targetPath.toRealPath();
535-
if (realParentPath.startsWith(realTargetPath)) {
536-
return true;
537-
}
538-
539526
// If link is outside the project tree, consider as non recursive
540527
// The link still can create recursion in the tree, but we can't detect it here.
541528
}
@@ -573,6 +560,51 @@ private boolean isRecursiveLink(IFileStore parentStore, IFileInfo localInfo) {
573560
return false;
574561
}
575562

563+
/**
564+
* @param realParentPath real parent path object obtained as a result
565+
* of @code{Path.toRealPath()}
566+
* @param linkTarget the link target path as a string, may be relative or
567+
* absolute
568+
* @return true if the given target points backwards recursively to the given
569+
* parent path
570+
* @throws IOException
571+
*/
572+
private static boolean isRecursiveBackwardsLink(Path realParentPath, String linkTarget)
573+
throws IOException {
574+
// Cheap test first: literal target points to the literal parent
575+
Path normalizedLink = realParentPath.resolve(linkTarget).normalize();
576+
if (realParentPath.startsWith(normalizedLink)) {
577+
return true;
578+
}
579+
// Next check costs more time because it does real IO when resolving paths
580+
Path realTarget = normalizedLink.toRealPath();
581+
if (realParentPath.startsWith(realTarget)) {
582+
return true;
583+
}
584+
return false;
585+
}
586+
587+
/**
588+
* Disable or enable advanced recursive link checks. Advanced link checks may
589+
* hide valid directories in some cases, see bug 537449.
590+
*
591+
* @param enable <code>true</code> to enable advanced recursive link checks,
592+
* <code>false</code> to disable them.
593+
*/
594+
public static void enableAdvancedRecursiveLinkChecks(boolean enable) {
595+
disable_advanced_recursive_link_checks = !enable;
596+
}
597+
598+
/**
599+
* Returns whether advanced recursive link checks are enabled.
600+
*
601+
* @return <code>true</code> if advanced recursive link checks are enabled,
602+
* <code>false</code> otherwise.
603+
*/
604+
public static boolean isAdvancedRecursiveLinkChecksEnabled() {
605+
return !disable_advanced_recursive_link_checks;
606+
}
607+
576608
protected boolean isValidLevel(int currentLevel, int depth) {
577609
return switch (depth) {
578610
case IResource.DEPTH_INFINITE -> true;

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

Lines changed: 126 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,27 @@
2626
import static org.junit.Assume.assumeTrue;
2727

2828
import java.io.IOException;
29+
import java.nio.file.Files;
30+
import java.nio.file.Path;
31+
import java.nio.file.Paths;
2932
import org.eclipse.core.filesystem.EFS;
3033
import org.eclipse.core.filesystem.IFileStore;
34+
import org.eclipse.core.internal.localstore.UnifiedTree;
3135
import org.eclipse.core.resources.IProject;
3236
import org.eclipse.core.resources.IResource;
3337
import org.eclipse.core.resources.IResourceVisitor;
3438
import org.eclipse.core.resources.IWorkspaceRunnable;
3539
import org.eclipse.core.runtime.CoreException;
3640
import org.eclipse.core.runtime.IPath;
37-
import org.eclipse.core.tests.resources.WorkspaceTestRule;
38-
import org.junit.Rule;
39-
import org.junit.Test;
41+
import org.eclipse.core.tests.resources.util.WorkspaceResetExtension;
42+
import org.junit.jupiter.api.Test;
43+
import org.junit.jupiter.api.extension.ExtendWith;
44+
import org.junit.jupiter.params.ParameterizedTest;
45+
import org.junit.jupiter.params.provider.ValueSource;
4046

47+
@ExtendWith(WorkspaceResetExtension.class)
4148
public class SymlinkResourceTest {
4249

43-
@Rule
44-
public WorkspaceTestRule workspaceRule = new WorkspaceTestRule();
45-
4650
private void mkLink(IFileStore dir, String src, String tgt, boolean isDir) throws CoreException, IOException {
4751
createSymLink(dir.toLocalFile(EFS.NONE, createTestMonitor()), src, tgt, isDir);
4852
}
@@ -70,6 +74,87 @@ protected void createBug358830Structure(IFileStore rootDir) throws CoreException
7074
mkLink(folderA, "link", IPath.fromOSString("../").toOSString(), true);
7175
}
7276

77+
/**
78+
* Test a case of both recursive and non recursive symbolic links that uncovered
79+
* issue described in <a href=
80+
* "https://github.com/eclipse-platform/eclipse.platform/issues/2220">GitHub bug
81+
* 2220</a>.
82+
*
83+
* <pre>{@code
84+
* /A/B -> /X/Y/Z (B is symbolic link as precondition)
85+
* /A/B/C/D -> ../../D (non-recursive)
86+
* /A/B/C/E -> ../../Z (recursive)
87+
* }</pre>
88+
*
89+
* The starting path /A/B/C is already based on link. The real path of start is
90+
* /X/Y/Z/C so ../../Z points (recursive) to /X/Y/Z. /X/Y/D and /X/Y/Z is what
91+
* we expect to resolve but we fail in both cases with NoSuchFileException.
92+
*/
93+
@ParameterizedTest
94+
@ValueSource(booleans = { false, true })
95+
public void testGithubBug2220(boolean useAdvancedLinkCheck) throws Exception {
96+
assumeTrue("only relevant for platforms supporting symbolic links", canCreateSymLinks());
97+
final boolean originalValue = UnifiedTree.isAdvancedRecursiveLinkChecksEnabled();
98+
try {
99+
UnifiedTree.enableAdvancedRecursiveLinkChecks(useAdvancedLinkCheck);
100+
IProject project = getWorkspace().getRoot().getProject("testGithubBug2220");
101+
createInWorkspace(project);
102+
103+
/* Re-use projects which are cleaned up automatically */
104+
getWorkspace().run((IWorkspaceRunnable) monitor -> {
105+
/* delete open project because we must re-open with BACKGROUND_REFRESH */
106+
project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
107+
project.create(null);
108+
try {
109+
createGithubBug2220Structure(EFS.getStore(project.getLocationURI()));
110+
} catch (IOException e) {
111+
throw new IllegalStateException("unexpected IOException occurred", e);
112+
}
113+
// Bug only happens with BACKGROUND_REFRESH.
114+
project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
115+
}, null);
116+
117+
// wait for BACKGROUND_REFRESH to complete.
118+
waitForRefresh();
119+
project.accept(new IResourceVisitor() {
120+
int resourceCount = 0;
121+
122+
@Override
123+
public boolean visit(IResource resource) {
124+
resourceCount++;
125+
// We have 1 root + .settings + prefs + + .project + 10 elements --> 14 elements
126+
// to visit at most
127+
System.out.println(resourceCount + " visited: " + resource.getFullPath());
128+
assertTrue("Expected max 15 elements to visit, got: " + resourceCount, resourceCount <= 15);
129+
return true;
130+
}
131+
});
132+
} finally {
133+
UnifiedTree.enableAdvancedRecursiveLinkChecks(originalValue);
134+
}
135+
}
136+
137+
/**
138+
* <pre>{@code
139+
* /A/B -> /X/Y/Z (B is symbolic link as precondition)
140+
* /A/B/C/D -> ../../D (non-recursive)
141+
* /A/B/C/E -> ../../Z (recursive)
142+
* }</pre>
143+
*
144+
* The starting path /A/B/C is already based on link. The real path of start is
145+
* /X/Y/Z/C so ../../Z points (recursive) to /X/Y/Z.
146+
*/
147+
protected void createGithubBug2220Structure(IFileStore rootDir) throws CoreException, IOException {
148+
Path root = rootDir.toLocalFile(EFS.NONE, createTestMonitor()).toPath();
149+
Files.createDirectories(root.resolve("A"));
150+
Files.createDirectories(root.resolve("X/Y/Z"));
151+
Files.createDirectories(root.resolve("X/Y/D"));
152+
Files.createSymbolicLink(root.resolve("A/B"), root.resolve("X/Y/Z"));
153+
Files.createDirectories(root.resolve("A/B/C"));
154+
Files.createSymbolicLink(root.resolve("A/B/C/D"), Paths.get("../../D"));
155+
Files.createSymbolicLink(root.resolve("A/B/C/E"), Paths.get("../../Z"));
156+
}
157+
73158
/**
74159
* Test a very specific case of mutually recursive symbolic links:
75160
* <pre> {@code
@@ -118,36 +203,42 @@ public boolean visit(IResource resource) {
118203
});
119204
}
120205

121-
@Test
122-
public void testBug358830() throws Exception {
206+
@ParameterizedTest
207+
@ValueSource(booleans = { false, true })
208+
public void testBug358830(boolean useAdvancedLinkCheck) throws Exception {
123209
assumeTrue("only relevant for platforms supporting symbolic links", canCreateSymLinks());
124-
125-
IProject project = getWorkspace().getRoot().getProject("Project");
126-
createInWorkspace(project);
127-
/* Re-use projects which are cleaned up automatically */
128-
getWorkspace().run((IWorkspaceRunnable) monitor -> {
129-
/* delete open project because we must re-open with BACKGROUND_REFRESH */
130-
project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
131-
project.create(null);
132-
try {
133-
createBug358830Structure(EFS.getStore(project.getLocationURI()));
134-
} catch (IOException e) {
135-
throw new IllegalStateException("unexpected IOException occurred", e);
136-
}
137-
project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
138-
}, null);
139-
140-
//wait for BACKGROUND_REFRESH to complete.
141-
waitForRefresh();
142-
final int resourceCount[] = new int[] {0};
143-
project.accept(resource -> {
144-
resourceCount[0]++;
145-
return true;
146-
});
147-
// We have 1 root + 1 folder + 1 file (.project)
148-
// + .settings / resources prefs
149-
// --> 5 elements to visit
150-
assertEquals(5, resourceCount[0]);
210+
final boolean originalValue = UnifiedTree.isAdvancedRecursiveLinkChecksEnabled();
211+
try {
212+
UnifiedTree.enableAdvancedRecursiveLinkChecks(useAdvancedLinkCheck);
213+
IProject project = getWorkspace().getRoot().getProject("Project");
214+
createInWorkspace(project);
215+
/* Re-use projects which are cleaned up automatically */
216+
getWorkspace().run((IWorkspaceRunnable) monitor -> {
217+
/* delete open project because we must re-open with BACKGROUND_REFRESH */
218+
project.delete(IResource.NEVER_DELETE_PROJECT_CONTENT, createTestMonitor());
219+
project.create(null);
220+
try {
221+
createBug358830Structure(EFS.getStore(project.getLocationURI()));
222+
} catch (IOException e) {
223+
throw new IllegalStateException("unexpected IOException occurred", e);
224+
}
225+
project.open(IResource.BACKGROUND_REFRESH, createTestMonitor());
226+
}, null);
227+
228+
// wait for BACKGROUND_REFRESH to complete.
229+
waitForRefresh();
230+
final int resourceCount[] = new int[] { 0 };
231+
project.accept(resource -> {
232+
resourceCount[0]++;
233+
return true;
234+
});
235+
// We have 1 root + 1 folder + 1 file (.project)
236+
// + .settings / resources prefs
237+
// --> 5 elements to visit
238+
assertEquals(5, resourceCount[0]);
239+
} finally {
240+
UnifiedTree.enableAdvancedRecursiveLinkChecks(originalValue);
241+
}
151242
}
152243

153244
}

‎resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/regression/Bug_185247_LinuxTests.java‎

Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import java.util.ArrayList;
2727
import java.util.Arrays;
2828
import java.util.List;
29+
import org.eclipse.core.internal.localstore.UnifiedTree;
2930
import org.eclipse.core.internal.resources.ProjectDescription;
3031
import org.eclipse.core.resources.IProject;
3132
import org.eclipse.core.resources.IProjectDescription;
@@ -38,10 +39,11 @@
3839
import org.eclipse.core.runtime.URIUtil;
3940
import org.eclipse.core.tests.resources.util.WorkspaceResetExtension;
4041
import org.junit.jupiter.api.BeforeEach;
41-
import org.junit.jupiter.api.Test;
4242
import org.junit.jupiter.api.TestInfo;
4343
import org.junit.jupiter.api.extension.ExtendWith;
4444
import org.junit.jupiter.api.io.TempDir;
45+
import org.junit.jupiter.params.ParameterizedTest;
46+
import org.junit.jupiter.params.provider.ValueSource;
4547

4648
/**
4749
* Test cases for symbolic links in projects.
@@ -74,34 +76,45 @@ private void extractTestCasesArchive(IPath outputLocation) throws Exception {
7476
unzip(archive, outputLocation.toFile());
7577
}
7678

77-
@Test
78-
public void test1_trivial() throws Exception {
79-
runProjectTestCase();
79+
@ParameterizedTest
80+
@ValueSource(booleans = { false, true })
81+
public void test1_trivial(boolean useAdvancedLinkCheck) throws Exception {
82+
runProjectTestCase(useAdvancedLinkCheck);
8083
}
8184

82-
@Test
83-
public void test2_mutual() throws Exception {
84-
runProjectTestCase();
85+
@ParameterizedTest
86+
@ValueSource(booleans = { false, true })
87+
public void test2_mutual(boolean useAdvancedLinkCheck) throws Exception {
88+
runProjectTestCase(useAdvancedLinkCheck);
8589
}
8690

87-
@Test
88-
public void test3_outside_tree() throws Exception {
89-
runProjectTestCase();
91+
@ParameterizedTest
92+
@ValueSource(booleans = { false, true })
93+
public void test3_outside_tree(boolean useAdvancedLinkCheck) throws Exception {
94+
runProjectTestCase(useAdvancedLinkCheck);
9095
}
9196

92-
@Test
93-
public void test5_transitive_mutual() throws Exception {
94-
runProjectTestCase();
97+
@ParameterizedTest
98+
@ValueSource(booleans = { false, true })
99+
public void test5_transitive_mutual(boolean useAdvancedLinkCheck) throws Exception {
100+
runProjectTestCase(useAdvancedLinkCheck);
95101
}
96102

97-
@Test
98-
public void test6_nonrecursive() throws Exception {
99-
runProjectTestCase();
103+
@ParameterizedTest
104+
@ValueSource(booleans = { false, true })
105+
public void test6_nonrecursive(boolean useAdvancedLinkCheck) throws Exception {
106+
runProjectTestCase(useAdvancedLinkCheck);
100107
}
101108

102-
private void runProjectTestCase() throws Exception {
103-
// refresh should hang, if bug 105554 re-occurs
104-
importProjectAndRefresh(testMethodName);
109+
private void runProjectTestCase(boolean useAdvancedLinkCheck) throws Exception {
110+
final boolean originalValue = UnifiedTree.isAdvancedRecursiveLinkChecksEnabled();
111+
try {
112+
UnifiedTree.enableAdvancedRecursiveLinkChecks(useAdvancedLinkCheck);
113+
// refresh should hang, if bug 105554 re-occurs
114+
importProjectAndRefresh(testMethodName);
115+
} finally {
116+
UnifiedTree.enableAdvancedRecursiveLinkChecks(originalValue);
117+
}
105118
}
106119

107120
private void importProjectAndRefresh(String projectName) throws Exception {

0 commit comments

Comments
 (0)