Only allow regular files and directories spawn outputs to be uploaded to a remote cache.
The remote cache protocol only knows about regular files and
directories. Currently, during action output upload, symlinks are
resolved into regular files. This means cached "executions" of an
action may have different output file types than the original
execution, which can be a footgun. This CL bans symlinks from cachable
spawn outputs and fixes https://github.com/bazelbuild/bazel/issues/4840.
The interface of SpawnCache.CacheHandle.store is refactored:
1. The outputs parameter is removed, since that can be retrieved from the underlying Spawn.
2. It can now throw ExecException in order to fail actions.
Closes #4902.
Change-Id: I0d1d94d48779b970bb5d0840c66a14c189ab0091
PiperOrigin-RevId: 190608852
diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index 0a0a2a8..dd4cdea 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -41,7 +41,6 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.time.Duration;
-import java.util.ArrayList;
import java.util.List;
import java.util.SortedMap;
import java.util.concurrent.atomic.AtomicInteger;
@@ -95,8 +94,7 @@
// Actual execution.
spawnResult = spawnRunner.exec(spawn, policy);
if (cacheHandle.willStore()) {
- cacheHandle.store(
- spawnResult, listExistingOutputFiles(spawn, actionExecutionContext.getExecRoot()));
+ cacheHandle.store(spawnResult);
}
}
}
@@ -120,19 +118,6 @@
return ImmutableList.of(spawnResult);
}
- private List<Path> listExistingOutputFiles(Spawn spawn, Path execRoot) {
- ArrayList<Path> outputFiles = new ArrayList<>();
- for (ActionInput output : spawn.getOutputFiles()) {
- Path outputPath = execRoot.getRelative(output.getExecPathString());
- // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead
- // of statting the files here again.
- if (outputPath.exists()) {
- outputFiles.add(outputPath);
- }
- }
- return outputFiles;
- }
-
private final class SpawnExecutionPolicyImpl implements SpawnExecutionPolicy {
private final Spawn spawn;
private final ActionExecutionContext actionExecutionContext;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
index 1374b47..206aa58 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
@@ -19,10 +19,8 @@
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
-import com.google.devtools.build.lib.vfs.Path;
import java.io.Closeable;
import java.io.IOException;
-import java.util.Collection;
import java.util.NoSuchElementException;
/**
@@ -33,32 +31,31 @@
*/
public interface SpawnCache extends ActionContext {
/** A no-op implementation that has no result, and performs no upload. */
- public static CacheHandle NO_RESULT_NO_STORE = new CacheHandle() {
- @Override
- public boolean hasResult() {
- return false;
- }
+ public static CacheHandle NO_RESULT_NO_STORE =
+ new CacheHandle() {
+ @Override
+ public boolean hasResult() {
+ return false;
+ }
- @Override
- public SpawnResult getResult() {
- throw new NoSuchElementException();
- }
+ @Override
+ public SpawnResult getResult() {
+ throw new NoSuchElementException();
+ }
- @Override
- public boolean willStore() {
- return false;
- }
+ @Override
+ public boolean willStore() {
+ return false;
+ }
- @Override
- public void store(SpawnResult result, Collection<Path> files)
- throws InterruptedException, IOException {
- // Do nothing.
- }
+ @Override
+ public void store(SpawnResult result) throws InterruptedException, IOException {
+ // Do nothing.
+ }
- @Override
- public void close() {
- }
- };
+ @Override
+ public void close() {}
+ };
/**
* Helper method to create a {@link CacheHandle} from a successful {@link SpawnResult} instance.
@@ -81,14 +78,12 @@
}
@Override
- public void store(SpawnResult result, Collection<Path> files)
- throws InterruptedException, IOException {
+ public void store(SpawnResult result) throws InterruptedException, IOException {
throw new IllegalStateException();
}
@Override
- public void close() {
- }
+ public void close() {}
};
}
@@ -148,8 +143,7 @@
* <p>If the current thread is interrupted, then this method should return as quickly as
* possible with an {@link InterruptedException}.
*/
- void store(SpawnResult result, Collection<Path> files)
- throws InterruptedException, IOException;
+ void store(SpawnResult result) throws ExecException, InterruptedException, IOException;
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
index be81ad6..3bd935c 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java
@@ -15,13 +15,16 @@
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.concurrent.ThreadSafety;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
import com.google.devtools.build.lib.remote.util.DigestUtil;
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.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.remoteexecution.v1test.ActionResult;
import com.google.devtools.remoteexecution.v1test.Command;
import com.google.devtools.remoteexecution.v1test.Digest;
@@ -93,7 +96,7 @@
Collection<Path> files,
FileOutErr outErr,
boolean uploadAction)
- throws IOException, InterruptedException;
+ throws ExecException, IOException, InterruptedException;
/**
* Download a remote blob to a local destination.
@@ -253,10 +256,9 @@
}
}
- /**
- * The UploadManifest is used to mutualize upload between the RemoteActionCache implementations.
- */
- public class UploadManifest {
+ /** UploadManifest adds output metadata to a {@link ActionResult}. */
+ static class UploadManifest {
+ private final DigestUtil digestUtil;
private final ActionResult.Builder result;
private final Path execRoot;
private final Map<Digest, Path> digestToFile;
@@ -266,7 +268,8 @@
* Create an UploadManifest from an ActionResult builder and an exec root. The ActionResult
* builder is populated through a call to {@link #addFile(Digest, Path)}.
*/
- public UploadManifest(ActionResult.Builder result, Path execRoot) {
+ public UploadManifest(DigestUtil digestUtil, ActionResult.Builder result, Path execRoot) {
+ this.digestUtil = digestUtil;
this.result = result;
this.execRoot = execRoot;
@@ -275,24 +278,31 @@
}
/**
- * Add a collection of files (and directories) to the UploadManifest. Adding a directory has the
+ * Add a collection of files or directories to the UploadManifest. Adding a directory has the
* effect of 1) uploading a {@link Tree} protobuf message from which the whole structure of the
* directory, including the descendants, can be reconstructed and 2) uploading all the
* non-directory descendant files.
+ *
+ * <p>Attempting to a upload symlink results in a {@link
+ * com.google.build.lib.actions.ExecException}, since cachable actions shouldn't emit symlinks.
*/
- public void addFiles(Collection<Path> files) throws IOException, InterruptedException {
+ public void addFiles(Collection<Path> files)
+ throws ExecException, IOException, InterruptedException {
for (Path file : files) {
// TODO(ulfjack): Maybe pass in a SpawnResult here, add a list of output files to that, and
// rely on the local spawn runner to stat the files, instead of statting here.
- if (!file.exists()) {
+ FileStatus stat = file.statIfFound(Symlinks.NOFOLLOW);
+ if (stat == null) {
// We ignore requested results that have not been generated by the action.
continue;
}
- if (file.isDirectory()) {
+ if (stat.isDirectory()) {
addDirectory(file);
- } else {
- Digest digest = digestUtil.compute(file);
+ } else if (stat.isFile()) {
+ Digest digest = digestUtil.compute(file, stat.getSize());
addFile(digest, file);
+ } else {
+ illegalOutput(file);
}
}
}
@@ -321,7 +331,7 @@
digestToFile.put(digest, file);
}
- private void addDirectory(Path dir) throws IOException {
+ private void addDirectory(Path dir) throws ExecException, IOException {
Tree.Builder tree = Tree.newBuilder();
Directory root = computeDirectory(dir, tree);
tree.setRoot(root);
@@ -340,7 +350,8 @@
digestToChunkers.put(chunker.digest(), chunker);
}
- private Directory computeDirectory(Path path, Tree.Builder tree) throws IOException {
+ private Directory computeDirectory(Path path, Tree.Builder tree)
+ throws ExecException, IOException {
Directory.Builder b = Directory.newBuilder();
List<Dirent> sortedDirent = new ArrayList<>(path.readdir(TreeNodeRepository.SYMLINK_POLICY));
@@ -353,15 +364,27 @@
Directory dir = computeDirectory(child, tree);
b.addDirectoriesBuilder().setName(name).setDigest(digestUtil.compute(dir));
tree.addChildren(dir);
- } else {
+ } else if (dirent.getType() == Dirent.Type.FILE) {
Digest digest = digestUtil.compute(child);
b.addFilesBuilder().setName(name).setDigest(digest).setIsExecutable(child.isExecutable());
digestToFile.put(digest, child);
+ } else {
+ illegalOutput(child);
}
}
return b.build();
}
+
+ private void illegalOutput(Path what) throws ExecException, IOException {
+ String kind = what.isSymbolicLink() ? "symbolic link" : "special file";
+ throw new UserExecException(
+ String.format(
+ "Output %s is a %s. Only regular files and directories may be "
+ + "uploaded to a remote cache. "
+ + "Change the file type or add the \"no-cache\" tag/execution requirement.",
+ what.relativeTo(execRoot), kind));
+ }
}
/** Release resources associated with the cache. The cache may not be used after calling this. */
diff --git a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
index 3f5a481..a29d7d2 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/GrpcRemoteCache.java
@@ -25,6 +25,7 @@
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.remote.Retrier.RetryException;
@@ -240,7 +241,7 @@
Collection<Path> files,
FileOutErr outErr,
boolean uploadAction)
- throws IOException, InterruptedException {
+ throws ExecException, IOException, InterruptedException {
ActionResult.Builder result = ActionResult.newBuilder();
upload(execRoot, files, outErr, result);
if (!uploadAction) {
@@ -266,8 +267,8 @@
}
void upload(Path execRoot, Collection<Path> files, FileOutErr outErr, ActionResult.Builder result)
- throws IOException, InterruptedException {
- UploadManifest manifest = new UploadManifest(result, execRoot);
+ throws ExecException, IOException, InterruptedException {
+ UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot);
manifest.addFiles(files);
List<Chunker> filesToUpload = new ArrayList<>();
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index 6d76692..9b515d1 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -160,8 +160,8 @@
}
@Override
- public void store(SpawnResult result, Collection<Path> files)
- throws InterruptedException, IOException {
+ public void store(SpawnResult result)
+ throws ExecException, InterruptedException, IOException {
if (options.experimentalGuardAgainstConcurrentChanges) {
try {
checkForConcurrentModifications();
@@ -175,6 +175,8 @@
&& Status.SUCCESS.equals(result.status())
&& result.exitCode() == 0;
Context previous = withMetadata.attach();
+ Collection<Path> files =
+ RemoteSpawnRunner.resolveActionInputs(execRoot, spawn.getOutputFiles());
try {
remoteCache.upload(actionKey, execRoot, files, policy.getFileOutErr(), uploadAction);
} catch (IOException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index fb9724b..bb858a9 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -16,6 +16,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputFileCache;
@@ -442,12 +443,12 @@
return result;
}
}
- List<Path> outputFiles = listExistingOutputFiles(execRoot, spawn);
+ boolean uploadAction =
+ Spawns.mayBeCached(spawn)
+ && Status.SUCCESS.equals(result.status())
+ && result.exitCode() == 0;
+ Collection<Path> outputFiles = resolveActionInputs(execRoot, spawn.getOutputFiles());
try {
- boolean uploadAction =
- Spawns.mayBeCached(spawn)
- && Status.SUCCESS.equals(result.status())
- && result.exitCode() == 0;
remoteCache.upload(actionKey, execRoot, outputFiles, policy.getFileOutErr(), uploadAction);
} catch (IOException e) {
if (verboseFailures) {
@@ -471,16 +472,12 @@
}
}
- static List<Path> listExistingOutputFiles(Path execRoot, Spawn spawn) {
- ArrayList<Path> outputFiles = new ArrayList<>();
- for (ActionInput output : spawn.getOutputFiles()) {
- Path outputPath = execRoot.getRelative(output.getExecPathString());
- // TODO(ulfjack): Store the actual list of output files in SpawnResult and use that instead
- // of statting the files here again.
- if (outputPath.exists()) {
- outputFiles.add(outputPath);
- }
- }
- return outputFiles;
+ /** Resolve a collection of {@link com.google.build.lib.actions.ActionInput}s to {@link Path}s. */
+ static Collection<Path> resolveActionInputs(
+ Path execRoot, Collection<? extends ActionInput> actionInputs) {
+ return actionInputs
+ .stream()
+ .map((inp) -> execRoot.getRelative(inp.getExecPath()))
+ .collect(ImmutableList.toImmutableList());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
index 2d4f4e9..845cf22 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCache.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.remote;
import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
@@ -114,7 +115,7 @@
Collection<Path> files,
FileOutErr outErr,
boolean uploadAction)
- throws IOException, InterruptedException {
+ throws ExecException, IOException, InterruptedException {
ActionResult.Builder result = ActionResult.newBuilder();
upload(result, execRoot, files);
if (outErr.getErrorPath().exists()) {
@@ -131,8 +132,8 @@
}
public void upload(ActionResult.Builder result, Path execRoot, Collection<Path> files)
- throws IOException, InterruptedException {
- UploadManifest manifest = new UploadManifest(result, execRoot);
+ throws ExecException, IOException, InterruptedException {
+ UploadManifest manifest = new UploadManifest(digestUtil, result, execRoot);
manifest.addFiles(files);
for (Map.Entry<Digest, Path> entry : manifest.getDigestToFile().entrySet()) {
diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
index afd31cc..e4dc92a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/util/DigestUtil.java
@@ -60,9 +60,11 @@
}
public Digest compute(Path file) throws IOException {
- long fileSize = file.getFileSize();
- byte[] digest = DigestUtils.getDigestOrFail(file, fileSize);
- return buildDigest(digest, fileSize);
+ return compute(file, file.getFileSize());
+ }
+
+ public Digest compute(Path file, long fileSize) throws IOException {
+ return buildDigest(DigestUtils.getDigestOrFail(file, fileSize), fileSize);
}
public Digest compute(VirtualActionInput input) throws IOException {
diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
index cd8c2c39..c4d3b5b 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
@@ -34,7 +34,6 @@
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
-import java.util.Collection;
import java.util.List;
import org.junit.Before;
import org.junit.Test;
@@ -140,7 +139,7 @@
// Must only be called exactly once.
verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class));
- verify(entry).store(eq(spawnResult), any(Collection.class));
+ verify(entry).store(eq(spawnResult));
}
@SuppressWarnings("unchecked")
@@ -167,6 +166,6 @@
}
// Must only be called exactly once.
verify(spawnRunner).exec(any(Spawn.class), any(SpawnExecutionPolicy.class));
- verify(entry).store(eq(result), any(Collection.class));
+ verify(entry).store(eq(result));
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
new file mode 100644
index 0000000..b4092a0
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java
@@ -0,0 +1,71 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.remote;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.clock.JavaClock;
+import com.google.devtools.build.lib.remote.AbstractRemoteActionCache.UploadManifest;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import com.google.devtools.remoteexecution.v1test.ActionResult;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link AbstractRemoteActionCache}. */
+@RunWith(JUnit4.class)
+public class AbstractRemoteActionCacheTests {
+
+ private FileSystem fs;
+ private Path execRoot;
+ private final DigestUtil digestUtil = new DigestUtil(HashFunction.SHA256);
+
+ @Before
+ public void setUp() throws Exception {
+ fs = new InMemoryFileSystem(new JavaClock(), HashFunction.SHA256);
+ execRoot = fs.getPath("/execroot");
+ execRoot.createDirectory();
+ }
+
+ @Test
+ public void uploadSymlinkAsFile() throws Exception {
+ ActionResult.Builder result = ActionResult.newBuilder();
+ UploadManifest um = new UploadManifest(digestUtil, result, execRoot);
+ Path link = fs.getPath("/execroot/link");
+ link.createSymbolicLink(fs.getPath("/execroot/target"));
+ assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(link))))
+ .hasMessageThat()
+ .contains("Only regular files and directories may be uploaded to a remote cache.");
+ }
+
+ @Test
+ public void uploadSymlinkInDirectory() throws Exception {
+ ActionResult.Builder result = ActionResult.newBuilder();
+ UploadManifest um = new UploadManifest(digestUtil, result, fs.getPath("/execroot"));
+ Path dir = fs.getPath("/execroot/dir");
+ dir.createDirectory();
+ fs.getPath("/execroot/dir/link").createSymbolicLink(fs.getPath("/execroot/target"));
+ assertThat(assertThrows(ExecException.class, () -> um.addFiles(ImmutableList.of(dir))))
+ .hasMessageThat()
+ .contains("Only regular files and directories may be uploaded to a remote cache.");
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 4acdca8..9faf8e4 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -165,7 +165,7 @@
ImmutableMap.of("VARIABLE", "value"),
/*executionInfo=*/ ImmutableMap.<String, String>of(),
/*inputs=*/ ImmutableList.of(ActionInputHelper.fromPath("input")),
- /*outputs=*/ ImmutableList.<ActionInput>of(),
+ /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
ResourceSet.ZERO);
Path stdout = fs.getPath("/tmp/stdout");
@@ -262,7 +262,7 @@
})
.when(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
- entry.store(result, outputFiles);
+ entry.store(result);
verify(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
}
@@ -272,21 +272,22 @@
// Checks that spawns that have mayBeCached false are not looked up in the remote cache,
// and also that their result is not uploaded to the remote cache. The artifacts, however,
// are uploaded.
- SimpleSpawn uncacheableSpawn = new SimpleSpawn(
- new FakeOwner("foo", "bar"),
- /*arguments=*/ ImmutableList.of(),
- /*environment=*/ ImmutableMap.of(),
- ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
- /*inputs=*/ ImmutableList.of(),
- /*outputs=*/ ImmutableList.<ActionInput>of(),
- ResourceSet.ZERO);
+ SimpleSpawn uncacheableSpawn =
+ new SimpleSpawn(
+ new FakeOwner("foo", "bar"),
+ /*arguments=*/ ImmutableList.of(),
+ /*environment=*/ ImmutableMap.of(),
+ ImmutableMap.of(ExecutionRequirements.NO_CACHE, ""),
+ /*inputs=*/ ImmutableList.of(),
+ /*outputs=*/ ImmutableList.of(ActionInputHelper.fromPath("/random/file")),
+ ResourceSet.ZERO);
CacheHandle entry = cache.lookup(uncacheableSpawn, simplePolicy);
verify(remoteCache, never())
.getCachedActionResult(any(ActionKey.class));
assertThat(entry.hasResult()).isFalse();
SpawnResult result = new SpawnResult.Builder().setExitCode(0).setStatus(Status.SUCCESS).build();
+ entry.store(result);
ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
- entry.store(result, outputFiles);
verify(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
}
@@ -301,7 +302,7 @@
SpawnResult result =
new SpawnResult.Builder().setExitCode(1).setStatus(Status.NON_ZERO_EXIT).build();
ImmutableList<Path> outputFiles = ImmutableList.of(fs.getPath("/random/file"));
- entry.store(result, outputFiles);
+ entry.store(result);
verify(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(false));
}
@@ -317,7 +318,7 @@
.when(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
- entry.store(result, outputFiles);
+ entry.store(result);
verify(remoteCache)
.upload(any(ActionKey.class), any(Path.class), eq(outputFiles), eq(outErr), eq(true));
diff --git a/src/test/shell/bazel/remote/remote_execution_http_test.sh b/src/test/shell/bazel/remote/remote_execution_http_test.sh
index fa21ada..4bd27555 100755
--- a/src/test/shell/bazel/remote/remote_execution_http_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_http_test.sh
@@ -127,6 +127,52 @@
fi
}
+function test_refuse_to_upload_symlink() {
+ cat > BUILD <<'EOF'
+genrule(
+ name = 'make-link',
+ outs = ['l', 't'],
+ cmd = 'touch $(location t) && ln -s t $(location l)',
+)
+EOF
+ bazel build \
+ --genrule_strategy=remote \
+ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+ //:make-link &> $TEST_log \
+ && fail "should have failed" || true
+ expect_log "/l is a symbolic link"
+
+ bazel build \
+ --experimental_remote_spawn_cache \
+ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+ //:make-link &> $TEST_log \
+ && fail "should have failed" || true
+ expect_log "/l is a symbolic link"
+}
+
+function test_refuse_to_upload_symlink_in_directory() {
+ cat > BUILD <<'EOF'
+genrule(
+ name = 'make-link',
+ outs = ['dir'],
+ cmd = 'mkdir $(location dir) && touch $(location dir)/t && ln -s t $(location dir)/l',
+)
+EOF
+ bazel build \
+ --genrule_strategy=remote \
+ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+ //:make-link &> $TEST_log \
+ && fail "should have failed" || true
+ expect_log "dir/l is a symbolic link"
+
+ bazel build \
+ --experimental_remote_spawn_cache \
+ --remote_http_cache=http://localhost:${hazelcast_port}/hazelcast/rest/maps \
+ //:make-link &> $TEST_log \
+ && fail "should have failed" || true
+ expect_log "dir/l is a symbolic link"
+}
+
function set_directory_artifact_testfixtures() {
mkdir -p a
cat > a/BUILD <<'EOF'
diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
index 277b882..2f5c6eb 100755
--- a/src/test/shell/bazel/remote/remote_execution_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -522,6 +522,23 @@
expect_log "Remote connection/protocol failed"
}
+function test_refuse_symlink_output() {
+ cat > BUILD <<'EOF'
+genrule(
+ name = 'make-link',
+ outs = ['l', 't'],
+ cmd = 'touch $(location t) && ln -s t $(location l)',
+)
+EOF
+
+ bazel build \
+ --genrule_strategy=remote \
+ --remote_executor=localhost:${worker_port} \
+ //:make-link >& TEST_log \
+ && fail "should have failed"# || true
+ expect_log "/l is a symbolic link"
+}
+
# TODO(alpha): Add a test that fails remote execution when remote worker
# supports sandbox.
diff --git a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
index 098b4ad..980822c 100644
--- a/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
+++ b/src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
@@ -24,6 +24,7 @@
import com.google.common.util.concurrent.ListeningExecutorService;
import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.remote.CacheNotFoundException;
import com.google.devtools.build.lib.remote.ExecutionStatusException;
import com.google.devtools.build.lib.remote.SimpleBlobStoreActionCache;
@@ -252,6 +253,7 @@
(cmdResult != null && cmdResult.getTerminationStatus().timedOut())
|| wasTimeout(timeoutMillis, System.currentTimeMillis() - startTime);
final int exitCode;
+ Status errStatus = null;
ExecuteResponse.Builder resp = ExecuteResponse.newBuilder();
if (wasTimeout) {
final String errMessage =
@@ -259,11 +261,11 @@
"Command:\n%s\nexceeded deadline of %f seconds.",
Arrays.toString(command.getArgumentsList().toArray()), timeoutMillis / 1000.0);
logger.warning(errMessage);
- resp.setStatus(
+ errStatus =
Status.newBuilder()
.setCode(Code.DEADLINE_EXCEEDED.getNumber())
.setMessage(errMessage)
- .build());
+ .build();
exitCode = LOCAL_EXEC_ERROR;
} else if (cmdResult == null) {
exitCode = LOCAL_EXEC_ERROR;
@@ -272,19 +274,29 @@
}
ActionResult.Builder result = ActionResult.newBuilder();
- cache.upload(result, execRoot, outputs);
+ try {
+ cache.upload(result, execRoot, outputs);
+ } catch (ExecException e) {
+ if (errStatus == null) {
+ errStatus =
+ Status.newBuilder()
+ .setCode(Code.FAILED_PRECONDITION.getNumber())
+ .setMessage(e.getMessage())
+ .build();
+ }
+ }
byte[] stdout = cmdResult.getStdout();
byte[] stderr = cmdResult.getStderr();
cache.uploadOutErr(result, stdout, stderr);
ActionResult finalResult = result.setExitCode(exitCode).build();
- if (exitCode == 0 && !action.getDoNotCache()) {
+ resp.setResult(finalResult);
+ if (errStatus != null) {
+ resp.setStatus(errStatus);
+ throw new ExecutionStatusException(errStatus, resp.build());
+ } else if (exitCode == 0 && !action.getDoNotCache()) {
ActionKey actionKey = digestUtil.computeActionKey(action);
cache.setCachedActionResult(actionKey, finalResult);
}
- resp.setResult(finalResult);
- if (wasTimeout) {
- throw new ExecutionStatusException(resp.getStatus(), resp.build());
- }
return finalResult;
}
