From 4720e67e39d93fcae78788b820cbfabb6bf3cce0 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Tue, 14 Mar 2023 03:28:15 -0700
Subject: [PATCH] Clear runfiles environment variables for `bazel run`

When an environment variable such as `RUNFILES_DIR` is set in the client environment when a target using runfiles libraries is run via `bazel run`, the libraries can't look up the runfiles directory or manifest.

This is fixed by clearing the runfiles-related environment variables from the env in which the target is executed.

Fixes #17571

Closes #17690.

PiperOrigin-RevId: 516474822
Change-Id: Ia5201d4334b286b36ba2e476e850b98992ca0ffa
---
 src/main/cpp/blaze.cc                         |  6 +++
 .../lib/actions/ActionExecutionContext.java   |  1 +
 ...ctionGraphTextOutputFormatterCallback.java |  1 +
 .../lib/runtime/commands/RunCommand.java      | 23 ++++++++-
 .../build/lib/util/CommandFailureUtils.java   | 22 +++++++++
 .../devtools/build/lib/worker/WorkerKey.java  |  7 +--
 src/main/protobuf/command_server.proto        |  1 +
 .../lib/util/CommandFailureUtilsTest.java     |  6 +++
 src/test/shell/bazel/run_test.sh              | 47 +++++++++++++++++++
 9 files changed, 109 insertions(+), 5 deletions(-)

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index 459dd524c62dc8..1ef38f81344bbe 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -2151,6 +2151,12 @@ unsigned int BlazeServer::Communicate(
       return blaze_exit_code::INTERNAL_ERROR;
     }
 
+    // Clear environment variables before setting the requested ones so that
+    // users can still explicitly override the clearing.
+    for (const auto &variable_name : request.environment_variable_to_clear()) {
+      UnsetEnv(variable_name);
+    }
+
     vector<string> argv(request.argv().begin(), request.argv().end());
     for (const auto &variable : request.environment_variable()) {
       SetEnv(variable.name(), variable.value());
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
index ce283b1accbc75..ce3c12352611c2 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -339,6 +339,7 @@ public void maybeReportSubcommand(Spawn spawn) {
             showSubcommands.prettyPrintArgs,
             spawn.getArguments(),
             spawn.getEnvironment(),
+            /* environmentVariablesToClear= */ null,
             getExecRoot().getPathString(),
             spawn.getConfigurationChecksum(),
             spawn.getExecutionPlatformLabelString());
diff --git a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java
index 6e954351a0709c..b0c97f6e7e276d 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java
@@ -280,6 +280,7 @@ private void writeAction(ActionAnalysisMetadata action, PrintStream printStream)
                           .map(a -> escapeBytestringUtf8(a))
                           .collect(toImmutableList()),
                   /* environment= */ null,
+                  /* environmentVariablesToClear= */ null,
                   /* cwd= */ null,
                   action.getOwner().getConfigurationChecksum(),
                   action.getExecutionPlatform() == null
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 13069d4bd5aac0..045b641efc5501 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -15,6 +15,9 @@
 package com.google.devtools.build.lib.runtime.commands;
 
 import com.google.common.annotations.VisibleForTesting;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static java.nio.charset.StandardCharsets.ISO_8859_1;
+
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
@@ -147,7 +150,7 @@ private static final class NoShellFoundException extends Exception {}
 
   public static final String MULTIPLE_TESTS_MESSAGE =
       "'run' only works with tests with one shard ('--test_sharding_strategy=disabled' is okay) "
-      + "and without --runs_per_test";
+          + "and without --runs_per_test";
 
   // The test policy to determine the environment variables from when running tests
   private final TestPolicy testPolicy;
@@ -157,11 +160,22 @@ private static final class NoShellFoundException extends Exception {}
 
   private static final FileType RUNFILES_MANIFEST = FileType.of(".runfiles_manifest");
 
+  private static final ImmutableList<String> ENV_VARIABLES_TO_CLEAR =
+      ImmutableList.of(
+          // These variables are all used by runfiles libraries to locate the runfiles directory or
+          // manifest and can cause incorrect behavior when set for the top-level binary run with
+          // bazel run.
+          "JAVA_RUNFILES",
+          "RUNFILES_DIR",
+          "RUNFILES_MANIFEST_FILE",
+          "RUNFILES_MANIFEST_ONLY",
+          "TEST_SRCDIR");
+
   public RunCommand(TestPolicy testPolicy) {
     this.testPolicy = testPolicy;
   }
 
-  @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
+  @VisibleForTesting // productionVisibility = Visibility.PRIVATE
   protected BuildResult processRequest(final CommandEnvironment env, BuildRequest request) {
     List<String> targetPatternStrings = request.getTargets();
     return new BuildTool(env)
@@ -542,6 +556,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
               /* prettyPrintArgs= */ false,
               cmdLine,
               runEnvironment,
+              ENV_VARIABLES_TO_CLEAR,
               workingDir.getPathString(),
               configuration.checksum(),
               /* executionPlatformAsLabelString= */ null);
@@ -629,6 +644,10 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
           .setValue(ByteString.copyFrom(variable.getValue(), StandardCharsets.ISO_8859_1))
           .build());
     }
