Skip to content

Commit

Permalink
Prevent non-hermetic worker flagfile reads
Browse files Browse the repository at this point in the history 
A worker command line referencing a flag file that is not an input to the action now results in an error instead of a non-hermetic file read.

Work towards #6526

Closes #18156.

PiperOrigin-RevId: 537883391
Change-Id: Ia72124716dbb9b51dbe8e6c3795cee117c2d0c4a
  • Loading branch information
@fmeum @copybara-github
fmeum authored and copybara-github committed Jun 5, 2023
1 parent 5539265 commit 36b2f8c
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 16 deletions.
28 changes: 19 additions & 9 deletions src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.XattrProvider;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse;
Expand Down Expand Up @@ -246,14 +247,15 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
private WorkRequest createWorkRequest(
Spawn spawn,
SpawnExecutionContext context,
SandboxInputs inputFiles,
List<String> flagfiles,
Map<VirtualActionInput, byte[]> virtualInputDigests,
InputMetadataProvider inputFileCache,
WorkerKey key)
throws IOException, InterruptedException {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
for (String flagfile : flagfiles) {
expandArgument(execRoot, flagfile, requestBuilder);
expandArgument(inputFiles, flagfile, requestBuilder);
}

List<ActionInput> inputs =
Expand Down Expand Up @@ -298,27 +300,34 @@ private WorkRequest createWorkRequest(
* <p>Also check that the argument is not an external repository label, because they start with
* `@` and are not flagfile locations.
*
* @param execRoot the current execroot of the build (relative paths will be assumed to be
* relative to this directory).
* @param inputs the inputs to locate flag files in.
* @param arg the argument to expand.
* @param requestBuilder the WorkRequest to whose arguments the expanded arguments will be added.
* @throws java.io.IOException if one of the files containing options cannot be read.
*/
static void expandArgument(Path execRoot, String arg, WorkRequest.Builder requestBuilder)
static void expandArgument(SandboxInputs inputs, String arg, WorkRequest.Builder requestBuilder)
throws IOException, InterruptedException {
if (arg.startsWith("@") && !arg.startsWith("@@") && !isExternalRepositoryLabel(arg)) {
if (Thread.interrupted()) {
throw new InterruptedException();
}
String argValue = arg.substring(1);
Path path = execRoot.getRelative(argValue);
RootedPath path = inputs.getFiles().get(PathFragment.create(argValue));
if (path == null) {
throw new IOException(
String.format(
"Failed to read @-argument '%s': file is not a declared input", argValue));
}
try {
for (String line : FileSystemUtils.readLines(path, UTF_8)) {
expandArgument(execRoot, line, requestBuilder);
for (String line : FileSystemUtils.readLines(path.asPath(), UTF_8)) {
expandArgument(inputs, line, requestBuilder);
}
} catch (IOException e) {
throw new IOException(
String.format("Failed to read @-argument '%s' from file '%s'.", argValue, path), e);
String.format(
"Failed to read @-argument '%s' from file '%s'.",
argValue, path.asPath().getPathString()),
e);
}
} else {
requestBuilder.addArguments(arg);
Expand Down Expand Up @@ -418,7 +427,8 @@ WorkResponse execInWorker(
workerOwner = new WorkerOwner(handle.getWorker());
workerOwner.getWorker().setReporter(workerOptions.workerVerbose ? reporter : null);
request =
createWorkRequest(spawn, context, flagFiles, virtualInputDigests, inputFileCache, key);
createWorkRequest(
spawn, context, inputFiles, flagFiles, virtualInputDigests, inputFileCache, key);

// We acquired a worker and resources -- mark that as queuing time.
spawnMetrics.setQueueTimeInMs((int) queueStopwatch.elapsed().toMillis());
Expand Down
48 changes: 44 additions & 4 deletions src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.build.lib.worker.WorkerPoolImpl.WorkerPoolConfig;
Expand Down Expand Up @@ -435,7 +438,16 @@ public void testExpandArgument_expandsArgumentsRecursively()
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
FileSystemUtils.writeIsoLatin1(fs.getPath("/file"), "arg1\n@file2\nmulti arg\n");
FileSystemUtils.writeIsoLatin1(fs.getPath("/file2"), "arg2\narg3");
WorkerSpawnRunner.expandArgument(fs.getPath("/"), "@file", requestBuilder);
SandboxInputs inputs =
new SandboxInputs(
ImmutableMap.of(
PathFragment.create("file"),
asRootedPath("/file"),
PathFragment.create("file2"),
asRootedPath("/file2")),
ImmutableMap.of(),
ImmutableMap.of(), ImmutableMap.of());
WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder);
assertThat(requestBuilder.getArgumentsList())
.containsExactly("arg1", "arg2", "arg3", "multi arg", "");
}
Expand All @@ -445,18 +457,28 @@ public void testExpandArgument_expandsOnlyProperArguments()
throws IOException, InterruptedException {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
FileSystemUtils.writeIsoLatin1(fs.getPath("/file"), "arg1\n@@nonfile\n@foo//bar\narg2");
WorkerSpawnRunner.expandArgument(fs.getPath("/"), "@file", requestBuilder);
SandboxInputs inputs =
new SandboxInputs(
ImmutableMap.of(PathFragment.create("file"), asRootedPath("/file")), ImmutableMap.of(),
ImmutableMap.of(), ImmutableMap.of());
WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder);
assertThat(requestBuilder.getArgumentsList())
.containsExactly("arg1", "@@nonfile", "@foo//bar", "arg2");
}

@Test
public void testExpandArgument_failsOnMissingFile() throws IOException {
public void testExpandArgument_failsOnMissingFile() {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
SandboxInputs inputs =
new SandboxInputs(
ImmutableMap.of(PathFragment.create("file"), asRootedPath("/dir/file")),
ImmutableMap.of(),
ImmutableMap.of(),
ImmutableMap.of());
IOException e =
assertThrows(
IOException.class,
() -> WorkerSpawnRunner.expandArgument(fs.getPath("/dir/"), "@file", requestBuilder));
() -> WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder));
assertThat(e).hasMessageThat().contains("file");
assertThat(e).hasMessageThat().contains("/dir/file");
}
Expand Down Expand Up @@ -564,6 +586,24 @@ private WorkerSpawnRunner createWorkerSpawnRunner(WorkerOptions workerOptions) {
new JavaClock());
}

