Skip to content

Commit

Permalink
Fix a NPE in compatible target skipping
Browse files Browse the repository at this point in the history 
Under a complex set of conditions a NPE is triggered. It was caused by IncompatibleTargetChecker constructing a RuleConfiguredTarget without adding a VisibilityProviderImpl to it. All other locations where RuleConfiguredTarget is constructed add the provider, namely RuleConfiguredTargetBuilder.

When the set of conditions is met, code path checking the visibility retrieves the provider without checking if it's available.

The data in VisibilityProvider is duplicated with the data already in RuleConfiguredTarget. Fix by removing the provider and retrieving the data directly.

Add the regression test with description how it happens.

PiperOrigin-RevId: 479339612
Change-Id: I8e29a9882de87d884f53e104ae14d6a126a8096b
  • Loading branch information
@comius
comius authored and Copybara-Service committed Oct 6, 2022
1 parent dbcf260 commit 26be41a
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 9 deletions.
1 change: 0 additions & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
View file
Expand Up @@ -94,7 +94,6 @@ public RuleConfiguredTargetBuilder(RuleContext ruleContext) {
this.ruleContext = ruleContext;
// Avoid building validations in analysis tests (b/143988346)
add(LicensesProvider.class, LicensesProviderImpl.of(ruleContext));
add(VisibilityProvider.class, new VisibilityProviderImpl(ruleContext.getVisibility()));
}

/**
Expand Down
13 changes: 10 additions & 3 deletions ...n/java/com/google/devtools/build/lib/analysis/configuredtargets/FileConfiguredTarget.java
View file
Expand Up @@ -108,9 +108,16 @@ public String filePathForFileTypeMatcher() {
}

@Override
public <P extends TransitiveInfoProvider> P getProvider(Class<P> provider) {
AnalysisUtils.checkProvider(provider);
return providers.getProvider(provider);
public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
AnalysisUtils.checkProvider(providerClass);
final P provider = providers.getProvider(providerClass);
if (provider != null) {
return provider;
} else if (providerClass.isAssignableFrom(getClass())) {
return providerClass.cast(this);
} else {
return null;
}
}

@Override
Expand Down
14 changes: 10 additions & 4 deletions ...java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java
View file
Expand Up @@ -73,11 +73,17 @@ public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass)
AnalysisUtils.checkProvider(providerClass);

P provider = nonBaseProviders.getProvider(providerClass);
if (provider == null) {
provider = base.getProvider(providerClass);
if (provider != null) {
return provider;
}

return provider;
provider = base.getProvider(providerClass);
if (provider != null) {
return provider;
}
if (providerClass.isAssignableFrom(getClass())) {
return providerClass.cast(this);
}
return null;
}

@Override
Expand Down
11 changes: 10 additions & 1 deletion ...n/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java
View file
Expand Up @@ -21,6 +21,7 @@
import com.google.common.collect.Interner;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.analysis.AnalysisUtils;
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
Expand Down Expand Up @@ -198,7 +199,15 @@ public String getRuleClassString() {
public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
// TODO(bazel-team): Should aspects be allowed to override providers on the configured target
// class?
return providers.getProvider(providerClass);
AnalysisUtils.checkProvider(providerClass);
final P provider = providers.getProvider(providerClass);
if (provider != null) {
return provider;
}
if (providerClass.isAssignableFrom(getClass())) {
return providerClass.cast(this);
}
return null;
}

@Override
Expand Down
226 changes: 226 additions & 0 deletions src/test/java/com/google/devtools/build/lib/skyframe/ToolchainsForTargetsTest.java
View file
Expand Up @@ -21,8 +21,11 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.eventbus.EventBus;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
Expand All @@ -42,6 +45,7 @@
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -517,6 +521,228 @@ public void toolchainWithDifferentExecutionPlatforms_doesNotGenerateConflictingC
assertThat(toolchainBContext).hasResolvedToolchain("//toolchains:toolchain_1_impl");
}

@CanIgnoreReturnValue
private AnalysisResult updateExplicitTarget(String label) throws Exception {
return update(
new EventBus(),
defaultFlags().with(Flag.KEEP_GOING),
/*explicitTargetPatterns=*/ ImmutableSet.of(Label.parseAbsoluteUnchecked(label)),
/*aspects=*/ ImmutableList.of(),
/*aspectsParameters=*/ ImmutableMap.of(),
label);
}

