Repository: bazelbuild/bazel
Issue: bazel run should not inherit RUNFILES_DIR from OS
Labels: type: bug
P2
team-ExternalDeps
help wanted

### Description of the bug:


Runfile libraries such as [github.com/bazelbuild/rules_go/go/runfiles](https://github.com/bazelbuild/rules_go/blob/948f9587d27821afacfb5fb632fda0eb28ffd2ff/go/runfiles/runfiles.go#L48) relies on the environment variables `RUNFILES_DIR` and `RUNFILES_MANIFEST_FILE` to work correctly. These two variables are set by Bazel during `bazel run`. However, if they are set in OS, Bazel would inherit them and not set them anymore, leading to machine dependent behaviors.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


1. Create a repo like this:

```
-- WORKSPACE --
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "io_bazel_rules_go",
    sha256 = "56d8c5a5c91e1af73eca71a6fab2ced959b67c86d12ba37feedb0a2dfea441a6",
    urls = [
        "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.37.0/rules_go-v0.37.0.zip",
        "https://github.com/bazelbuild/rules_go/releases/download/v0.37.0/rules_go-v0.37.0.zip",
    ],
)

load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

go_rules_dependencies()

go_register_toolchains(version = "1.19.3")
-- BUILD.bazel --
load("@io_bazel_rules_go//go:def.bzl", "go_binary")

go_binary(
    name = "runfiles",
    srcs = ["main.go"],
    data = ["@go_sdk//:tools"],
    deps = ["@io_bazel_rules_go//go/runfiles"],
)
-- main.go --
package main

import (
        "fmt"
        "log"

        "github.com/bazelbuild/rules_go/go/runfiles"
)

func main() {
        loc, err := runfiles.Rlocation("go_sdk/bin/gofmt")
        if err != nil {
                log.Fatal(err)
        }
        fmt.Println(loc)
}
```

2. Run:
```bash
RUNFILES_DIR=/tmp bazel run //:runfiles
```

It outputs `/tmp/go_sdk/bin/gofmt`, but Bazel should ignore `RUNFILES_DIR=/tmp`, and output something like `/private/var/tmp/_bazel_zplin/a20570cf42c983bd18597126a6b3c06e/external/go_sdk/bin/gofmt` instead.


### Which operating system are you running Bazel on?


macOS

### What is the output of `bazel info release`?


release 6.0.0-homebrew

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


_No response_

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


_No response_

### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
The list of variables potentially affecting runfiles libraries further includes `JAVA_RUNFILES`, `PYTHON_RUNFILES` and `RUNFILES_MANIFEST_ONLY`.

@meteorcloudy @Wyverald What do you think, could `bazel run` drop these variables from the environment? Not doing so potentially breaks the runfiles library "promise" to "just work" with `bazel run`.
In general, I think it makes sense, but I wonder under what circumstances, users would set those env vars before running `bazel run`?  And what if the user just runs the binary directly, it could still be a problem, right?
I'll defer to @linzhp for the first question.

Running a binary directly already has some notable differences from running it via `bazel run`, e.g. `args` an `env` aren't honored. I don't think users would expect everything to just work if they do that, but they should probably be able to assume that `bazel run` makes it so.
> users would set those env vars before running bazel run? 

This is just a way to reproduce the issue. In the real scenario, they set `RUNFILES_DIR` in their `$HOME/.zshrc` file when they were debugging something else in the past and forgot to remove it. When we enable Python Gazelle extension, which uses the runfiles library to locate Python parser, they found Gazelle no longer work for them.

> And what if the user just runs the binary directly, it could still be a problem, right?
In such scenario, it's users responsibility to set `RUNFILES_DIR` correctly, or pass the files via command line arguments.


I see, thanks for the context! I'm happy to review a PR to address this issue ;)
A fix for this issue has been included in [Bazel 6.4.0 RC2](https://releases.bazel.build/6.4.0/rc2/index.html). Please test out the release candidate and report any issues as soon as possible. Thanks!

Pull request review comments:

Patch that resolved the issue:
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.


Test patch:
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"