+    execDescription.addAllEnvironmentVariableToClear(
+        ENV_VARIABLES_TO_CLEAR.stream()
+            .map(s -> ByteString.copyFrom(s, ISO_8859_1))
+            .collect(toImmutableList()));
 
     return BlazeCommandResult.execute(execDescription.build());
   }
diff --git a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java
index 8e7a4a91383242..a3e7d15bd9c765 100644
--- a/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/util/CommandFailureUtils.java
@@ -22,6 +22,7 @@
 import java.io.File;
 import java.util.Collection;
 import java.util.Comparator;
+import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -41,6 +42,8 @@ private interface DescribeCommandImpl {
     void describeCommandCwd(String cwd, StringBuilder message);
     void describeCommandEnvPrefix(StringBuilder message, boolean isolated);
     void describeCommandEnvVar(StringBuilder message, Map.Entry<String, String> entry);
+
+    void describeCommandUnsetEnvVar(StringBuilder message, String name);
     /**
      * Formats the command element and adds it to the message.
      *
@@ -83,6 +86,12 @@ public void describeCommandEnvVar(StringBuilder message, Map.Entry<String, Strin
           .append(ShellEscaper.escapeString(entry.getValue())).append(" \\\n  ");
     }
 
+    @Override
+    public void describeCommandUnsetEnvVar(StringBuilder message, String name) {
+      // Only the short form of --unset is supported on macOS.
+      message.append("-u ").append(ShellEscaper.escapeString(name)).append(" \\\n  ");
+    }
+
     @Override
     public void describeCommandElement(
         StringBuilder message, String commandElement, boolean isBinary) {
@@ -123,6 +132,11 @@ public void describeCommandEnvVar(StringBuilder message, Map.Entry<String, Strin
           .append(entry.getValue()).append("\n  ");
     }
 
+    @Override
+    public void describeCommandUnsetEnvVar(StringBuilder message, String name) {
+      message.append("SET ").append(name).append('=').append("\n  ");
+    }
+
     @Override
     public void describeCommandElement(
         StringBuilder message, String commandElement, boolean isBinary) {
@@ -156,6 +170,7 @@ public static String describeCommand(
       boolean prettyPrintArgs,
       Collection<String> commandLineElements,
       @Nullable Map<String, String> environment,
+      @Nullable List<String> environmentVariablesToClear,
       @Nullable String cwd,
       @Nullable String configurationChecksum,
       @Nullable String executionPlatformAsLabelString) {
@@ -205,6 +220,12 @@ public static String describeCommand(
       if (environment != null) {
         describeCommandImpl.describeCommandEnvPrefix(
             message, form != CommandDescriptionForm.COMPLETE_UNISOLATED);
+        if (environmentVariablesToClear != null) {
+          for (String name : Ordering.natural().sortedCopy(environmentVariablesToClear)) {
+            message.append("  ");
+            describeCommandImpl.describeCommandUnsetEnvVar(message, name);
+          }
+        }
         // A map can never have two keys with the same value, so we only need to compare the keys.
         Comparator<Map.Entry<String, String>> mapEntryComparator = comparingByKey();
         for (Map.Entry<String, String> entry :
@@ -291,6 +312,7 @@ static String describeCommandFailure(
             /* prettyPrintArgs= */ false,
             commandLineElements,
             env,
+            null,
             cwd,
             configurationChecksum,
             executionPlatformAsLabelString));
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
index 2ffacc9824b522..3c781a8a2f2f89 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java
@@ -211,11 +211,12 @@ public String toString() {
     // debugging.
     return CommandFailureUtils.describeCommand(
         CommandDescriptionForm.COMPLETE,
-        /*prettyPrintArgs=*/ false,
+        /* prettyPrintArgs= */ false,
         args,
         env,
+        /* environmentVariablesToClear= */ null,
         execRoot.getPathString(),
-        /*configurationChecksum=*/ null,
-        /*executionPlatformAsLabelString=*/ null);
+        /* configurationChecksum= */ null,
+        /* executionPlatformAsLabelString= */ null);
   }
 }
