Report error instead of crash if test uses reserved env variable

This fixes bazelbuild#16094 Previously Bazel will crash if test uses reserved env variable (like `TEST_NAME`). Let's report an error in this case.
blindpirate · Aug 15, 2022 · 40d263c · 40d263c
1 parent 61c433e
commit 40d263c
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 15 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java
@@ -83,6 +83,13 @@
 /** Runs TestRunnerAction actions. */
 // TODO(bazel-team): add tests for this strategy.
 public class StandaloneTestStrategy extends TestStrategy {
+  private static final String TEST_NAME_ENV = "TEST_NAME";
+  private static final String TEST_SHARD_INDEX_ENV = "TEST_SHARD_INDEX";
+  private static final String TEST_TOTAL_SHARDS_ENV = "TEST_TOTAL_SHARDS";
+  private static final String IS_COVERAGE_SPAWN_ENV = "IS_COVERAGE_SPAWN";
+  private static final ImmutableSet<String> RESERVED_ENVS =
+      ImmutableSet.of(TEST_NAME_ENV, TEST_SHARD_INDEX_ENV, TEST_TOTAL_SHARDS_ENV, IS_COVERAGE_SPAWN_ENV);
+
   private static final ImmutableMap<String, String> ENV_VARS =
       ImmutableMap.<String, String>builder()
           .put("TZ", "UTC")
@@ -105,24 +112,34 @@ public StandaloneTestStrategy(
     this.tmpDirRoot = tmpDirRoot;
   }
 
+  private TestExecException createTestExecException(String errorMessage) {
+    return new TestExecException(
+        errorMessage,
+        FailureDetail.newBuilder()
+            .setTestAction(
+                TestAction.newBuilder().setCode(TestAction.Code.LOCAL_TEST_PREREQ_UNMET))
+            .setMessage(errorMessage)
+            .build()
+    );
+  }
+
   @Override
   public TestRunnerSpawn createTestRunnerSpawn(
       TestRunnerAction action, ActionExecutionContext actionExecutionContext)
       throws ExecException, InterruptedException {
     if (action.getExecutionSettings().getInputManifest() == null) {
-      String errorMessage = "cannot run local tests with --nobuild_runfile_manifests";
-      throw new TestExecException(
-          errorMessage,
-          FailureDetail.newBuilder()
-              .setTestAction(
-                  TestAction.newBuilder().setCode(TestAction.Code.LOCAL_TEST_PREREQ_UNMET))
-              .setMessage(errorMessage)
-              .build());
+      throw createTestExecException("cannot run local tests with --nobuild_runfile_manifests");
     }
     Map<String, String> testEnvironment =
         createEnvironment(
             actionExecutionContext, action, tmpDirRoot, executionOptions.splitXmlGeneration);
 
+    for (Map.Entry<String, String> entry: testEnvironment.entrySet()) {
+      if (RESERVED_ENVS.contains(entry.getKey())) {
+        throw createTestExecException(String.format("cannot set env variable %s=%s because the key is reserved", entry.getKey(), entry.getValue()));
+      }
+    }
+
     Map<String, String> executionInfo =
         new TreeMap<>(action.getTestProperties().getExecutionInfo());
     if (!action.shouldCacheResult()) {
@@ -582,12 +599,12 @@ private static Spawn createXmlGeneratingSpawn(
     // "PATH" and "TEST_BINARY" are also required, they should always be set in testEnv.
     Preconditions.checkArgument(testEnv.containsKey("PATH"));
     Preconditions.checkArgument(testEnv.containsKey("TEST_BINARY"));
-    envBuilder.putAll(testEnv).put("TEST_NAME", action.getTestName());
+    envBuilder.putAll(testEnv).put(TEST_NAME_ENV, action.getTestName());
     // testEnv only contains TEST_SHARD_INDEX and TEST_TOTAL_SHARDS if the test action is sharded,
     // we need to set the default value when the action isn't sharded.
     if (!action.isSharded()) {
-      envBuilder.put("TEST_SHARD_INDEX", "0");
-      envBuilder.put("TEST_TOTAL_SHARDS", "0");
+      envBuilder.put(TEST_SHARD_INDEX_ENV, "0");
+      envBuilder.put(TEST_TOTAL_SHARDS_ENV, "0");
     }
     Map<String, String> executionInfo =
         Maps.newHashMapWithExpectedSize(action.getExecutionInfo().size() + 1);
@@ -627,11 +644,11 @@ private static Spawn createCoveragePostProcessingSpawn(
     Map<String, String> testEnvironment =
         createEnvironment(actionExecutionContext, action, tmpDirRoot, splitXmlGeneration);
 
-    testEnvironment.put("TEST_SHARD_INDEX", Integer.toString(action.getShardNum()));
+    testEnvironment.put(TEST_SHARD_INDEX_ENV, Integer.toString(action.getShardNum()));
     testEnvironment.put(
-        "TEST_TOTAL_SHARDS", Integer.toString(action.getExecutionSettings().getTotalShards()));
-    testEnvironment.put("TEST_NAME", action.getTestName());
-    testEnvironment.put("IS_COVERAGE_SPAWN", "1");
+        TEST_TOTAL_SHARDS_ENV, Integer.toString(action.getExecutionSettings().getTotalShards()));
+    testEnvironment.put(TEST_NAME_ENV, action.getTestName());
+    testEnvironment.put(IS_COVERAGE_SPAWN_ENV, "1");
     return new SimpleSpawn(
         action,
         args,

diff --git a/src/test/shell/bazel/bazel_test_test.sh b/src/test/shell/bazel/bazel_test_test.sh
@@ -953,6 +953,24 @@ EOF
   expect_log "cannot run local tests with --nobuild_runfile_manifests"
 }
 
+function test_test_with_reserved_env_variable() {
+  mkdir -p dir
+
+  touch dir/test.sh
+  chmod u+x dir/test.sh
+  cat <<'EOF' > dir/BUILD
+sh_test(
+    name = 'test',
+    srcs = ['test.sh'],
+    env = {
+      "TEST_NAME": "foo"
+    },
+)
+EOF
+  bazel test //dir:test >& $TEST_log && fail "should have failed"
+  expect_log "cannot set env variable TEST_NAME=foo because the key is reserved"
+}
+
 function test_run_from_external_repo_sibling_repository_layout() {
   cat <<EOF > WORKSPACE
 local_repository(