@Test
public void targetCompatibleWith_matchesExecCompatibleWith() throws Exception {
scratch.file(
"platforms/BUILD",
"constraint_setting(name = 'local_setting')",
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
"platform(name = 'local_platform_a',",
" constraint_values = [':local_value_a'],",
")",
"platform(name = 'local_platform_b',",
" constraint_values = [':local_value_b'],",
")");
useConfiguration(
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b");
scratch.file(
"foo/BUILD",
"sh_binary(name = 'tool', srcs = ['a.sh'], ",
" target_compatible_with = ['//platforms:local_value_b'])",
"genrule(name = 'runtool', tools = [':tool'], cmd = '', outs = ['b.txt'],",
" exec_compatible_with = ['//platforms:local_value_b'])");

AnalysisResult result = updateExplicitTarget("//foo:runtool");

assertThat(result.hasError()).isFalse();
assertNoEvents();
}

@Test
public void targetCompatibleWith_mismatchesExecCompatibleWith() throws Exception {
scratch.file(
"platforms/BUILD",
"constraint_setting(name = 'local_setting')",
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
"platform(name = 'local_platform_a',",
" constraint_values = [':local_value_a'],",
")",
"platform(name = 'local_platform_b',",
" constraint_values = [':local_value_b'],",
")");
useConfiguration(
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b");
scratch.file(
"foo/BUILD",
"sh_binary(name = 'tool', srcs = ['a.sh'], ",
" target_compatible_with = ['//platforms:local_value_a'])",
"genrule(name = 'runtool', tools = [':tool'], cmd = '', outs = ['b.txt'],",
" exec_compatible_with = ['//platforms:local_value_b'])");

reporter.removeHandler(failFastHandler);
AnalysisResult result = updateExplicitTarget("//foo:runtool");

assertThat(result.hasError()).isTrue();
assertContainsEvent(
"Target //foo:runtool is incompatible and cannot be built, but was explicitly requested");
}

@Test
public void targetCompatibleWith_mismatchesExecCompatibleInDifferentPackage() throws Exception {
// Regression test for a case where incompatibility happens in an aspect tool in a different
// package over a Starlark target with
// --incompatible_visibility_private_attributes_at_definition
// The tool is replaced with a fake target {@link
// IncommpatibleTargetChecker#createIncompatibleRuleConfiguredTarget)
// and it's verified if the tool is visible to the Starlark target.
scratch.file(
"platforms/BUILD",
"constraint_setting(name = 'local_setting')",
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
"platform(name = 'local_platform_a',",
" constraint_values = [':local_value_a'],",
")",
"platform(name = 'local_platform_b',",
" constraint_values = [':local_value_b'],",
")");

useConfiguration(
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b",
"--incompatible_visibility_private_attributes_at_definition");
scratch.file(
"foo/lib.bzl",
"def _impl(ctx):",
" pass",
"my_rule = rule(",
" _impl,",
" attrs = { '_my_tool': attr.label(default = '//tool') },",
")");
scratch.file(
"tool/BUILD",
"sh_binary(name = 'tool', srcs = ['a.cc'], ",
" target_compatible_with = ['//platforms:local_value_a'])");
scratch.file(
"foo/BUILD", //
"load(':lib.bzl','my_rule')",
"my_rule(name = 'target_in_different_package')");

reporter.removeHandler(failFastHandler);
AnalysisResult result = updateExplicitTarget("//foo:target_in_different_package");

assertThat(result.hasError()).isTrue();
assertContainsEvent(
"Target //foo:target_in_different_package is incompatible and cannot be built, but was"
+ " explicitly requested");
}

@Test
public void targetCompatibleWith_mismatchesExecCompatibleDepInDifferentPackage()
throws Exception {
scratch.file(
"platforms/BUILD",
"constraint_setting(name = 'local_setting')",
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
"platform(name = 'local_platform_a',",
" constraint_values = [':local_value_a'],",
")",
"platform(name = 'local_platform_b',",
" constraint_values = [':local_value_b'],",
")");

useConfiguration(
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b",
"--incompatible_visibility_private_attributes_at_definition");
scratch.file(
"foo/lib.bzl",
"def _impl(ctx):",
" pass",
"my_rule = rule(",
" _impl,",
" attrs = { '_my_tool': attr.label(default = '//tool') },",
")");
scratch.file(
"tool/BUILD",
"sh_binary(name = 'tool', srcs = ['a.cc'], ",
" target_compatible_with = ['//platforms:local_value_a'])");
scratch.file(
"foo/BUILD",
"load(':lib.bzl','my_rule')",
"my_rule(name = 'dep')",
"filegroup(name = 'target_with_dep', srcs = [':dep'])");

reporter.removeHandler(failFastHandler);
AnalysisResult result = updateExplicitTarget("//foo:target_with_dep");

assertThat(result.hasError()).isTrue();
assertContainsEvent(
"Target //foo:target_with_dep is incompatible and cannot be built, but was"
+ " explicitly requested");
}

@Test
public void targetCompatibleWith_mismatchesExecCompatibleWithinAspect() throws Exception {
// Regression test for a case where incompatibility happens in an aspect tool in a different
// package over a Starlark target with
// --incompatible_visibility_private_attributes_at_definition
// The tool is replaced with a fake target {@link
// IncommpatibleTargetChecker#createIncompatibleRuleConfiguredTarget)
// and it's verified if the tool is visible to the Starlark target the aspect is over.
scratch.file(
"platforms/BUILD",
"constraint_setting(name = 'local_setting')",
"constraint_value(name = 'local_value_a', constraint_setting = ':local_setting')",
"constraint_value(name = 'local_value_b', constraint_setting = ':local_setting')",
"platform(name = 'local_platform_a',",
" constraint_values = [':local_value_a'],",
")",
"platform(name = 'local_platform_b',",
" constraint_values = [':local_value_b'],",
")");

useConfiguration(
"--extra_execution_platforms=//platforms:local_platform_a,//platforms:local_platform_b",
"--incompatible_visibility_private_attributes_at_definition");
scratch.file(
"foo/lib.bzl",
"def _impl_aspect(ctx, target):",
" return []",
"my_aspect = aspect(",
" _impl_aspect,",
" attrs = { '_my_tool': attr.label(default = '//tool') },",
" exec_compatible_with = ['//platforms:local_value_a'],",
")",
"def _impl(ctx):",
" pass",
"my_rule = rule(",
" _impl,",
" attrs = { 'deps': attr.label_list(aspects = [my_aspect]) },",
")",
"simple_starlark_rule = rule(",
" _impl,",
")");
scratch.file(
"tool/BUILD",
"sh_binary(name = 'tool', srcs = ['a.cc'], ",
" target_compatible_with = ['//platforms:local_value_b'])");
scratch.file(
"foo/BUILD",
"load(':lib.bzl','my_rule', 'simple_starlark_rule')",
"simple_starlark_rule(name = 'simple_dep')",
"my_rule(name = 'target_with_aspect', deps = [':simple_dep'])");

reporter.removeHandler(failFastHandler);
AnalysisResult result = updateExplicitTarget("//foo:target_with_aspect");

// TODO(bazel-team): This should report an error similarly to {@code
// #targetCompatibleWith_mismatchesExecCompatibleDepInDifferentPackage}
assertThat(result.hasError()).isFalse();
}

private void assertHasBaselineCoverageAction(String label, String progressMessage)
throws InterruptedException {
Action coverageAction = Iterables.getOnlyElement(getActions(label));
Expand Down

0 comments on commit 26be41a

Please sign in to comment.