From 7598bc6bdf74b21cc75d7475cf5ad04a6c3bdcea Mon Sep 17 00:00:00 2001 From: lberki Date: Fri, 2 Oct 2020 01:33:14 -0700 Subject: [PATCH] Make Bazel not raise an error when a symlink points to its own ancestor. This is only an error when the symlink is to be traversed recursively. E.g. `a/a/a/b` is perfectly acceptable if `a/a` is a symlink to `.`, except if we want to recursively traverse into it during a glob or recursive target pattern expansion. RELNOTES: Blaze now allows symbolic links that point to their own ancestor unless they are traversed recursively by e.g. a //... recursive target pattern or a recursive glob. PiperOrigin-RevId: 334982640 --- .../devtools/build/lib/actions/FileValue.java | 246 +++++++++++++++++- .../google/devtools/build/lib/skyframe/BUILD | 7 + .../build/lib/skyframe/FileFunction.java | 118 ++++----- .../build/lib/skyframe/GlobFunction.java | 26 ++ .../lib/skyframe/ProcessPackageDirectory.java | 17 ++ .../RecursiveFilesystemTraversalFunction.java | 13 + .../build/lib/skyframe/FileFunctionTest.java | 101 +++---- .../build/lib/skyframe/GlobFunctionTest.java | 2 + .../packages/AbstractPackageLoaderTest.java | 2 +- src/test/shell/bazel/bazelignore_test.sh | 12 +- 10 files changed, 407 insertions(+), 137 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java index 7c2f4166bf93f5..55c5ab43bde674 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileValue.java @@ -101,6 +101,18 @@ public boolean isDirectory() { */ public abstract ImmutableList logicalChainDuringResolution(); + /** + * If a symlink pointing back to its own ancestor was encountered during the resolution of this + * {@link FileValue}, returns the path to it. Otherwise, returns null. + */ + public abstract ImmutableList pathToUnboundedAncestorSymlinkExpansionChain(); + + /** + * If a symlink pointing back to its own ancestor was encountered during the resolution of this + * {@link FileValue}, returns the symlinks in the cycle. Otherwise, returns null. + */ + public abstract ImmutableList unboundedAncestorSymlinkExpansionChain(); + /** * Returns the real rooted path of the file, taking ancestor symlinks into account. For example, * the rooted path ['root']/['a/b'] is really ['root']/['c/b'] if 'a' is a symlink to 'c'. Note @@ -165,6 +177,8 @@ public SkyFunctionName functionName() { */ public static FileValue value( ImmutableList logicalChainDuringResolution, + ImmutableList pathToUnboundedAncestorSymlinkExpansionChain, + ImmutableList unboundedAncestorSymlinkExpansionChain, RootedPath originalRootedPath, FileStateValue fileStateValueFromAncestors, RootedPath realRootedPath, @@ -205,17 +219,36 @@ public static FileValue value( if (fileStateValueFromAncestors.getType() == FileStateType.SYMLINK) { PathFragment symlinkTarget = fileStateValueFromAncestors.getSymlinkTarget(); - return shouldStoreChain - ? new SymlinkFileValueWithStoredChain( - realRootedPath, realFileStateValue, logicalChainDuringResolution, symlinkTarget) - : new SymlinkFileValueWithoutStoredChain( - realRootedPath, realFileStateValue, symlinkTarget); + if (pathToUnboundedAncestorSymlinkExpansionChain != null) { + return new SymlinkFileValueWithSymlinkCycle( + realRootedPath, + realFileStateValue, + logicalChainDuringResolution, + symlinkTarget, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } else if (shouldStoreChain) { + return new SymlinkFileValueWithStoredChain( + realRootedPath, realFileStateValue, logicalChainDuringResolution, symlinkTarget); + } else { + return new SymlinkFileValueWithoutStoredChain( + realRootedPath, realFileStateValue, symlinkTarget); + } + } else { + if (pathToUnboundedAncestorSymlinkExpansionChain != null) { + return new DifferentRealPathFileValueWithSymlinkCycle( + realRootedPath, + realFileStateValue, + logicalChainDuringResolution, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } else if (shouldStoreChain) { + return new DifferentRealPathFileValueWithStoredChain( + realRootedPath, realFileStateValue, logicalChainDuringResolution); + } else { + return new DifferentRealPathFileValueWithoutStoredChain(realRootedPath, realFileStateValue); + } } - - return shouldStoreChain - ? new DifferentRealPathFileValueWithStoredChain( - realRootedPath, realFileStateValue, logicalChainDuringResolution) - : new DifferentRealPathFileValueWithoutStoredChain(realRootedPath, realFileStateValue); } /** @@ -240,6 +273,16 @@ public ImmutableList logicalChainDuringResolution() { return ImmutableList.of(rootedPath); } + @Override + public ImmutableList pathToUnboundedAncestorSymlinkExpansionChain() { + return null; + } + + @Override + public ImmutableList unboundedAncestorSymlinkExpansionChain() { + return null; + } + @Override public RootedPath realRootedPath() { return rootedPath; @@ -273,6 +316,84 @@ public String toString() { } } + /** + * A {@link FileValue} whose resolution required traversing a symlink chain caused by a symlink + * pointing to its own ancestor but which eventually points to a real file. + */ + @AutoCodec.VisibleForSerialization + @AutoCodec + public static class DifferentRealPathFileValueWithSymlinkCycle + extends DifferentRealPathFileValueWithStoredChain { + // We can't store an exception here because this needs to be serialized, AutoCodec chokes on + // object cycles and FilesystemInfiniteSymlinkCycleException somehow sets its cause to itself + protected final ImmutableList pathToUnboundedAncestorSymlinkExpansionChain; + protected final ImmutableList unboundedAncestorSymlinkExpansionChain; + + public DifferentRealPathFileValueWithSymlinkCycle( + RootedPath realRootedPath, + FileStateValue realFileStateValue, + ImmutableList logicalChainDuringResolution, + ImmutableList pathToUnboundedAncestorSymlinkExpansionChain, + ImmutableList unboundedAncestorSymlinkExpansionChain) { + super(realRootedPath, realFileStateValue, logicalChainDuringResolution); + this.pathToUnboundedAncestorSymlinkExpansionChain = + pathToUnboundedAncestorSymlinkExpansionChain; + this.unboundedAncestorSymlinkExpansionChain = unboundedAncestorSymlinkExpansionChain; + } + + @Override + public ImmutableList pathToUnboundedAncestorSymlinkExpansionChain() { + return pathToUnboundedAncestorSymlinkExpansionChain; + } + + @Override + public ImmutableList unboundedAncestorSymlinkExpansionChain() { + return unboundedAncestorSymlinkExpansionChain; + } + + @Override + public int hashCode() { + return Objects.hash( + realRootedPath, + realFileStateValue, + logicalChainDuringResolution, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } + + @Override + public boolean equals(Object obj) { + if (obj == null) { + return false; + } + + if (obj.getClass() != DifferentRealPathFileValueWithSymlinkCycle.class) { + return false; + } + + DifferentRealPathFileValueWithSymlinkCycle other = + (DifferentRealPathFileValueWithSymlinkCycle) obj; + return realRootedPath.equals(other.realRootedPath) + && realFileStateValue.equals(other.realFileStateValue) + && logicalChainDuringResolution.equals(other.logicalChainDuringResolution) + && pathToUnboundedAncestorSymlinkExpansionChain.equals( + other.pathToUnboundedAncestorSymlinkExpansionChain) + && unboundedAncestorSymlinkExpansionChain.equals( + other.unboundedAncestorSymlinkExpansionChain); + } + + @Override + public String toString() { + return String.format( + "symlink ancestor (real_path=%s, real_state=%s, chain=%s, path=%s, cycle=%s)", + realRootedPath, + realFileStateValue, + logicalChainDuringResolution, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } + } + /** * Implementation of {@link FileValue} for paths whose fully resolved path is different than the * requested path, but the path itself is not a symlink. For example, this is the case for the @@ -309,6 +430,16 @@ public ImmutableList logicalChainDuringResolution() { return logicalChainDuringResolution; } + @Override + public ImmutableList pathToUnboundedAncestorSymlinkExpansionChain() { + return null; + } + + @Override + public ImmutableList unboundedAncestorSymlinkExpansionChain() { + return null; + } + @Override public boolean equals(Object obj) { if (obj == null) { @@ -369,6 +500,16 @@ public ImmutableList logicalChainDuringResolution() { throw new IllegalStateException(this.toString()); } + @Override + public ImmutableList pathToUnboundedAncestorSymlinkExpansionChain() { + return null; + } + + @Override + public ImmutableList unboundedAncestorSymlinkExpansionChain() { + return null; + } + @Override public boolean equals(Object obj) { if (obj == null) { @@ -396,12 +537,93 @@ public String toString() { } } + /** + * A {@link FileValue} whose resolution required traversing a symlink chain caused by a symlink + * pointing to its own ancestor and which eventually points to a symlink. + */ + @AutoCodec.VisibleForSerialization + @AutoCodec + public static final class SymlinkFileValueWithSymlinkCycle + extends SymlinkFileValueWithStoredChain { + // We can't store an exception here because this needs to be serialized, AutoCodec chokes on + // object cycles and FilesystemInfiniteSymlinkCycleException somehow sets its cause to itself + private final ImmutableList pathToUnboundedAncestorSymlinkExpansionChain; + private final ImmutableList unboundedAncestorSymlinkExpansionChain; + + public SymlinkFileValueWithSymlinkCycle( + RootedPath realRootedPath, + FileStateValue realFileStateValue, + ImmutableList logicalChainDuringResolution, + PathFragment linkTarget, + ImmutableList pathToUnboundedAncestorSymlinkExpansionChain, + ImmutableList unboundedAncestorSymlinkExpansionChain) { + super(realRootedPath, realFileStateValue, logicalChainDuringResolution, linkTarget); + this.pathToUnboundedAncestorSymlinkExpansionChain = + pathToUnboundedAncestorSymlinkExpansionChain; + this.unboundedAncestorSymlinkExpansionChain = unboundedAncestorSymlinkExpansionChain; + } + + @Override + public ImmutableList pathToUnboundedAncestorSymlinkExpansionChain() { + return pathToUnboundedAncestorSymlinkExpansionChain; + } + + @Override + public ImmutableList unboundedAncestorSymlinkExpansionChain() { + return unboundedAncestorSymlinkExpansionChain; + } + + @Override + public int hashCode() { + return Objects.hash( + realRootedPath, + realFileStateValue, + logicalChainDuringResolution, + linkTarget, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } + + @Override + public boolean equals(Object obj) { + if (obj == null) { + return false; + } + + if (obj.getClass() != SymlinkFileValueWithSymlinkCycle.class) { + return false; + } + + SymlinkFileValueWithSymlinkCycle other = (SymlinkFileValueWithSymlinkCycle) obj; + return realRootedPath.equals(other.realRootedPath) + && realFileStateValue.equals(other.realFileStateValue) + && logicalChainDuringResolution.equals(other.logicalChainDuringResolution) + && linkTarget.equals(other.linkTarget) + && pathToUnboundedAncestorSymlinkExpansionChain.equals( + other.pathToUnboundedAncestorSymlinkExpansionChain) + && unboundedAncestorSymlinkExpansionChain.equals( + other.unboundedAncestorSymlinkExpansionChain); + } + + @Override + public String toString() { + return String.format( + "symlink ancestor (real_path=%s, real_state=%s, target=%s, chain=%s, path=%s, cycle=%s)", + realRootedPath, + realFileStateValue, + linkTarget, + logicalChainDuringResolution, + pathToUnboundedAncestorSymlinkExpansionChain, + unboundedAncestorSymlinkExpansionChain); + } + } + /** Implementation of {@link FileValue} for paths that are themselves symlinks. */ @AutoCodec.VisibleForSerialization @AutoCodec - public static final class SymlinkFileValueWithStoredChain + public static class SymlinkFileValueWithStoredChain extends DifferentRealPathFileValueWithStoredChain { - private final PathFragment linkTarget; + protected final PathFragment linkTarget; @VisibleForTesting public SymlinkFileValueWithStoredChain( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index fa674c41c3e348..8062c162451154 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1057,6 +1057,7 @@ java_library( name = "collect_packages_under_directory_function", srcs = ["CollectPackagesUnderDirectoryFunction.java"], deps = [ + "process_package_directory", ":collect_packages_under_directory_value", ":recursive_directory_traversal_function", ":recursive_pkg_key", @@ -1524,6 +1525,8 @@ java_library( srcs = ["GlobFunction.java"], deps = [ ":directory_listing_value", + ":file_symlink_infinite_expansion_exception", + ":file_symlink_infinite_expansion_uniqueness_function", ":glob_descriptor", ":glob_value", ":ignored_package_prefixes_value", @@ -2020,6 +2023,8 @@ java_library( ":directory_listing_value", ":dirents", ":file_symlink_exception", + ":file_symlink_infinite_expansion_exception", + ":file_symlink_infinite_expansion_uniqueness_function", ":package_lookup_value", ":process_package_directory_result", "//src/main/java/com/google/devtools/build/lib/actions:file_metadata", @@ -2095,6 +2100,8 @@ java_library( deps = [ ":action_execution_value", ":directory_listing_value", + ":file_symlink_infinite_expansion_exception", + ":file_symlink_infinite_expansion_uniqueness_function", ":package_lookup_value", ":sky_functions", ":tree_artifact_value", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java index bae2c189d8f10b..7e0668a225b72b 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileFunction.java @@ -66,11 +66,7 @@ public FileFunction(AtomicReference pkgLocator) { this.nonexistentFileReceiver = nonexistentFileReceiver; } - @Override - public FileValue compute(SkyKey skyKey, Environment env) - throws FileFunctionException, InterruptedException { - RootedPath rootedPath = (RootedPath) skyKey.argument(); - + private static class SymlinkResolutionState { // Suppose we have a path p. One of the goals of FileFunction is to resolve the "real path", if // any, of p. The basic algorithm is to use the fully resolved path of p's parent directory to // determine the fully resolved path of p. This is complicated when symlinks are involved, and @@ -91,6 +87,18 @@ public FileValue compute(SkyKey skyKey, Environment env) // See the usage in checkPathSeenDuringPartialResolutionInternal. TreeSet sortedLogicalChain = Sets.newTreeSet(); + ImmutableList pathToUnboundedAncestorSymlinkExpansionChain = null; + ImmutableList unboundedAncestorSymlinkExpansionChain = null; + + private SymlinkResolutionState() {} + } + + @Override + public FileValue compute(SkyKey skyKey, Environment env) + throws FileFunctionException, InterruptedException { + RootedPath rootedPath = (RootedPath) skyKey.argument(); + SymlinkResolutionState symlinkResolutionState = new SymlinkResolutionState(); + // Fully resolve the path of the parent directory, but only if the current file is not the // filesystem root (has no parent) or a package path root (treated opaquely and handled by // skyframe's DiffAwareness interface). @@ -99,7 +107,7 @@ public FileValue compute(SkyKey skyKey, Environment env) // an ancestor is part of a symlink cycle, we want to detect that quickly as it gives a more // informative error message than we'd get doing bogus filesystem operations. PartialResolutionResult resolveFromAncestorsResult = - resolveFromAncestors(rootedPath, sortedLogicalChain, logicalChain, env); + resolveFromAncestors(rootedPath, symlinkResolutionState, env); if (resolveFromAncestorsResult == null) { return null; } @@ -107,7 +115,9 @@ public FileValue compute(SkyKey skyKey, Environment env) FileStateValue fileStateValueFromAncestors = resolveFromAncestorsResult.fileStateValue; if (fileStateValueFromAncestors.getType() == FileStateType.NONEXISTENT) { return FileValue.value( - ImmutableList.copyOf(logicalChain), + ImmutableList.copyOf(symlinkResolutionState.logicalChain), + symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain, + symlinkResolutionState.unboundedAncestorSymlinkExpansionChain, rootedPath, FileStateValue.NONEXISTENT_FILE_STATE_NODE, rootedPathFromAncestors, @@ -120,11 +130,7 @@ public FileValue compute(SkyKey skyKey, Environment env) while (realFileStateValue.getType().isSymlink()) { PartialResolutionResult getSymlinkTargetRootedPathResult = getSymlinkTargetRootedPath( - realRootedPath, - realFileStateValue.getSymlinkTarget(), - sortedLogicalChain, - logicalChain, - env); + realRootedPath, realFileStateValue.getSymlinkTarget(), symlinkResolutionState, env); if (getSymlinkTargetRootedPathResult == null) { return null; } @@ -133,7 +139,9 @@ public FileValue compute(SkyKey skyKey, Environment env) } return FileValue.value( - ImmutableList.copyOf(logicalChain), + ImmutableList.copyOf(symlinkResolutionState.logicalChain), + symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain, + symlinkResolutionState.unboundedAncestorSymlinkExpansionChain, rootedPath, // TODO(b/123922036): This is a bug. Should be 'fileStateValueFromAncestors'. fileStateValueFromAncestors, @@ -156,24 +164,19 @@ private RootedPath toRootedPath(Path path) { */ @Nullable private PartialResolutionResult resolveFromAncestors( - RootedPath rootedPath, - TreeSet sortedLogicalChain, - ArrayList logicalChain, - Environment env) + RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env) throws InterruptedException, FileFunctionException { RootedPath parentRootedPath = rootedPath.getParentDirectory(); return parentRootedPath != null - ? resolveFromAncestorsWithParent( - rootedPath, parentRootedPath, sortedLogicalChain, logicalChain, env) - : resolveFromAncestorsNoParent(rootedPath, sortedLogicalChain, logicalChain, env); + ? resolveFromAncestorsWithParent(rootedPath, parentRootedPath, symlinkResolutionState, env) + : resolveFromAncestorsNoParent(rootedPath, symlinkResolutionState, env); } @Nullable private PartialResolutionResult resolveFromAncestorsWithParent( RootedPath rootedPath, RootedPath parentRootedPath, - TreeSet sortedLogicalChain, - ArrayList logicalChain, + SymlinkResolutionState symlinkResolutionState, Environment env) throws InterruptedException, FileFunctionException { PathFragment relativePath = rootedPath.getRootRelativePath(); @@ -197,7 +200,7 @@ private PartialResolutionResult resolveFromAncestorsWithParent( for (RootedPath parentPartialRootedPath : parentFileValue.logicalChainDuringResolution()) { checkAndNotePathSeenDuringPartialResolution( - getChild(parentPartialRootedPath, baseName), sortedLogicalChain, logicalChain, env); + getChild(parentPartialRootedPath, baseName), symlinkResolutionState, env); if (env.valuesMissing()) { return null; } @@ -214,12 +217,9 @@ private PartialResolutionResult resolveFromAncestorsWithParent( @Nullable private PartialResolutionResult resolveFromAncestorsNoParent( - RootedPath rootedPath, - TreeSet sortedLogicalChain, - ArrayList logicalChain, - Environment env) + RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env) throws InterruptedException, FileFunctionException { - checkAndNotePathSeenDuringPartialResolution(rootedPath, sortedLogicalChain, logicalChain, env); + checkAndNotePathSeenDuringPartialResolution(rootedPath, symlinkResolutionState, env); if (env.valuesMissing()) { return null; } @@ -249,8 +249,7 @@ private PartialResolutionResult(RootedPath rootedPath, FileStateValue fileStateV private PartialResolutionResult getSymlinkTargetRootedPath( RootedPath rootedPath, PathFragment symlinkTarget, - TreeSet sortedLogicalChain, - ArrayList logicalChain, + SymlinkResolutionState symlinkResolutionState, Environment env) throws FileFunctionException, InterruptedException { Path path = rootedPath.asPath(); @@ -265,44 +264,35 @@ private PartialResolutionResult getSymlinkTargetRootedPath( : path.getRelative(symlinkTarget); } RootedPath symlinkTargetRootedPath = toRootedPath(symlinkTargetPath); - checkPathSeenDuringPartialResolution( - symlinkTargetRootedPath, sortedLogicalChain, logicalChain, env); + checkPathSeenDuringPartialResolution(symlinkTargetRootedPath, symlinkResolutionState, env); if (env.valuesMissing()) { return null; } // The symlink target could have a different parent directory, which itself could be a directory // symlink (or have an ancestor directory symlink)! - return resolveFromAncestors(symlinkTargetRootedPath, sortedLogicalChain, logicalChain, env); + return resolveFromAncestors(symlinkTargetRootedPath, symlinkResolutionState, env); } private void checkAndNotePathSeenDuringPartialResolution( - RootedPath rootedPath, - TreeSet sortedLogicalChain, - ArrayList logicalChain, - Environment env) + RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env) throws FileFunctionException, InterruptedException { Path path = rootedPath.asPath(); - checkPathSeenDuringPartialResolutionInternal( - rootedPath, path, sortedLogicalChain, logicalChain, env); - sortedLogicalChain.add(path); - logicalChain.add(rootedPath); + checkPathSeenDuringPartialResolutionInternal(rootedPath, path, symlinkResolutionState, env); + symlinkResolutionState.sortedLogicalChain.add(path); + symlinkResolutionState.logicalChain.add(rootedPath); } private void checkPathSeenDuringPartialResolution( - RootedPath rootedPath, - TreeSet sortedLogicalChain, - ArrayList logicalChain, - Environment env) + RootedPath rootedPath, SymlinkResolutionState symlinkResolutionState, Environment env) throws FileFunctionException, InterruptedException { checkPathSeenDuringPartialResolutionInternal( - rootedPath, rootedPath.asPath(), sortedLogicalChain, logicalChain, env); + rootedPath, rootedPath.asPath(), symlinkResolutionState, env); } private void checkPathSeenDuringPartialResolutionInternal( RootedPath rootedPath, Path path, - TreeSet sortedLogicalChain, - ArrayList logicalChain, + SymlinkResolutionState symlinkResolutionState, Environment env) throws FileFunctionException, InterruptedException { // We are about to perform another step of partial real path resolution. 'logicalChain' is the @@ -329,12 +319,13 @@ private void checkPathSeenDuringPartialResolutionInternal( // candidate p for (ii) and (iii). SkyKey uniquenessKey = null; FileSymlinkException fse = null; - Path seenFloorPath = sortedLogicalChain.floor(path); - Path seenCeilingPath = sortedLogicalChain.ceiling(path); - if (sortedLogicalChain.contains(path)) { + Path seenFloorPath = symlinkResolutionState.sortedLogicalChain.floor(path); + Path seenCeilingPath = symlinkResolutionState.sortedLogicalChain.ceiling(path); + if (symlinkResolutionState.sortedLogicalChain.contains(path)) { // 'rootedPath' is [transitively] a symlink to a previous element in the symlink chain (i). Pair, ImmutableList> pathAndChain = - CycleUtils.splitIntoPathAndChain(isPathPredicate(path), logicalChain); + CycleUtils.splitIntoPathAndChain( + isPathPredicate(path), symlinkResolutionState.logicalChain); FileSymlinkCycleException fsce = new FileSymlinkCycleException(pathAndChain.getFirst(), pathAndChain.getSecond()); uniquenessKey = FileSymlinkCycleUniquenessFunction.key(fsce.getCycle()); @@ -345,21 +336,26 @@ private void checkPathSeenDuringPartialResolutionInternal( Pair, ImmutableList> pathAndChain = CycleUtils.splitIntoPathAndChain( isPathPredicate(seenFloorPath), - ImmutableList.copyOf(Iterables.concat(logicalChain, ImmutableList.of(rootedPath)))); + ImmutableList.copyOf( + Iterables.concat( + symlinkResolutionState.logicalChain, ImmutableList.of(rootedPath)))); uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond()); fse = new FileSymlinkInfiniteExpansionException( pathAndChain.getFirst(), pathAndChain.getSecond()); } else if (seenCeilingPath != null && seenCeilingPath.startsWith(path)) { // 'rootedPath' is [transitively] a symlink to an ancestor of a previous element in the // symlink chain (iii). - Pair, ImmutableList> pathAndChain = - CycleUtils.splitIntoPathAndChain( - isPathPredicate(seenCeilingPath), - ImmutableList.copyOf(Iterables.concat(logicalChain, ImmutableList.of(rootedPath)))); - uniquenessKey = FileSymlinkInfiniteExpansionUniquenessFunction.key(pathAndChain.getSecond()); - fse = - new FileSymlinkInfiniteExpansionException( - pathAndChain.getFirst(), pathAndChain.getSecond()); + if (symlinkResolutionState.unboundedAncestorSymlinkExpansionChain == null) { + Pair, ImmutableList> pathAndChain = + CycleUtils.splitIntoPathAndChain( + isPathPredicate(seenCeilingPath), + ImmutableList.copyOf( + Iterables.concat( + symlinkResolutionState.logicalChain, ImmutableList.of(rootedPath)))); + symlinkResolutionState.pathToUnboundedAncestorSymlinkExpansionChain = + pathAndChain.getFirst(); + symlinkResolutionState.unboundedAncestorSymlinkExpansionChain = pathAndChain.getSecond(); + } } if (uniquenessKey != null) { // Note that this dependency is merely to ensure that each unique symlink error gets diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java index b8324859368f76..8aee2956b12fcc 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java @@ -235,6 +235,28 @@ public SkyValue compute(SkyKey skyKey, Environment env) if (!symlinkFileValue.exists()) { continue; } + + // This check is more strict than necessary: we raise an error if globbing traverses into + // a directory for any reason, even though it's only necessary if that reason was the + // resolution of a recursive glob ("**"). Fixing this would require plumbing the ancestor + // symlink information through DirectoryListingValue. + if (symlinkFileValue.isDirectory() + && symlinkFileValue.unboundedAncestorSymlinkExpansionChain() != null) { + SkyKey uniquenessKey = + FileSymlinkInfiniteExpansionUniquenessFunction.key( + symlinkFileValue.unboundedAncestorSymlinkExpansionChain()); + env.getValue(uniquenessKey); + if (env.valuesMissing()) { + return null; + } + + FileSymlinkInfiniteExpansionException symlinkException = + new FileSymlinkInfiniteExpansionException( + symlinkFileValue.pathToUnboundedAncestorSymlinkExpansionChain(), + symlinkFileValue.unboundedAncestorSymlinkExpansionChain()); + throw new GlobFunctionException(symlinkException, Transience.PERSISTENT); + } + Dirent dirent = symlinkFileMap.get(lookedUpKeyAndValue.getKey()); String fileName = dirent.getName(); if (symlinkFileValue.isDirectory()) { @@ -414,5 +436,9 @@ private static final class GlobFunctionException extends SkyFunctionException { public GlobFunctionException(InconsistentFilesystemException e, Transience transience) { super(e, transience); } + + public GlobFunctionException(FileSymlinkInfiniteExpansionException e, Transience transience) { + super(e, transience); + } } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java index 0b93eeaeb530e6..c614c183749455 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ProcessPackageDirectory.java @@ -96,6 +96,23 @@ public ProcessPackageDirectoryResult getPackageExistenceAndSubdirDeps( return ProcessPackageDirectoryResult.EMPTY_RESULT; } + if (fileValue.unboundedAncestorSymlinkExpansionChain() != null) { + SkyKey uniquenessKey = + FileSymlinkInfiniteExpansionUniquenessFunction.key( + fileValue.unboundedAncestorSymlinkExpansionChain()); + env.getValue(uniquenessKey); + if (env.valuesMissing()) { + return null; + } + + FileSymlinkInfiniteExpansionException symlinkException = + new FileSymlinkInfiniteExpansionException( + fileValue.pathToUnboundedAncestorSymlinkExpansionChain(), + fileValue.unboundedAncestorSymlinkExpansionChain()); + return reportErrorAndReturn( + symlinkException.getMessage(), symlinkException, rootRelativePath, env.getListener()); + } + PackageIdentifier packageId = PackageIdentifier.create(repositoryName, rootRelativePath); if ((packageId.getRepository().isDefault() || packageId.getRepository().isMain()) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java index 8b273878c9669b..3bba228e536c23 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java @@ -338,6 +338,19 @@ private static FileInfo lookUpFileInfo(Environment env, TraversalRequest travers if (env.valuesMissing()) { throw new MissingDepException(); } + if (fileValue.unboundedAncestorSymlinkExpansionChain() != null) { + SkyKey uniquenessKey = + FileSymlinkInfiniteExpansionUniquenessFunction.key( + fileValue.unboundedAncestorSymlinkExpansionChain()); + env.getValue(uniquenessKey); + if (env.valuesMissing()) { + throw new MissingDepException(); + } + + throw new FileSymlinkInfiniteExpansionException( + fileValue.pathToUnboundedAncestorSymlinkExpansionChain(), + fileValue.unboundedAncestorSymlinkExpansionChain()); + } if (fileValue.exists()) { // If it exists, it may either be a symlink or a file/directory. PathFragment unresolvedLinkTarget = null; diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java index dc8113f430c7f3..4e0ac4ce93750b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java @@ -459,7 +459,16 @@ public void testMutuallyReferencingSymlinks() throws Exception { @Test public void testRecursiveNestingSymlink() throws Exception { symlink("a/a", "../a"); - assertError("a/a/b"); + file("b"); + assertNoError("a/a/a/a/b"); + } + + @Test + public void testSimpleUnboundedAncestorSymlinkExpansionChainReported() throws Exception { + symlink("a/a", "../a"); + FileValue v = valueForPath(path("a/a")); + assertThat(v.unboundedAncestorSymlinkExpansionChain()) + .containsExactly(rootedPath("a/a"), rootedPath("a")); } @Test @@ -1103,7 +1112,7 @@ public void testFileStateEquality() throws Exception { public void testSymlinkToPackagePathBoundary() throws Exception { Path path = path("this/is/a/path"); FileSystemUtils.ensureSymbolicLink(path, pkgRoot.asPath()); - assertError("this/is/a/path"); + assertNoError("this/is/a/path"); } private void runTestSimpleInfiniteSymlinkExpansion( @@ -1164,20 +1173,24 @@ private void runTestSimpleInfiniteSymlinkExpansion( .setEventHandler(eventHandler) .build(); EvaluationResult result = driver.evaluate(keys, evaluationContext); - assertThat(result.hasError()).isTrue(); - for (SkyKey key : errorKeys) { - ErrorInfo errorInfo = result.getError(key); - // FileFunction detects infinite symlink expansion explicitly. - assertThat(errorInfo.getCycleInfo()).isEmpty(); - FileSymlinkInfiniteExpansionException fsiee = - (FileSymlinkInfiniteExpansionException) errorInfo.getException(); - assertThat(fsiee).hasMessageThat().contains("Infinite symlink expansion"); - assertThat(fsiee.getChain()).containsExactlyElementsIn(expectedChain).inOrder(); + if (symlinkToAncestor) { + assertThat(result.hasError()).isFalse(); + } else { + assertThat(result.hasError()).isTrue(); + for (SkyKey key : errorKeys) { + ErrorInfo errorInfo = result.getError(key); + // FileFunction detects infinite symlink expansion explicitly. + assertThat(errorInfo.getCycleInfo()).isEmpty(); + FileSymlinkInfiniteExpansionException fsiee = + (FileSymlinkInfiniteExpansionException) errorInfo.getException(); + assertThat(fsiee).hasMessageThat().contains("Infinite symlink expansion"); + assertThat(fsiee.getChain()).containsExactlyElementsIn(expectedChain).inOrder(); + } + // Check that the unique symlink expansion error was reported exactly once. + assertThat(eventHandler.getEvents()).hasSize(1); + assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage()) + .contains("infinite symlink expansion detected"); } - // Check that the unique symlink expansion error was reported exactly once. - assertThat(eventHandler.getEvents()).hasSize(1); - assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage()) - .contains("infinite symlink expansion detected"); } @Test @@ -1206,15 +1219,13 @@ public void testInfiniteSymlinkExpansion_relativeSymlinkToAncestor() throws Exce @Test public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor() throws Exception { symlink("d", "a"); - Path abPath = directory("a/b"); - Path abcPath = abPath.getChild("c"); + directory("a/b"); symlink("a/b/c", "../../d/b"); + symlink("e", "a/b/c"); + Path fPath = symlink("f", "e"); - RootedPath rootedPathABC = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(abcPath)); - RootedPath rootedPathAB = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(abPath)); - RootedPath rootedPathDB = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("d/b"))); - - SkyKey keyABC = FileValue.key(rootedPathABC); + RootedPath rootedPathF = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(fPath)); + SkyKey keyF = FileValue.key(rootedPathF); StoredEventHandler eventHandler = new StoredEventHandler(); SequentialBuildDriver driver = makeDriver(); @@ -1224,25 +1235,16 @@ public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor() throws Ex .setNumThreads(DEFAULT_THREAD_COUNT) .setEventHandler(eventHandler) .build(); - EvaluationResult result = - driver.evaluate(ImmutableList.of(keyABC), evaluationContext); + EvaluationResult result = driver.evaluate(ImmutableList.of(keyF), evaluationContext); - assertThatEvaluationResult(result).hasErrorEntryForKeyThat(keyABC).isNotTransient(); - assertThatEvaluationResult(result) - .hasErrorEntryForKeyThat(keyABC) - .hasExceptionThat() - .isInstanceOf(FileSymlinkInfiniteExpansionException.class); - FileSymlinkInfiniteExpansionException fiee = - (FileSymlinkInfiniteExpansionException) result.getError(keyABC).getException(); - assertThat(fiee).hasMessageThat().contains("Infinite symlink expansion"); - assertThat(fiee.getPathToChain()).isEmpty(); - assertThat(fiee.getChain()) - .containsExactly(rootedPathABC, rootedPathDB, rootedPathAB) + assertThatEvaluationResult(result).hasNoError(); + FileValue e = result.get(keyF); + assertThat(e.pathToUnboundedAncestorSymlinkExpansionChain()) + .containsExactly(rootedPath("f"), rootedPath("e")) + .inOrder(); + assertThat(e.unboundedAncestorSymlinkExpansionChain()) + .containsExactly(rootedPath("a/b/c"), rootedPath("d/b"), rootedPath("a/b")) .inOrder(); - - assertThat(eventHandler.getEvents()).hasSize(1); - assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage()) - .contains("infinite symlink expansion detected"); } @Test @@ -1253,10 +1255,6 @@ public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor_levelsOfDir RootedPath rootedPathDir1AB = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir1/a/b"))); - RootedPath rootedPathDir2B = - RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir2/b"))); - RootedPath rootedPathDir1 = RootedPath.toRootedPath(pkgRoot, pkgRoot.relativize(path("dir1"))); - SkyKey keyDir1AB = FileValue.key(rootedPathDir1AB); StoredEventHandler eventHandler = new StoredEventHandler(); @@ -1270,22 +1268,7 @@ public void testInfiniteSymlinkExpansion_symlinkToReferrerToAncestor_levelsOfDir EvaluationResult result = driver.evaluate(ImmutableList.of(keyDir1AB), evaluationContext); - assertThatEvaluationResult(result).hasErrorEntryForKeyThat(keyDir1AB).isNotTransient(); - assertThatEvaluationResult(result) - .hasErrorEntryForKeyThat(keyDir1AB) - .hasExceptionThat() - .isInstanceOf(FileSymlinkInfiniteExpansionException.class); - FileSymlinkInfiniteExpansionException fiee = - (FileSymlinkInfiniteExpansionException) result.getError(keyDir1AB).getException(); - assertThat(fiee).hasMessageThat().contains("Infinite symlink expansion"); - assertThat(fiee.getPathToChain()).isEmpty(); - assertThat(fiee.getChain()) - .containsExactly(rootedPathDir1AB, rootedPathDir2B, rootedPathDir1) - .inOrder(); - - assertThat(eventHandler.getEvents()).hasSize(1); - assertThat(Iterables.getOnlyElement(eventHandler.getEvents()).getMessage()) - .contains("infinite symlink expansion detected"); + assertThatEvaluationResult(result).hasNoError(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java index 1978e0fd8a1cbd..9a93bd3117267a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java @@ -684,6 +684,8 @@ public void testResilienceToFilesystemInconsistencies_directoryExistence() throw FileValue pkgDirValue = FileValue.value( ImmutableList.of(pkgRootedPath), + null, + null, pkgRootedPath, pkgDirFileStateValue, pkgRootedPath, diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java index f0823500bdfb50..0aa644ed6d91bb 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoaderTest.java @@ -226,7 +226,7 @@ public void externalFile_AssumeNonExistentAndImmutable() throws Exception { @Test public void testNonPackageEventsReported() throws Exception { path("foo").createDirectoryAndParents(); - symlink("foo/infinitesymlinkpkg", path("foo")); + symlink("foo/infinitesymlinkpkg", path("foo/infinitesymlinkpkg/subdir")); PackageIdentifier pkgId = PackageIdentifier.createInMainRepo("foo/infinitesymlinkpkg"); PackageLoader.Result result; try (PackageLoader pkgLoader = newPackageLoader()) { diff --git a/src/test/shell/bazel/bazelignore_test.sh b/src/test/shell/bazel/bazelignore_test.sh index 83b67272bc1204..6113c30b785c86 100755 --- a/src/test/shell/bazel/bazelignore_test.sh +++ b/src/test/shell/bazel/bazelignore_test.sh @@ -98,18 +98,22 @@ test_broken_BUILD_files_ignored() { || fail "directory mentioned in .bazelignore not ignored as it should" } -test_symlink_loop_ignored() { +test_symlink_cycle_ignored() { rm -rf work && mkdir work && cd work create_workspace_with_default_repos WORKSPACE mkdir -p ignoreme/deep (cd ignoreme/deep && ln -s . loop) touch BUILD - bazel build ... && fail "Expected failure" || : + + # This should really fail, but it does not: + # https://github.com/bazelbuild/bazel/issues/12148 + bazel build ... >& $TEST_log || fail "Expected success" + expect_log "Infinite symlink expansion" echo; echo echo ignoreme > .bazelignore - bazel build ... \ - || fail "directory mentioned in .bazelignore not ignored as it should" + bazel build ... >& $TEST_log || fail "Expected success" + expect_not_log "Infinite symlink expansion" } test_build_specific_target() {