From 963de91de9d7720087be777113009647f1b5d1a0 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 15 Aug 2023 09:29:05 -0700 Subject: [PATCH] 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 --- .../bzlmod/SingleExtensionEvalFunction.java | 2 +- .../starlark/StarlarkRepositoryFunction.java | 4 ++-- .../android/AndroidNdkRepositoryFunction.java | 7 +++--- .../android/AndroidSdkRepositoryFunction.java | 7 +++--- .../rules/repository/RepositoryFunction.java | 15 +++++++------ .../skyframe/ActionEnvironmentFunction.java | 14 +++++------- .../shell/bazel/starlark_repository_test.sh | 22 +++++++++++++++++++ 7 files changed, 47 insertions(+), 24 deletions(-) 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 748735a88288e2..4d00ea10c18a3e 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 @@ -159,7 +159,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ModuleExtension extension = ((ModuleExtension) exported); ImmutableMap 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 140697445739bb..fc2c43d1a1b276 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 @@ -285,8 +285,8 @@ public RepositoryDirectoryValue.Builder fetch( } @SuppressWarnings("unchecked") - private static Iterable getEnviron(Rule rule) { - return (Iterable) rule.getAttr("$environ"); + private static ImmutableSet getEnviron(Rule rule) { + return ImmutableSet.copyOf((Iterable) 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 d6bfd2d4d0f420..934fa82e1b4ccd 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 @@ public class AndroidNdkRepositoryFunction extends AndroidRepositoryFunction { private static final String PATH_ENV_VAR = "ANDROID_NDK_HOME"; private static final PathFragment PLATFORMS_DIR = PathFragment.create("platforms"); - private static final ImmutableList PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR); + private static final ImmutableSet 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 @@ public boolean verifyMarkerData(Rule rule, Map markerData, Envir 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 @@ public RepositoryDirectoryValue.Builder fetch( SkyKey key) throws InterruptedException, RepositoryFunctionException { Map 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 5875814a248c8e..e52dbb6303f011 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 @@ public String toString() { 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 PATH_ENV_VAR_AS_LIST = ImmutableList.of(PATH_ENV_VAR); + private static final ImmutableSet PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR); private static final ImmutableList LOCAL_MAVEN_REPOSITORIES = ImmutableList.of( "extras/android/m2repository", "extras/google/m2repository", "extras/m2repository"); @@ -180,7 +181,7 @@ public boolean verifyMarkerData(Rule rule, Map markerData, Envir 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 @@ public RepositoryDirectoryValue.Builder fetch( SkyKey key) throws RepositoryFunctionException, InterruptedException { Map 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 26d04ae28c2288..2ac13825d39ce8 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; @@ -55,6 +55,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; @@ -172,11 +173,11 @@ public abstract RepositoryDirectoryValue.Builder fetch( throws InterruptedException, RepositoryFunctionException; @SuppressWarnings("unchecked") - private static Collection getEnviron(Rule rule) { + private static ImmutableSet getEnviron(Rule rule) { if (rule.isAttrDefined("$environ", Type.STRING_LIST)) { - return (Collection) rule.getAttr("$environ"); + return ImmutableSet.copyOf((Collection) rule.getAttr("$environ")); } - return ImmutableList.of(); + return ImmutableSet.of(); } /** @@ -288,7 +289,7 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env) */ @Nullable protected Map declareEnvironmentDependencies( - Map markerData, Environment env, Iterable keys) + Map markerData, Environment env, Set keys) throws InterruptedException { ImmutableMap envDep = getEnvVarValues(env, keys); if (envDep == null) { @@ -300,7 +301,7 @@ protected Map declareEnvironmentDependencies( } @Nullable - public static ImmutableMap getEnvVarValues(Environment env, Iterable keys) + public static ImmutableMap getEnvVarValues(Environment env, Set keys) throws InterruptedException { ImmutableMap environ = ActionEnvironmentFunction.getEnvironmentView(env, keys); if (environ == null) { @@ -329,7 +330,7 @@ public static ImmutableMap getEnvVarValues(Environment env, Iter * Environment)} function to verify the values for environment variables. */ protected boolean verifyEnvironMarkerData( - Map markerData, Environment env, Collection keys) + Map markerData, Environment env, Set keys) throws InterruptedException { ImmutableMap 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 4edf0fb1b31078..2a8cd3f8c80963 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.common.collect.Interner; import com.google.devtools.build.lib.bugreport.BugReport; @@ -27,6 +28,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; /** @@ -77,13 +79,9 @@ public SkyFunctionName functionName() { * if and only if some dependencies from Skyframe still need to be resolved. */ @Nullable - public static ImmutableMap getEnvironmentView( - Environment env, Iterable keys) throws InterruptedException { - ImmutableList.Builder skyframeKeysBuilder = ImmutableList.builder(); - for (String key : keys) { - skyframeKeysBuilder.add(key(key)); - } - ImmutableList skyframeKeys = skyframeKeysBuilder.build(); + public static ImmutableMap getEnvironmentView(Environment env, Set 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 5569735b30d612..4ded3acfc5bb0d 100755 --- a/src/test/shell/bazel/starlark_repository_test.sh +++ b/src/test/shell/bazel/starlark_repository_test.sh @@ -2356,4 +2356,26 @@ EOF bazel build --experimental_repository_disable_download //:it || fail "Failed to build" } +function test_duplicate_value_in_environ() { + cat >> WORKSPACE < 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"