@Test
public void testExpandArgument_failsOnUndeclaredInput() {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
SandboxInputs inputs =
new SandboxInputs(
ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of(), ImmutableMap.of());
IOException e =
assertThrows(
IOException.class,
() -> WorkerSpawnRunner.expandArgument(inputs, "@file", requestBuilder));
assertThat(e).hasMessageThat().contains("file");
assertThat(e).hasMessageThat().contains("declared input");
}

private RootedPath asRootedPath(String path) {
return RootedPath.toRootedPath(Root.absoluteRoot(fs), fs.getPath(path));
}

private static String logMarker(String text) {
return "---8<---8<--- " + text + " ---8<---8<---\n";
}
Expand Down
15 changes: 12 additions & 3 deletions src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.sandbox.SandboxHelpers.SandboxInputs;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.lib.vfs.util.FileSystems;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;
import java.io.File;
Expand Down Expand Up @@ -47,9 +51,14 @@ public void expandArgumentsPreservesEmptyLines() throws Exception {
flags.forEach(pw::println);
}

Path execRoot = fs.getPath(folder.getRoot().getAbsolutePath());
RootedPath path =
RootedPath.toRootedPath(Root.absoluteRoot(fs), fs.getPath(flagfile.getAbsolutePath()));
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
WorkerSpawnRunner.expandArgument(execRoot, "@flagfile.txt", requestBuilder);
SandboxInputs inputs =
new SandboxInputs(
ImmutableMap.of(PathFragment.create("flagfile.txt"), path), ImmutableMap.of(),
ImmutableMap.of(), ImmutableMap.of());
WorkerSpawnRunner.expandArgument(inputs, "@flagfile.txt", requestBuilder);

assertThat(requestBuilder.getArgumentsList()).containsExactlyElementsIn(flags);
}
Expand Down

0 comments on commit 36b2f8c

Please sign in to comment.