Skip to content

Commit

Permalink
Automated [] rollback of commit 525fa71.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Contributor finds some bugs and after fixing some bugs there are more bugs to fix now.

*** Original change description ***


Mount whole directories into the sandbox when possible

This halves the overhead with sandboxing enabled vs disabled for a test
that basically only mounts a bunch of files out of a directory, and
slows that same test with a single extra file added to the directory
(but not mounted) by only ~4%.

The test is <https://gist.github.com/bsilver8192/10527a862ce16bb7f79a>
with 30000 inputs moved to a subdirectory and on...

***

ROLLBACK_OF=119138157

--
MOS_MIGRATED_REVID=119828267
  • Loading branch information
@hermione521 @dslomov
hermione521 authored and dslomov committed Apr 14, 2016
1 parent 7c50909 commit 386f242
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 179 deletions.
93 changes: 5 additions & 88 deletions src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import com.google.devtools.build.lib.unix.NativePosixFiles;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
Expand All @@ -55,12 +54,9 @@
import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.AbstractMap.SimpleImmutableEntry;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -258,70 +254,13 @@ private static void finalizeMountPath(
MountMap finalizedMounts, Path target, Path source, FileStatus stat) throws IOException {
// The source must exist.
Preconditions.checkArgument(stat != null, "%s does not exist", source.toString());
finalizedMounts.put(target, source);

if (stat.isSymbolicLink()) {
Path symlinkTarget = source.resolveSymbolicLinks();
Preconditions.checkArgument(
symlinkTarget.exists(), "%s does not exist", symlinkTarget.toString());
finalizedMounts.put(target, symlinkTarget);
} else {
finalizedMounts.put(target, source);
}
}

/**
* Checks if a folder is fully mounted (meaning it can be mounted as a whole instead
* of mounting its contents individually).
*
* The results of previous lookups are cached to make this process faster, and this
* function updates those caches as appropriate.
*
* @param directory the directory to check for being fully mounted or not
* @param mounts all of the files which are being mounted
* @param fullyMountedDirectories all the directories which have been found to be
* fully mounted previously
* @param notFullyMountedDirectories all the directories which been found to not be
* fully mounted previously
*/
private static boolean isFullyMounted(
Entry<Path, Path> directory,
MountMap mounts,
Set<Entry<Path, Path>> fullyMountedDirectories,
Set<Entry<Path, Path>> notFullyMountedDirectories)
throws IOException {
if (fullyMountedDirectories.contains(directory)) {
return true;
} else if (notFullyMountedDirectories.contains(directory)) {
return false;
} else {
boolean result = true;
for (Dirent entry : directory.getValue().readdir(Symlinks.NOFOLLOW)) {
Path entryKey = directory.getKey().getChild(entry.getName());
Path entryValue = directory.getValue().getChild(entry.getName());
if (entry.getType() == Dirent.Type.DIRECTORY) {
if (!isFullyMounted(
new SimpleImmutableEntry<Path, Path>(entryKey, entryValue),
mounts,
fullyMountedDirectories,
notFullyMountedDirectories)) {
result = false;
break;
}
} else {
Path mountsValue = mounts.get(entryKey);
if (mountsValue == null || !mountsValue.equals(entryValue)) {
result = false;
break;
}
}
}

if (result) {
fullyMountedDirectories.add(directory);
} else {
notFullyMountedDirectories.add(directory);
}
return result;
finalizedMounts.put(symlinkTarget, symlinkTarget);
}
}

