From e2e235359ccdc4b6a122586b9d7c99abbddd65f5 Mon Sep 17 00:00:00 2001 From: nharmata Date: Wed, 6 Feb 2019 21:00:01 -0800 Subject: [PATCH] Fix blatant logic bug in FileFunction that causes incorrect FileValues on incremental evaluations. For a path like "/b", we should be using FileStateValue(r/b), where "r" is the fully resolved path of "", when we construct the FileValue. This is important in the case where "" has a different resolved path (due to ancestor directory symlinks). This was the intent of the FileFunction + FileStateValue algorithm, but the code's structure hid the bug. While I'm here, also slightly improve this. The bug is: if "r/b" changes (e.g. goes from a directory to a symlink, or goes from a symlink pointing to t1 to a symlink pointing to t2), then the Skyframe's diff will correctly contain FileStateValue(r/b). But if FileFunction(/b) uses FileStateValue(/b), then FileValue(/b) will be wrong. Note that FileFunction#resolveFromAncestors is already aptly declaring the dep on FileValue(r/b) -- the bug is that we're not using it when constructing the FileValue! Fwiw, this bug is most easily exploited when multiple levels of directory symlinks are involved. That's a fairly obscure setup, so I'm not super surprised it took us this long to notice this bug. RELNOTES: None PiperOrigin-RevId: 232803261 --- .../devtools/build/lib/actions/FileValue.java | 28 ++++++---- .../build/lib/skyframe/FileFunction.java | 55 +++++++++---------- .../build/lib/skyframe/FileFunctionTest.java | 37 +++++++++++-- 3 files changed, 76 insertions(+), 44 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 996ef03cb4691b..bf6befed98e60d 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 @@ -150,22 +150,26 @@ public SkyFunctionName functionName() { * not be used for symlink cycles. */ public static FileValue value( - RootedPath rootedPath, - FileStateValue fileStateValue, + RootedPath originalRootedPath, + FileStateValue fileStateValueFromAncestors, RootedPath realRootedPath, FileStateValue realFileStateValue) { - if (rootedPath.equals(realRootedPath)) { - Preconditions.checkState(fileStateValue.getType() != FileStateType.SYMLINK, - "rootedPath: %s, fileStateValue: %s, realRootedPath: %s, realFileStateValue: %s", - rootedPath, fileStateValue, realRootedPath, realFileStateValue); - return new RegularFileValue(rootedPath, fileStateValue); + if (originalRootedPath.equals(realRootedPath)) { + Preconditions.checkState( + fileStateValueFromAncestors.getType() != FileStateType.SYMLINK, + "originalRootedPath: %s, fileStateValue: %s, realRootedPath: %s, " + + "fileStateValueFromAncestors: %s", + originalRootedPath, + fileStateValueFromAncestors, + realRootedPath, + realFileStateValue); + return new RegularFileValue(originalRootedPath, fileStateValueFromAncestors); } else { - if (fileStateValue.getType() == FileStateType.SYMLINK) { - return new SymlinkFileValue(realRootedPath, realFileStateValue, - fileStateValue.getSymlinkTarget()); + if (fileStateValueFromAncestors.getType() == FileStateType.SYMLINK) { + return new SymlinkFileValue( + realRootedPath, realFileStateValue, fileStateValueFromAncestors.getSymlinkTarget()); } else { - return new DifferentRealPathFileValue( - realRootedPath, realFileStateValue); + return new DifferentRealPathFileValue(realRootedPath, realFileStateValue); } } } 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 a0088593748edc..269a52149392a4 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 @@ -71,45 +71,42 @@ public FileFunction(AtomicReference pkgLocator) { public FileValue compute(SkyKey skyKey, Environment env) throws FileFunctionException, InterruptedException { RootedPath rootedPath = (RootedPath) skyKey.argument(); - RootedPath realRootedPath = null; - FileStateValue realFileStateValue = null; PathFragment relativePath = rootedPath.getRootRelativePath(); - // Resolve ancestor symlinks, 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). Note that this is the first thing we do - if 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. - if (!relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { + // 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). + // + // This entails resolving ancestor symlinks fully. Note that this is the first thing we do - if + // 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. + RootedPath rootedPathFromAncestors; + FileStateValue fileStateValueFromAncestors; + if (relativePath.equals(PathFragment.EMPTY_FRAGMENT)) { + FileStateValue fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath)); + if (fileStateValue == null) { + return null; + } + rootedPathFromAncestors = rootedPath; + fileStateValueFromAncestors = fileStateValue; + } else { Pair resolvedState = resolveFromAncestors(rootedPath, env); if (resolvedState == null) { return null; } - realRootedPath = resolvedState.getFirst(); - realFileStateValue = resolvedState.getSecond(); - if (realFileStateValue.getType() == FileStateType.NONEXISTENT) { + rootedPathFromAncestors = resolvedState.getFirst(); + fileStateValueFromAncestors = resolvedState.getSecond(); + if (fileStateValueFromAncestors.getType() == FileStateType.NONEXISTENT) { return FileValue.value( rootedPath, FileStateValue.NONEXISTENT_FILE_STATE_NODE, - realRootedPath, - realFileStateValue); - } - } - - FileStateValue fileStateValue; - if (rootedPath.equals(realRootedPath)) { - fileStateValue = Preconditions.checkNotNull(realFileStateValue, rootedPath); - } else { - fileStateValue = (FileStateValue) env.getValue(FileStateValue.key(rootedPath)); - if (fileStateValue == null) { - return null; + rootedPathFromAncestors, + fileStateValueFromAncestors); } } - if (realFileStateValue == null) { - realRootedPath = rootedPath; - realFileStateValue = fileStateValue; - } + RootedPath realRootedPath = rootedPathFromAncestors; + FileStateValue realFileStateValue = fileStateValueFromAncestors; ArrayList symlinkChain = new ArrayList<>(); TreeSet orderedSeenPaths = Sets.newTreeSet(); @@ -124,7 +121,9 @@ public FileValue compute(SkyKey skyKey, Environment env) realRootedPath = resolvedState.getFirst(); realFileStateValue = resolvedState.getSecond(); } - return FileValue.value(rootedPath, fileStateValue, realRootedPath, realFileStateValue); + + return FileValue.value( + rootedPath, fileStateValueFromAncestors, realRootedPath, realFileStateValue); } /** 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 22835e90efd766..2defb07818f732 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 @@ -199,17 +199,17 @@ private SequentialBuildDriver makeDriver(ExternalFileAction externalFileAction) } private FileValue valueForPath(Path path) throws InterruptedException { - return valueForPathHelper(pkgRoot, path); + return valueForPathHelper(pkgRoot, path, makeDriver()); } private FileValue valueForPathOutsidePkgRoot(Path path) throws InterruptedException { - return valueForPathHelper(Root.absoluteRoot(fs), path); + return valueForPathHelper(Root.absoluteRoot(fs), path, makeDriver()); } - private FileValue valueForPathHelper(Root root, Path path) throws InterruptedException { + private FileValue valueForPathHelper(Root root, Path path, SequentialBuildDriver driver) + throws InterruptedException { PathFragment pathFragment = root.relativize(path); RootedPath rootedPath = RootedPath.toRootedPath(root, pathFragment); - SequentialBuildDriver driver = makeDriver(); SkyKey key = FileValue.key(rootedPath); EvaluationResult result = driver.evaluate(ImmutableList.of(key), EVALUATION_OPTIONS); assertThat(result.hasError()).isFalse(); @@ -1261,6 +1261,35 @@ public void testInjectionOverIOException() throws Exception { assertThat(result.get(fooKey).exists()).isTrue(); } + @Test + public void testMultipleLevelsOfDirectorySymlinks_Clean() throws Exception { + symlink("a/b/c", "../c"); + Path abcd = path("a/b/c/d"); + symlink("a/c/d", "../d"); + assertThat(valueForPath(abcd).isSymlink()).isTrue(); + } + + @Test + public void testMultipleLevelsOfDirectorySymlinks_Incremental() throws Exception { + SequentialBuildDriver driver = makeDriver(); + + symlink("a/b/c", "../c"); + Path acd = directory("a/c/d"); + Path abcd = path("a/b/c/d"); + + FileValue abcdFileValue = valueForPathHelper(pkgRoot, abcd, driver); + assertThat(abcdFileValue.isDirectory()).isTrue(); + assertThat(abcdFileValue.isSymlink()).isFalse(); + + acd.delete(); + symlink("a/c/d", "../d"); + differencer.invalidate(ImmutableList.of(fileStateSkyKey("a/c/d"))); + + abcdFileValue = valueForPathHelper(pkgRoot, abcd, driver); + + assertThat(abcdFileValue.isSymlink()).isTrue(); + } + private void checkRealPath(String pathString) throws Exception { Path realPath = pkgRoot.getRelative(pathString).resolveSymbolicLinks(); assertRealPath(pathString, pkgRoot.relativize(realPath).toString());