Avoid exceptions from hermetic sandbox for unsupported artifact subcl…

…asses

Avoid exceptions from linux hermetic sandbox for unsupported FileArtifactValue subclasses.

Also adds test cases to confirm no regression of existing functionality.

Fixes bazelbuild#15340

Closes bazelbuild#16739.

Change-Id: I0f1373f6f99328b8277fe1cec87d3946b83481c1
PiperOrigin-RevId: 490490477

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

@@ -66,6 +66,8 @@ final class LinuxSandboxedSpawnRunner extends AbstractSandboxSpawnRunner {
   private static final Map<Path, Boolean> isSupportedMap = new HashMap<>();
   private static final AtomicBoolean warnedAboutNonHermeticTmp = new AtomicBoolean();
 
+  private static final AtomicBoolean warnedAboutUnsupportedModificationCheck = new AtomicBoolean();
+
   /**
    * Returns whether the linux sandbox is supported on the local machine by running a small command
    * in it.
@@ -216,9 +218,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     ImmutableSet<Path> writableDirs = getWritableDirs(sandboxExecRoot, environment);
 
     SandboxInputs inputs =
-        helpers.processInputFiles(
-            context.getInputMapping(PathFragment.EMPTY_FRAGMENT),
-            execRoot);
+        helpers.processInputFiles(context.getInputMapping(PathFragment.EMPTY_FRAGMENT), execRoot);
     SandboxOutputs outputs = helpers.getOutputs(spawn);
 
     Duration timeout = context.getTimeout();
@@ -439,22 +439,39 @@ private void checkForConcurrentModifications(SpawnExecutionContext context)
       throws IOException, ForbiddenActionInputException {
     for (ActionInput input : context.getInputMapping(PathFragment.EMPTY_FRAGMENT).values()) {
       if (input instanceof VirtualActionInput) {
+        // Virtual inputs are not existing in file system and can't be tampered with via sandbox. No
+        // need to check them.
         continue;
       }
 
       FileArtifactValue metadata = context.getMetadataProvider().getMetadata(input);
-      Path path = execRoot.getRelative(input.getExecPath());
+      if (!metadata.getType().isFile()) {
+        // The hermetic sandbox creates hardlinks from files inside sandbox to files outside
+        // sandbox. The content of the files outside the sandbox could have been tampered with via
+        // the hardlinks. Therefore files are checked for modifications. But directories and
+        // unresolved symlinks are not represented as hardlinks in sandbox and don't need to be
+        // checked. By continue and not checking them, we avoid UnsupportedOperationException and
+        // IllegalStateException.
+        continue;
+      }
 
+      Path path = execRoot.getRelative(input.getExecPath());
       try {
         if (wasModifiedSinceDigest(metadata.getContentsProxy(), path)) {
           throw new IOException("input dependency " + path + " was modified during execution.");
         }
       } catch (UnsupportedOperationException e) {
-        throw new IOException(
-            "input dependency "
-                + path
-                + " could not be checked for modifications during execution.",
-            e);
+        // Some FileArtifactValue implementations are ignored safely and silently already by the
+        // isFile check above. The remaining ones should probably be checked, but some are not
+        // supporting necessary operations.
+        if (warnedAboutUnsupportedModificationCheck.compareAndSet(false, true)) {
+          reporter.handle(
+              Event.warn(
+                  String.format(
+                      "Input dependency %s of type %s could not be checked for modifications during"
+                          + " execution. Suppressing similar warnings.",
+                      path, metadata.getClass().getSimpleName())));
+        }
       }
     }
   }

src/test/shell/bazel/bazel_hermetic_sandboxing_test.sh

@@ -95,6 +95,37 @@ import import_module
 EOF
 
   cat << 'EOF' > examples/hermetic/BUILD
+
+load(
+  "test.bzl",
+  "overwrite_via_symlink",
+  "overwrite_file_from_declared_directory",
+  "subdirectories_in_declared_directory",
+  "other_artifacts",
+)
+
+overwrite_via_symlink(
+  name = "overwrite_via_resolved_symlink",
+  resolve_symlink = True
+)
+
+overwrite_via_symlink(
+  name = "overwrite_via_unresolved_symlink",
+  resolve_symlink = False
+)
+
+overwrite_file_from_declared_directory(
+  name = "overwrite_file_from_declared_directory"
+)
+
+subdirectories_in_declared_directory(
+  name = "subdirectories_in_declared_directory"
+)
+
+other_artifacts(
+  name = "other_artifacts"
+)
+
 genrule(
   name = "absolute_path",
   srcs = ["script_absolute_path.sh"], # unknown_file.txt not referenced.
@@ -129,6 +160,141 @@ genrule(
          (echo overwrite text > $(location :input_file)) && \
          (echo success > $@)) || (echo fail > $@)",
 )
+EOF
+
+  cat << 'EOF' > examples/hermetic/test.bzl
+
+def _overwrite_via_symlink_impl(ctx):
+    file = ctx.actions.declare_file(ctx.attr.name + ".file")
+    if ctx.attr.resolve_symlink:
+        symlink = ctx.actions.declare_file(ctx.attr.name + ".symlink")
+    else:
+        symlink = ctx.actions.declare_symlink(ctx.attr.name + ".symlink")
+
+    ctx.actions.write(file, "")
+
+    if ctx.attr.resolve_symlink:
+        ctx.actions.symlink(
+            output = symlink,
+            target_file = file
+        )
+        # Symlink become resolved to RegularFileArtifactValue.
+        needed_inputs = [symlink]
+    else:
+        ctx.actions.symlink(
+            output = symlink,
+            target_path = file.basename
+        )
+        # Symlink become UnresolvedSymlinkArtifactValue and would be
+        # dangling unless also providing the actual file as input to sandbox.
+        needed_inputs = [symlink, file]
+
+    result_file = ctx.actions.declare_file(ctx.attr.name + ".result")
+
+    # Try invalid write to the input file via the symlink
+    ctx.actions.run_shell(
+       command = "chmod u+w $1 && echo hello >> $1 && ls -lR > $2",
+       arguments = [symlink.path, result_file.path],
+       inputs = needed_inputs,
+       outputs = [result_file],
+    )
+
+    return [DefaultInfo(files = depset([result_file]))]
+
+overwrite_via_symlink = rule(
+    attrs = {
+        "resolve_symlink" : attr.bool(),
+    },
+    implementation = _overwrite_via_symlink_impl,
+)
+
+
+def _overwrite_file_from_declared_directory_impl(ctx):
+    dir = ctx.actions.declare_directory(ctx.attr.name + ".dir")
+
+    ctx.actions.run_shell(
+        command = "mkdir -p $1/subdir && touch $1/subdir/file",
+        arguments = [dir.path],
+        outputs = [dir],
+    )
+
+    # Try invalid write to input file, with file as implicit input
+    # from declared directory.
+    result_file = ctx.actions.declare_file(ctx.attr.name + ".result")
+    ctx.actions.run_shell(
+       command = "chmod -R u+w $1 && echo hello >> $1/subdir/file && touch $2",
+       arguments = [dir.path, result_file.path],
+       inputs = [dir],
+       outputs = [result_file],
+    )
+
+    return [DefaultInfo(files = depset([result_file]))]
+
+overwrite_file_from_declared_directory = rule(
+    implementation = _overwrite_file_from_declared_directory_impl,
+)
+
+
+def _subdirectories_in_declared_directory_impl(ctx):
+    dir = ctx.actions.declare_directory(ctx.attr.name + ".dir")
+
+    ctx.actions.run_shell(
+        command = "mkdir -p %s/subdir1/subdir2" % dir.path,
+        outputs = [dir],
+    )
+
+    result_file = ctx.actions.declare_file(ctx.attr.name + ".result")
+    ctx.actions.run_shell(
+        command = "ls -lRH %s > %s" % (dir.path, result_file.path),
+        inputs = [dir],
+        outputs = [result_file],
+    )
+
+    return [DefaultInfo(files = depset([result_file]))]
+
+subdirectories_in_declared_directory = rule(
+    implementation = _subdirectories_in_declared_directory_impl,
+)
+
+
+def _other_artifacts_impl(ctx):
+
+    # Produce artifacts of other types
+
+    regular_file_artifact = ctx.actions.declare_file(ctx.attr.name + ".regular_file_artifact")
+    directory_artifact = ctx.actions.declare_file(ctx.attr.name + ".directory_artifact")
+    tree_artifact = ctx.actions.declare_directory(ctx.attr.name + ".tree_artifact")
+    unresolved_symlink_artifact = ctx.actions.declare_symlink(ctx.attr.name + ".unresolved_symlink_artifact")
+
+    ctx.actions.run_shell(
+        command = "touch %s && mkdir %s" % (regular_file_artifact.path, directory_artifact.path),
+        outputs = [regular_file_artifact, tree_artifact, directory_artifact],
+    )
+
+    ctx.actions.symlink(
+        output = unresolved_symlink_artifact,
+        target_path="dangling"
+    )
+
+    # Test other artifact types as input to hermetic sandbox.
+
+    all_artifacts = [regular_file_artifact,
+                     directory_artifact,
+                     tree_artifact,
+                     unresolved_symlink_artifact]
+    input_paths_string = " ".join([a.path for a in all_artifacts])
+    result_file = ctx.actions.declare_file(ctx.attr.name + ".result")
+    ctx.actions.run_shell(
+        command = "ls -lR %s > %s" % (input_paths_string, result_file.path),
+        inputs = all_artifacts,
+        outputs = [result_file],
+    )
+
+    return [DefaultInfo(files = depset([result_file]))]
+
+other_artifacts = rule(
+    implementation = _other_artifacts_impl,
+)
 EOF
 }
 
@@ -141,17 +307,13 @@ function test_absolute_path() {
 
 # Test that the build can't escape the sandbox by resolving symbolic link.
 function test_symbolic_link() {
-  [ "$PLATFORM" != "darwin" ] || return 0
-
   bazel build examples/hermetic:symbolic_link &> $TEST_log \
     && fail "Fail due to non hermetic sandbox: examples/hermetic:symbolic_link" || true
   expect_log "cat: \/execroot\/main\/examples\/hermetic\/unknown_file.txt: No such file or directory"
 }
 
 # Test that the sandbox discover if the bazel python rule miss dependencies.
 function test_missing_python_deps() {
-  [ "$PLATFORM" != "darwin" ] || return 0
-
   bazel test examples/hermetic:py_module_test --test_output=all &> $TEST_TMPDIR/log \
     && fail "Fail due to non hermetic sandbox: examples/hermetic:py_module_test" || true
 
@@ -160,7 +322,6 @@ function test_missing_python_deps() {
 
 # Test that the intermediate corrupt input file gets re:evaluated
 function test_writing_input_file() {
-  [ "$PLATFORM" != "darwin" ] || return 0
   # Write an input file, this should cause the hermetic sandbox to fail with an exception
   bazel build examples/hermetic:write_input_test &> $TEST_log  \
     && fail "Fail due to non hermetic sandbox: examples/hermetic:write_input_test" || true
@@ -177,7 +338,48 @@ function test_writing_input_file() {
   expect_log "original text input"
 }
 
+# Test that invalid write of input file is detected, when file is accessed via resolved symlink.
+function test_overwrite_via_resolved_symlink() {
+  bazel build examples/hermetic:overwrite_via_resolved_symlink &> $TEST_log  \
+    && fail "Hermetic sandbox did not detect invalid write to input file"
+  expect_log "input dependency .* was modified during execution."
+}
+
+# Test that invalid write of input file is detected, when file is accessed via unresolved symlink.
+function test_overwrite_via_unresolved_symlink() {
+  bazel build examples/hermetic:overwrite_via_unresolved_symlink &> $TEST_log  \
+    && fail "Hermetic sandbox did not detect invalid write to input file"
+  expect_log "input dependency .* was modified during execution."
+}
+
+# Test that invalid write of input file is detected, when file is found implicit via declared directory.
+function test_overwrite_file_from_declared_directory() {
+  bazel build examples/hermetic:overwrite_file_from_declared_directory &> $TEST_log  \
+    && fail "Hermetic sandbox did not detect invalid write to input file"
+  expect_log "input dependency .* was modified during execution."
+}
+
+# Test that the sandbox can handle deep directory trees from declared directory.
+function test_subdirectories_in_declared_directory() {
+  bazel build examples/hermetic:subdirectories_in_declared_directory &> $TEST_log
+  cat bazel-bin/examples/hermetic/subdirectories_in_declared_directory.result
+  assert_contains "dir/subdir1/subdir2" "bazel-bin/examples/hermetic/subdirectories_in_declared_directory.result"
+}
+
+# Test that the sandbox is not crashing and not producing warnings for various types of artifacts.
+# Regression test for Issue #15340
+function test_other_artifacts() {
+  bazel shutdown # Clear memory about duplicated warnings
+  bazel build examples/hermetic:other_artifacts &> $TEST_log
+  expect_not_log "WARNING"
+  assert_contains "regular_file_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  assert_contains "unresolved_symlink_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  assert_contains "directory_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+  assert_contains "tree_artifact" "bazel-bin/examples/hermetic/other_artifacts.result"
+}
+
 # The test shouldn't fail if the environment doesn't support running it.
 check_sandbox_allowed || exit 0
+[ "$PLATFORM" != "darwin" ] || exit 0
 
 run_suite "hermetic_sandbox"