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.

src/main/cpp/blaze.cc

@@ -2144,6 +2144,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());

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java

@@ -341,6 +341,7 @@ public void maybeReportSubcommand(Spawn spawn) {
             showSubcommands.prettyPrintArgs,
             spawn.getArguments(),
             spawn.getEnvironment(),
+            /* environmentVariablesToClear=*/ null,
             getExecRoot().getPathString(),
             spawn.getConfigurationChecksum(),
             spawn.getExecutionPlatformLabelString());

src/main/java/com/google/devtools/build/lib/query2/aquery/ActionGraphTextOutputFormatterCallback.java

@@ -281,6 +281,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

src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -106,6 +106,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
 /** Builds and run a target with the given command line arguments. */
@@ -145,6 +147,17 @@ public static class RunOptions extends OptionsBase {
 
   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"
+  );
+
   /** The test policy to determine the environment variables from when running tests */
   private final TestPolicy testPolicy;
 
@@ -211,6 +224,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
               /* prettyPrintArgs= */ false,
               runCommandLine.args,
               runCommandLine.runEnvironment,
+              ENV_VARIABLES_TO_CLEAR,
               runCommandLine.workingDir.getPathString(),
               builtTargets.configuration.checksum(),
               /* executionPlatformAsLabelString= */ null);
@@ -261,6 +275,7 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti
               runCommandLine.workingDir,
               runCommandLine.args,
               runEnv.buildOrThrow(),
+              ENV_VARIABLES_TO_CLEAR,
               builtTargets.configuration));
     } catch (RunCommandException e) {
       return e.result;
@@ -678,6 +693,7 @@ private static ExecRequest buildExecRequest(
       Path workingDir,
       ImmutableList<String> args,
       ImmutableSortedMap<String, String> runEnv,
+      ImmutableList<String> runEnvToClear,
       BuildConfigurationValue configuration)
       throws RunCommandException {
     ExecRequest.Builder execDescription =
@@ -729,6 +745,9 @@ private static ExecRequest buildExecRequest(
               .setValue(ByteString.copyFrom(variable.getValue(), ISO_8859_1))
               .build());
     }
+    execDescription.addAllEnvironmentVariableToClear(
+        runEnvToClear.stream().map(s -> ByteString.copyFrom(s, ISO_8859_1)).collect(
+            Collectors.toList()));
     return execDescription.build();
   }
 

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,7 @@ 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 +85,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 +131,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("UNSET ").append(name).append("\n  ");
+    }
+
     @Override
     public void describeCommandElement(
         StringBuilder message, String commandElement, boolean isBinary) {
@@ -156,6 +169,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 +219,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 +311,7 @@ static String describeCommandFailure(
             /* prettyPrintArgs= */ false,
             commandLineElements,
             env,
+            null,
             cwd,
             configurationChecksum,
             executionPlatformAsLabelString));

src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java

@@ -214,6 +214,7 @@ public String toString() {
         /*prettyPrintArgs=*/ false,
         args,
         env,
+        /*environmentVariablesToClear=*/ null,
         execRoot.getPathString(),
         /*configurationChecksum=*/ null,
         /*executionPlatformAsLabelString=*/ null);

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.

src/test/java/com/google/devtools/build/lib/util/CommandFailureUtilsTest.java

@@ -15,10 +15,12 @@
 
 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;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -210,6 +212,8 @@ public void describeCommandPrettyPrintArgs() throws Exception {
     env.put("FOO", "foo");
     env.put("PATH", "/usr/bin:/bin:/sbin");
 
+    List<String> envToClear = ImmutableList.of("CLEAR", "THIS");
+
     String cwd = "/my/working/directory";
     PlatformInfo executionPlatform =
         PlatformInfo.builder().setLabel(Label.parseCanonicalUnchecked("//platform:exec")).build();
@@ -219,6 +223,7 @@ public void describeCommandPrettyPrintArgs() throws Exception {
             true,
             Arrays.asList(args),
             env,
+            envToClear,
             cwd,
             "cfg12345",
             executionPlatform.label().toString());
@@ -227,6 +232,8 @@ public void describeCommandPrettyPrintArgs() throws Exception {
         .isEqualTo(
             "(cd /my/working/directory && \\\n"
                 + "  exec env - \\\n"
+                + "    --unset=CLEAR \\\n"
+                + "    --unset=THIS \\\n"
                 + "    FOO=foo \\\n"
                 + "    PATH=/usr/bin:/bin:/sbin \\\n"
                 + "  some_command \\\n"