diff --git a/src/main/protobuf/command_server.proto b/src/main/protobuf/command_server.proto
index 5a1155df007f99..e2987492327383 100644
--- a/src/main/protobuf/command_server.proto
+++ b/src/main/protobuf/command_server.proto
@@ -92,6 +92,7 @@ message ExecRequest {
   bytes working_directory = 1;
   repeated bytes argv = 2;
   repeated EnvironmentVariable environment_variable = 3;
+  repeated bytes environment_variable_to_clear = 4;
 }
 
 // Contains metadata and result data for a command execution.
diff --git a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java
index fdc9027f09891c..44b8badbb9c24b 100644
--- a/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java
@@ -15,6 +15,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.cmdline.Label;
 import java.util.Arrays;
@@ -210,6 +211,8 @@ public void describeCommandPrettyPrintArgs() throws Exception {
     env.put("FOO", "foo");
     env.put("PATH", "/usr/bin:/bin:/sbin");
 
+    ImmutableList<String> envToClear = ImmutableList.of("CLEAR", "THIS");
+
     String cwd = "/my/working/directory";
     PlatformInfo executionPlatform =
         PlatformInfo.builder().setLabel(Label.parseAbsoluteUnchecked("//platform:exec")).build();
@@ -219,6 +222,7 @@ public void describeCommandPrettyPrintArgs() throws Exception {
             true,
             Arrays.asList(args),
             env,
+            envToClear,
             cwd,
             "cfg12345",
             executionPlatform.label().toString());
@@ -227,6 +231,8 @@ public void describeCommandPrettyPrintArgs() throws Exception {
         .isEqualTo(
             "(cd /my/working/directory && \\\n"
                 + "  exec env - \\\n"
+                + "    -u CLEAR \\\n"
+                + "    -u THIS \\\n"
                 + "    FOO=foo \\\n"
                 + "    PATH=/usr/bin:/bin:/sbin \\\n"
                 + "  some_command \\\n"
diff --git a/src/test/shell/bazel/run_test.sh b/src/test/shell/bazel/run_test.sh
index fd6c3c0de5d53e..82a987446306a2 100755
--- a/src/test/shell/bazel/run_test.sh
+++ b/src/test/shell/bazel/run_test.sh
@@ -164,4 +164,51 @@ EOF
   true
 }
 
+function test_run_with_runfiles_env() {
+  mkdir -p b
+  cat > b/BUILD <<'EOF'
+sh_binary(
+  name = "binary",
+  srcs = ["binary.sh"],
+  deps = ["@bazel_tools//tools/bash/runfiles"],
+)
+EOF
+  cat > b/binary.sh <<'EOF'
+#!/usr/bin/env bash
+# --- begin runfiles.bash initialization v3 ---
+# Copy-pasted from the Bazel Bash runfiles library v3.
+set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$0.runfiles/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+# --- end runfiles.bash initialization v3 ---
+
+own_path=$(rlocation main/b/binary.sh)
+echo "own path: $own_path"
+test -f "$own_path"
+EOF
+  chmod +x b/binary.sh
+
+  bazel run //b:binary --script_path=script.bat &>"$TEST_log" \
+    || fail "Script generation should succeed"
+
+  cat ./script.bat &>"$TEST_log"
+
+  # Make it so that the runfiles variables point to an incorrect but valid
+  # runfiles directory/manifest, simulating a left over one from a different
+  # test to which RUNFILES_DIR and RUNFILES_MANIFEST_FILE point in the client
+  # env.
+  BOGUS_RUNFILES_DIR="$(pwd)/bogus_runfiles/bazel_tools/tools/bash/runfiles"
+  mkdir -p "$BOGUS_RUNFILES_DIR"
+  touch "$BOGUS_RUNFILES_DIR/runfiles.bash"
+  BOGUS_RUNFILES_MANIFEST_FILE="$(pwd)/bogus_manifest"
+  echo "bazel_tools/tools/bash/runfiles/runfiles.bash bogus/path" > "$BOGUS_RUNFILES_MANIFEST_FILE"
+
+  RUNFILES_DIR="$BOGUS_RUNFILES_DIR" RUNFILES_MANIFEST_FILE="$BOGUS_RUNFILES_MANIFEST_FILE" \
+     ./script.bat || fail "Run should succeed"
+}
+
 run_suite "run_under_tests"