Expand All @@ -341,7 +280,6 @@ private static boolean isFullyMounted(
*/
@VisibleForTesting
static MountMap finalizeMounts(Map<Path, Path> mounts) throws IOException {
Set<Entry<Path, Path>> fullyMountedDirectories = new HashSet<>();
MountMap finalizedMounts = new MountMap();
for (Entry<Path, Path> mount : mounts.entrySet()) {
Path target = mount.getKey();
Expand All @@ -350,37 +288,16 @@ static MountMap finalizeMounts(Map<Path, Path> mounts) throws IOException {
FileStatus stat = source.statNullable(Symlinks.NOFOLLOW);

if (stat != null && stat.isDirectory()) {
fullyMountedDirectories.add(new SimpleImmutableEntry<Path, Path>(target, source));
for (Path subSource : FileSystemUtils.traverseTree(source, Predicates.alwaysTrue())) {
Path subTarget = target.getRelative(subSource.relativeTo(source));
FileStatus subStat = subSource.statNullable(Symlinks.NOFOLLOW);
if (subStat.isDirectory()) {
fullyMountedDirectories.add(new SimpleImmutableEntry<Path, Path>(subTarget, subSource));
}
finalizeMountPath(finalizedMounts, subTarget, subSource, subStat);
finalizeMountPath(
finalizedMounts, subTarget, subSource, subSource.statNullable(Symlinks.NOFOLLOW));
}
} else {
finalizeMountPath(finalizedMounts, target, source, stat);
}
}

MountMap deduplicatedMounts = new MountMap();
Set<Entry<Path, Path>> notFullyMountedDirectories = new HashSet<>();
for (Entry<Path, Path> mount : finalizedMounts.entrySet()) {
Entry<Path, Path> parent =
new SimpleImmutableEntry<>(
mount.getKey().getParentDirectory(), mount.getValue().getParentDirectory());
if (!parent.getKey().equals(parent.getValue())
|| !isFullyMounted(
parent, finalizedMounts, fullyMountedDirectories, notFullyMountedDirectories)) {
deduplicatedMounts.put(mount.getKey(), mount.getValue());
}
}

for (Entry<Path, Path> mount : fullyMountedDirectories) {
deduplicatedMounts.put(mount.getKey(), mount.getValue());
}
return deduplicatedMounts;
return finalizedMounts;
}

/**
Expand Down
7 changes: 3 additions & 4 deletions src/main/tools/namespace-sandbox.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,9 @@ static void Usage(int argc, char *const *argv, const char *fmt, ...) {
" -t <timeout> in case timeout occurs, how long to wait before killing "
"the child with SIGKILL\n"
" -d <dir> create an empty directory in the sandbox\n"
" -M/-m <source/target> system file or directory to mount inside the "
"sandbox\n"
" Multiple files or directories can be specified and each of them "
"will be mounted readonly.\n"
" -M/-m <source/target> system directory to mount inside the sandbox\n"
" Multiple directories can be specified and each of them will be "
"mounted readonly.\n"
" The -M option specifies which directory to mount, the -m option "
"specifies where to\n"
" mount it in the sandbox.\n"
Expand Down
96 changes: 9 additions & 87 deletions src/test/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedStrategyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import java.io.IOException;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -104,9 +105,6 @@ private Map<Path, Path> mounts(Map<String, String> linksAndFiles) throws Excepti
private ImmutableMap<String, String> userFriendlyAsserts(List<String> asserts) {
return userFriendlyMap(asserts(asserts));
}
private ImmutableMap<String, String> userFriendlyAsserts(Map<String, String> asserts) {
return userFriendlyMap(asserts(asserts));
}

private ImmutableMap<Path, Path> asserts(List<String> asserts) {
ImmutableMap.Builder<Path, Path> pathifiedAsserts = ImmutableMap.builder();
Expand All @@ -117,15 +115,6 @@ private ImmutableMap<Path, Path> asserts(List<String> asserts) {
return pathifiedAsserts.build();
}

private ImmutableMap<Path, Path> asserts(Map<String, String> asserts) {
ImmutableMap.Builder<Path, Path> pathifiedAsserts = ImmutableMap.builder();
for (Map.Entry<String, String> file : asserts.entrySet()) {
pathifiedAsserts.put(
workspaceDir.getRelative(file.getKey()), workspaceDir.getRelative(file.getValue()));
}
return pathifiedAsserts.build();
}

private void createTreeStructure(Map<String, String> linksAndFiles) throws Exception {
for (Entry<String, String> entry : linksAndFiles.entrySet()) {
Path filePath = workspaceDir.getRelative(entry.getKey());
Expand All @@ -148,9 +137,10 @@ public void testResolvesRelativeFileToFileSymlinkInSameDir() throws Exception {
Map<String, String> testFiles = new LinkedHashMap<>();
testFiles.put("symlink.txt", "goal.txt");
testFiles.put("goal.txt", "");
testFiles.put("other.txt", "");

Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "goal.txt");
List<String> assertMounts = new ArrayList<>();
assertMounts.add("symlink.txt");
assertMounts.add("goal.txt");

assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
}
Expand All @@ -160,10 +150,9 @@ public void testResolvesRelativeFileToFileSymlinkInSubDir() throws Exception {
Map<String, String> testFiles =
ImmutableMap.of(
"symlink.txt", "x/goal.txt",
"x/goal.txt", "",
"x/other.txt", "");
"x/goal.txt", "");

Map<String, String> assertMounts = ImmutableMap.of("symlink.txt", "x/goal.txt");
List<String> assertMounts = ImmutableList.of("symlink.txt", "x/goal.txt");
assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
}

Expand All @@ -172,10 +161,9 @@ public void testResolvesRelativeFileToFileSymlinkInParentDir() throws Exception
Map<String, String> testFiles =
ImmutableMap.of(
"x/symlink.txt", "../goal.txt",
"goal.txt", "",
"x/other.txt", "");
"goal.txt", "");

Map<String, String> assertMounts = ImmutableMap.of("x/symlink.txt", "goal.txt");
List<String> assertMounts = ImmutableList.of("x/symlink.txt", "goal.txt");

assertThat(userFriendlyMounts(testFiles)).isEqualTo(userFriendlyAsserts(assertMounts));
}
Expand All @@ -190,73 +178,7 @@ public void testRecursesSubDirs() throws Exception {
"a/b/y.txt", "z.txt",
"a/b/z.txt", "");

List<String> assertMounts = ImmutableList.of("a/b");

assertThat(userFriendlyMounts(testFiles, inputFile))
.isEqualTo(userFriendlyAsserts(assertMounts));
}

@Test
public void testDetectsWholeDir() throws Exception {
ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/z.txt");

Map<String, String> testFiles =
ImmutableMap.of(
"a/x.txt", "",
"a/z.txt", "");

List<String> assertMounts = ImmutableList.of("a");

assertThat(userFriendlyMounts(testFiles, inputFile))
.isEqualTo(userFriendlyAsserts(assertMounts));
}

@Test
public void testExcludesOtherDir() throws Exception {
ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/y.txt");

Map<String, String> testFiles =
ImmutableMap.of(
"a/x.txt", "",
"a/y.txt", "",
"a/b/", "");

List<String> assertMounts = ImmutableList.of("a/x.txt", "a/y.txt");

assertThat(userFriendlyMounts(testFiles, inputFile))
.isEqualTo(userFriendlyAsserts(assertMounts));
}

@Test
public void testExcludesOtherFiles() throws Exception {
ImmutableList<String> inputFile = ImmutableList.of("a/x.txt", "a/z.txt");

Map<String, String> testFiles =
ImmutableMap.of(
"a/x.txt", "",
"a/y.txt", "z.txt",
"a/z.txt", "");

List<String> assertMounts = ImmutableList.of("a/x.txt", "a/z.txt");

assertThat(userFriendlyMounts(testFiles, inputFile))
.isEqualTo(userFriendlyAsserts(assertMounts));
}

@Test
public void testRecognizesOtherSymlinks() throws Exception {
ImmutableList<String> inputFile = ImmutableList.of("a/a/x.txt", "a/a/y.txt");

Map<String, String> testFiles =
ImmutableMap.of(
"a/a/x.txt", "../b/x.txt",
"a/a/y.txt", "",
"a/b/x.txt", "");

Map<String, String> assertMounts =
ImmutableMap.of(
"a/a/x.txt", "a/b/x.txt",
"a/a/y.txt", "a/a/y.txt");
List<String> assertMounts = ImmutableList.of("a/b/x.txt", "a/b/y.txt", "a/b/z.txt");

assertThat(userFriendlyMounts(testFiles, inputFile))
.isEqualTo(userFriendlyAsserts(assertMounts));
Expand Down

0 comments on commit 386f242

Please sign in to comment.