Fix crash when `environ` contains duplicate entries
The values of the internal `$environ` attribute of repository rules is now deduplicated. `ActionEnvironmentFunction#getEnvironmentView`'s signature is updated to receive a `Set` to ensure deduplication for all callers.
Previously, a repository rule with duplicate values in `environ` could crash with:
```
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.RuntimeException: Unrecoverable error while evaluating node 'REPOSITORY_DIRECTORY:@foo' (requested by nodes 'IGNORED_PACKAGE_PREFIXES:@foo')
at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:633)
at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:365)
at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(Unknown Source)
at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.lang.IllegalArgumentException: Multiple entries with same key: FOO=bar and FOO=bar
at com.google.common.collect.ImmutableMap.conflictException(ImmutableMap.java:377)
at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:371)
at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:241)
at com.google.common.collect.RegularImmutableMap.fromEntryArrayCheckingBucketOverflow(RegularImmutableMap.java:132)
at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:94)
at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:573)
at com.google.common.collect.ImmutableMap$Builder.buildOrThrow(ImmutableMap.java:601)
at com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction.getEnvironmentView(ActionEnvironmentFunction.java:105)
at com.google.devtools.build.lib.rules.repository.RepositoryFunction.getEnvVarValues(RepositoryFunction.java:305)
...
```
Fixes #19244
Closes #19245.
PiperOrigin-RevId: 557156429
Change-Id: I454e88b1358cac9a9d787f295fdc9829c57860e4
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
index 7f56efc..9f2f9bb 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
@@ -153,7 +153,7 @@
ModuleExtension extension = (ModuleExtension) exported;
ImmutableMap<String, String> extensionEnvVars =
- RepositoryFunction.getEnvVarValues(env, extension.getEnvVariables());
+ RepositoryFunction.getEnvVarValues(env, ImmutableSet.copyOf(extension.getEnvVariables()));
if (extensionEnvVars == null) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
index 4f4307c..42d5880 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java
@@ -375,8 +375,8 @@
}
@SuppressWarnings("unchecked")
- private static Iterable<String> getEnviron(Rule rule) {
- return (Iterable<String>) rule.getAttr("$environ");
+ private static ImmutableSet<String> getEnviron(Rule rule) {
+ return ImmutableSet.copyOf((Iterable<String>) rule.getAttr("$environ"));
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
index d6bfd2d..934fa82 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidNdkRepositoryFunction.java
@@ -18,6 +18,7 @@
import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -71,7 +72,7 @@
private static final String PATH_ENV_VAR = "ANDROID_NDK_HOME";
private static final PathFragment PLATFORMS_DIR = PathFragment.create("platforms");
- private static final ImmutableList<String> PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR);
+ private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);
private static String getDefaultCrosstool(Integer majorRevision) {
// From NDK 17, libc++ replaces gnu-libstdc++ as the default STL.
@@ -262,7 +263,7 @@
if (attributes.isAttributeValueExplicitlySpecified("path")) {
return true;
}
- return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_LIST);
+ return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
}
@Override
@@ -276,7 +277,7 @@
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
Map<String, String> environ =
- declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
+ declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
index 5875814..e52dbb6 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java
@@ -19,6 +19,7 @@
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -163,7 +164,7 @@
private static final PathFragment SYSTEM_IMAGES_DIR = PathFragment.create("system-images");
private static final AndroidRevision MIN_BUILD_TOOLS_REVISION = AndroidRevision.parse("30.0.0");
private static final String PATH_ENV_VAR = "ANDROID_HOME";
- private static final ImmutableList<String> PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR);
+ private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);
private static final ImmutableList<String> LOCAL_MAVEN_REPOSITORIES =
ImmutableList.of(
"extras/android/m2repository", "extras/google/m2repository", "extras/m2repository");
@@ -180,7 +181,7 @@
if (attributes.isAttributeValueExplicitlySpecified("path")) {
return true;
}
- return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_LIST);
+ return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
}
@Override
@@ -194,7 +195,7 @@
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
Map<String, String> environ =
- declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_LIST);
+ declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index 1a24d9b..e830637 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -19,8 +19,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -56,6 +56,7 @@
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Objects;
+import java.util.Set;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
@@ -190,11 +191,11 @@
throws InterruptedException, RepositoryFunctionException;
@SuppressWarnings("unchecked")
- private static Collection<String> getEnviron(Rule rule) {
+ private static ImmutableSet<String> getEnviron(Rule rule) {
if (rule.isAttrDefined("$environ", Type.STRING_LIST)) {
- return (Collection<String>) rule.getAttr("$environ");
+ return ImmutableSet.copyOf((Collection<String>) rule.getAttr("$environ"));
}
- return ImmutableList.of();
+ return ImmutableSet.of();
}
/**
@@ -306,7 +307,7 @@
*/
@Nullable
protected Map<String, String> declareEnvironmentDependencies(
- Map<String, String> markerData, Environment env, Iterable<String> keys)
+ Map<String, String> markerData, Environment env, Set<String> keys)
throws InterruptedException {
ImmutableMap<String, String> envDep = getEnvVarValues(env, keys);
if (envDep == null) {
@@ -318,7 +319,7 @@
}
@Nullable
- public static ImmutableMap<String, String> getEnvVarValues(Environment env, Iterable<String> keys)
+ public static ImmutableMap<String, String> getEnvVarValues(Environment env, Set<String> keys)
throws InterruptedException {
ImmutableMap<String, String> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
if (environ == null) {
@@ -347,7 +348,7 @@
* Environment)} function to verify the values for environment variables.
*/
protected boolean verifyEnvironMarkerData(
- Map<String, String> markerData, Environment env, Collection<String> keys)
+ Map<String, String> markerData, Environment env, Set<String> keys)
throws InterruptedException {
ImmutableMap<String, String> environ = ActionEnvironmentFunction.getEnvironmentView(env, keys);
if (env.valuesMissing()) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
index 2fdcfd5..dc4246e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
@@ -14,7 +14,8 @@
package com.google.devtools.build.lib.skyframe;
-import com.google.common.collect.ImmutableList;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
@@ -25,6 +26,7 @@
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.SkyframeLookupResult;
import java.util.Map;
+import java.util.Set;
import javax.annotation.Nullable;
/**
@@ -80,13 +82,9 @@
* if and only if some dependencies from Skyframe still need to be resolved.
*/
@Nullable
- public static ImmutableMap<String, String> getEnvironmentView(
- Environment env, Iterable<String> keys) throws InterruptedException {
- ImmutableList.Builder<SkyKey> skyframeKeysBuilder = ImmutableList.builder();
- for (String key : keys) {
- skyframeKeysBuilder.add(key(key));
- }
- ImmutableList<SkyKey> skyframeKeys = skyframeKeysBuilder.build();
+ public static ImmutableMap<String, String> getEnvironmentView(Environment env, Set<String> keys)
+ throws InterruptedException {
+ var skyframeKeys = keys.stream().map(ActionEnvironmentFunction::key).collect(toImmutableSet());
SkyframeLookupResult values = env.getValuesAndExceptions(skyframeKeys);
if (env.valuesMissing()) {
return null;
diff --git a/src/test/shell/bazel/starlark_repository_test.sh b/src/test/shell/bazel/starlark_repository_test.sh
index 138d20d..6fe7167 100755
--- a/src/test/shell/bazel/starlark_repository_test.sh
+++ b/src/test/shell/bazel/starlark_repository_test.sh
@@ -2386,4 +2386,26 @@
expect_log_n "hello world!" 1
}
+function test_duplicate_value_in_environ() {
+ cat >> WORKSPACE <<EOF
+load('//:def.bzl', 'repo')
+repo(name='foo')
+EOF
+
+ touch BUILD
+ cat > def.bzl <<'EOF'
+def _impl(repository_ctx):
+ repository_ctx.file("WORKSPACE")
+ repository_ctx.file("BUILD", """filegroup(name="bar",srcs=[])""")
+
+repo = repository_rule(
+ implementation=_impl,
+ environ=["FOO", "FOO"],
+)
+EOF
+
+ FOO=bar bazel build @foo//:bar >& $TEST_log \
+ || fail "Expected build to succeed"
+}
+
run_suite "local repository tests"
