From 4d57fcb60d6b68417f3ba965679bcac8e591a7e4 Mon Sep 17 00:00:00 2001 From: Alexey Kudravtsev Date: Wed, 12 Oct 2016 13:24:44 +0300 Subject: [PATCH] avoid inconsistencies after crazy renames/moves files onto themselves --- .../openapi/vfs/impl/FilePointerPartNode.java | 24 +++++++++---- .../impl/VirtualFilePointerManagerImpl.java | 36 +++++++++++-------- .../vfs/impl/VirtualFilePointerTest.java | 20 +++++++++-- 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java index 30414040dd45..a44a0bf8f380 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/FilePointerPartNode.java @@ -35,7 +35,7 @@ import java.util.List; // all file pointers we store in the tree with nodes corresponding to the file structure on disk class FilePointerPartNode { private static final FilePointerPartNode[] EMPTY_ARRAY = new FilePointerPartNode[0]; - @NotNull private String part; // common prefix of all file pointers beneath + @NotNull String part; // common prefix of all file pointers beneath @NotNull FilePointerPartNode[] children; FilePointerPartNode parent; // file pointers for this exact path (e.g. concatenation of all "part" fields down from the root). @@ -69,8 +69,6 @@ class FilePointerPartNode { boolean separator, @NotNull CharSequence childName, @NotNull FilePointerPartNode[] outNode) { - checkConsistency(); - int partStart; if (parent == null) { partStart = 0; @@ -137,20 +135,30 @@ class FilePointerPartNode { private static final boolean UNIT_TEST = ApplicationManager.getApplication().isUnitTestMode(); void checkConsistency() { if (UNIT_TEST && !ApplicationInfoImpl.isInPerformanceTest()) { - doCheckConsistency(); + doCheckConsistency(false); } } - private void doCheckConsistency() { + private void doCheckConsistency(boolean dotDotOccurred) { + dotDotOccurred |= part.contains(".."); // part must not contain ".." (except when the file pointer was created from URL with ".." inside) int childSum = 0; for (FilePointerPartNode child : children) { childSum += child.pointersUnder; - child.doCheckConsistency(); + child.doCheckConsistency(dotDotOccurred); assert child.parent == this; } childSum += leavesNumber(); assert (useCount == 0) == (leaves == null) : useCount + " - " + (leaves instanceof VirtualFilePointerImpl ? leaves : Arrays.toString((VirtualFilePointerImpl[])leaves)); assert pointersUnder == childSum : "expected: "+pointersUnder+"; actual: "+childSum; + Pair fileAndUrl = myFileAndUrl; + if (fileAndUrl != null && fileAndUrl.second != null) { + String url = fileAndUrl.second; + assert url.endsWith(part) : "part is: '"+part +"' but url is: '"+url+"'"; + } + boolean hasFile = fileAndUrl != null && fileAndUrl.first != null; + + // when the node contains real file its path should be canonical + assert !hasFile || !dotDotOccurred : "Path is not canonical: '"+getUrl()+"'; my part: '"+part+"'"; } @NotNull @@ -331,6 +339,10 @@ class FilePointerPartNode { return leaves == null ? null : leaves instanceof VirtualFilePointerImpl ? (VirtualFilePointerImpl)leaves : ((VirtualFilePointerImpl[])leaves)[0]; } + private String getUrl() { + return parent == null ? part : parent.getUrl() + part; + } + private int leavesNumber() { Object leaves = this.leaves; return leaves == null ? 0 : leaves instanceof VirtualFilePointerImpl ? 1 : ((VirtualFilePointerImpl[])leaves).length; diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java index ab9982938fa0..87f784a97498 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/VirtualFilePointerManagerImpl.java @@ -199,6 +199,14 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp url = VirtualFileManager.constructUrl(protocol, cleanPath); path = cleanPath; } + if (url.contains("..")) { + // the url of the form "/x/../y" should resolve to "/y" (or something else in the case of symlinks) + file = VirtualFileManager.getInstance().findFileByUrl(url); + if (file != null) { + url = file.getUrl(); + path = file.getPath(); + } + } } // else url has come from VirtualFile.getPath() and is good enough @@ -210,12 +218,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp private final Map myUrlToIdentity = new THashMap<>(); @NotNull private IdentityVirtualFilePointer getOrCreateIdentity(@NotNull String url, @Nullable VirtualFile found) { - IdentityVirtualFilePointer pointer = myUrlToIdentity.get(url); - if (pointer == null) { - pointer = new IdentityVirtualFilePointer(found, url); - myUrlToIdentity.put(url, pointer); - } - return pointer; + return myUrlToIdentity.computeIfAbsent(url, __ -> new IdentityVirtualFilePointer(found, url)); } @NotNull @@ -463,6 +466,14 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp myNodesToFire = toFireEvents; myNodesToUpdateUrl = toUpdateUrl; + + assertConsistency(); + } + + void assertConsistency() { + for (FilePointerPartNode root : myPointers.values()) { + root.checkConsistency(); + } } @Override @@ -474,7 +485,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp String urlBefore = node.myFileAndUrl.second; Pair after = node.update(); String urlAfter = after.second; - if (URL_COMPARATOR.compare(urlBefore, urlAfter) != 0) { + if (URL_COMPARATOR.compare(urlBefore, urlAfter) != 0 || !urlAfter.endsWith(node.part)) { List myPointers = new SmartList<>(); node.addAllPointersTo(myPointers); @@ -514,9 +525,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp myNodesToUpdateUrl = Collections.emptyList(); myEvents = Collections.emptyList(); myNodesToFire = Collections.emptyList(); - for (FilePointerPartNode root : myPointers.values()) { - root.checkConsistency(); - } + assertConsistency(); } void removeNode(@NotNull FilePointerPartNode node, VirtualFilePointerListener listener) { @@ -525,9 +534,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp if (rootNodeEmpty) { myPointers.remove(listener); } - else { - myPointers.get(listener).checkConsistency(); - } + assertConsistency(); } @Override @@ -538,8 +545,7 @@ public class VirtualFilePointerManagerImpl extends VirtualFilePointerManager imp } private static class DelegatingDisposable implements Disposable { - private static final ConcurrentMap ourInstances = - ContainerUtil.newConcurrentMap(ContainerUtil.identityStrategy()); + private static final ConcurrentMap ourInstances = ContainerUtil.newConcurrentMap(ContainerUtil.identityStrategy()); private final TObjectIntHashMap myCounts = new TObjectIntHashMap<>(); private final Disposable myParent; diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java b/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java index d8b7072ecf93..d273aefaec08 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java +++ b/platform/platform-tests/testSrc/com/intellij/openapi/vfs/impl/VirtualFilePointerTest.java @@ -45,6 +45,7 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.ui.UIUtil; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.io.File; import java.io.IOException; @@ -521,7 +522,7 @@ public class VirtualFilePointerTest extends PlatformTestCase { } } - private VirtualFilePointer createPointerByFile(final File file, final VirtualFilePointerListener fileListener) throws IOException { + private VirtualFilePointer createPointerByFile(@NotNull File file, @Nullable VirtualFilePointerListener fileListener) throws IOException { final String url = VirtualFileManager.constructUrl(LocalFileSystem.PROTOCOL, file.getCanonicalPath().replace(File.separatorChar, '/')); final VirtualFile vFile = refreshAndFind(url); return vFile == null @@ -721,7 +722,7 @@ public class VirtualFilePointerTest extends PlatformTestCase { }).useLegacyScaling().assertTiming(); } - public void testCidrConfusions() throws IOException { + public void testCidrCrazyAddCreateRenames() throws IOException { File tempDirectory = createTempDirectory(); final VirtualFile root = LocalFileSystem.getInstance().refreshAndFindFileByIoFile(tempDirectory); @@ -737,8 +738,9 @@ public class VirtualFilePointerTest extends PlatformTestCase { delete(dir1); assertSourceIs(null); // srcDir deleted, no more sources assertLibIs(dir2); // libDir stays the same - + myVirtualFilePointerManager.assertConsistency(); rename(dir2, "dir1"); + myVirtualFilePointerManager.assertConsistency(); assertSourceIs(dir2); // srcDir re-appeared, sources are "dir1" assertLibIs(dir2); // libDir renamed, libs are "dir1" now @@ -837,4 +839,16 @@ public class VirtualFilePointerTest extends PlatformTestCase { PsiTestUtil.removeAllRoots(getModule(), null); } } + + public void testDotDot() throws IOException { + File tempDirectory = createTempDirectory(); + final VirtualFile root = LocalFileSystem.getInstance().refreshAndFindFileByIoFile(tempDirectory); + + VirtualFile dir1 = createChildDirectory(root, "dir1"); + VirtualFile dir2 = createChildDirectory(root, "dir2"); + VirtualFile file = createChildData(dir1, "x.txt"); + + VirtualFilePointer pointer = myVirtualFilePointerManager.create(dir2.getUrl() + "/../" + dir1.getName() + "/" + file.getName(), disposable, null); + assertEquals(file, pointer.getFile()); + } }