Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config_setting visibility not enforced #12669

Closed
ulrfa opened this issue Dec 9, 2020 · 7 comments
Closed

config_setting visibility not enforced #12669

ulrfa opened this issue Dec 9, 2020 · 7 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug

Comments

@ulrfa
Copy link
Contributor

ulrfa commented Dec 9, 2020

Description of the problem / bug:

It should not be allowed to access a private config_setting from select in other package. But bazel fails to enforce the visibility.

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

pkgA/BUILD:

load(
    "@bazel_skylib//rules:common_settings.bzl",
    "bool_flag",
)

bool_flag(
    name = "myFlag",
    build_setting_default = False,
    # This visibility is working, however the config_setting is in same package.
    visibility = ["//visibility:private"]
)

config_setting(
    name = "mySetting",
    flag_values = {"myFlag": "True"},
    # This can be accessed from pkgB, despite private visibility
    visibility = ["//visibility:private"]
)

pkgB/BUILD:

genrule(
    name = "foo",
    srcs = [],
    outs = ["foo.out"],
    # Can access mySetting from another package, despite private visibility. 
    cmd = select({
        "//pkgA:mySetting": "echo running with mySetting > $@",
        "//conditions:default": "echo running default > $@"}),
)

I expect both of the following commands to fail, due to pkgB accessing private config_setting in pkgA, but no error is detected:

bbi_bazel build //pkgB:foo --//pkgA:myFlag=True
bbi_bazel build //pkgB:foo --//pkgA:myFlag=False

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

3.7.0

Have you found anything relevant by searching the web?

I searched, but have not found anything.

@aiuto aiuto added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Jan 7, 2021
@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) type: bug and removed untriaged labels Jan 15, 2021
@gregestren
Copy link
Contributor

Thanks for this report. This is easily reproducible.

I believe the solution is to:

  1. Include config_setting in
    validateDirectPrerequisite(attribute, configuredTarget);
  2. so its visibility is checked at
  3. This requires accessing config_setting from RuleContext.Builder as a ConfiguredTarget
  4. RuleContext.Builder stores its config_settings as ConfigMatchingProviders:
    ImmutableMap<Label, ConfigMatchingProvider> configConditions,
  5. These are populated from
    static ImmutableMap<Label, ConfigMatchingProvider> getConfigConditions(
    which retrieves the ConfiguredTargets but only preserves the providers.
  6. So we'd need to modify that code to keep access to the entire ConfiguredTarget.

Actually, another approach could be to do visibility checking directly in step 5. It's not ideal, since it's carving out yet another special code path for config_setting vs. other dependencies. But it'd be a lot less intrusive of a change.

@lisaonduty
Copy link

Hi,

I work with @ulrfa so I have also seen this lack of visibility setting. But then last week a user got a strange error where the visibility did matter and we thought that it was very strange. It showed out that the user used config_setting_group in Skylib and then it didn't work with faulty visibility settings.

Just to show as information:

pkgA/BUILD

load(
    "@bazel_skylib//rules:common_settings.bzl",
    "bool_flag",
)

bool_flag(
    name = "myFlagA",
    build_setting_default = False,
    # This visibility is working, however the config_setting is in same package.
    visibility = ["//visibility:private"]
)

bool_flag(
    name = "myFlagB",
    build_setting_default = False,
    # This visibility is working, however the config_setting is in same package.
    visibility = ["//visibility:private"]
)

config_setting(
    name = "mySettingA",
    flag_values = {"myFlagA": "True"},
    # This can be accessed from pkgB, despite private visibility
    visibility = ["//visibility:private"]
)

config_setting(
    name = "mySettingB",
    flag_values = {"myFlagB": "True"},
    # This can be accessed from pkgB, despite private visibility
    visibility = ["//visibility:private"]
)

pkgB/BUILD

load("@bazel_skylib//lib:selects.bzl", "selects")

genrule(
    name = "foo",
    srcs = [],
    outs = ["foo.out"],
    # Can access mySetting from another package, despite private visibility.
    cmd = select({
        "//pkgB:mySettingAandB": "echo running with mySetting > $@",
        "//conditions:default": "echo running default > $@"}),
)

selects.config_setting_group(
    name = "mySettingAandB",
    match_all = [
        "//pkgA:mySettingA",
        "//pkgA:mySettingB",
    ],
)

Then it will not work to build due to the private visibility setting:

bazel build //pkgB:foo --//pkgA:myFlagA=False --//pkgA:myFlagB=False
ERROR: /pkgB/BUILD:13:29: in alias rule //pkgB:mySettingAandB: target '//pkgA:mySettingA' is not visible from target '//pkgB:mySettingAandB'. Check the visibility declaration of the former target if you think the dependency is legitimate

It probably don't impact the needed update in Bazel that you @gregestren suggest but I just thought that it was an interesting finding to share. Me and @ulrfa was really confused when we got the fault even though we previous had seen that visibility for config_setting didn't work.

@gregestren
Copy link
Contributor

Hi @lisaonduty.

I agree that's subtle. The difference is that for select({<someKey>: <someValue>}), anything in <someKey> doesn't get visibility-checked, while anything in <someValue> gets visibility-checked as a normal dependency.

config_setting_group applies some macro magic that we can unwind with a query:

$  bazel query --output=build //pkgB:all
alias(
  name = "mySettingAandB",
  generator_name = "mySettingAandB",
  generator_function = "_config_setting_group",
  generator_location = "pkgB/BUILD:14:29",
  actual = select({"//pkgA:mySettingA": "//pkgA:mySettingB", "//conditions:default": "//pkgA:mySettingA"}),
)

You can see mySettingAandB gets unrolled to an alias where the select's <someValue> includes the config_setting.

@lisaonduty
Copy link

Thanks @gregestren for the explanation and also the tip about query. I didn't thought of using the query in this use case.

@gregestren
Copy link
Contributor

Put up a proof of concept PR at #12877.

gregestren added a commit to gregestren/bazel that referenced this issue Jan 28, 2021
Make select() keys normal attribute -> configured target prerequisite
dependencies. This automatically opts them into RuleContext.Builder's standard
visibility checking.

The main historic argument against this is interference with "manual
trimming". But that's an on-ice feature that needs fundamental reevaluation and
shouldn't block other code in the meantime.

This is a simplified alternative to
bazelbuild#12877, for bazelbuild#12669.

PiperOrigin-RevId: 354326088
Change-Id: Ifaafc563f22d401599540ead70e49db6a86a1995
@gregestren
Copy link
Contributor

FYI: I rolled back my PR because it breaks some projects that already set visibility on config_setting with bad values.

I'm rolling it forward again behind a flag. Stay tuned...

@gregestren gregestren reopened this Jan 29, 2021
@gregestren gregestren added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jan 29, 2021
@gregestren
Copy link
Contributor

Filed bugs for --incompatible_* flags: #12932. PR using them incoming.

gregestren added a commit to gregestren/bazel that referenced this issue Jul 16, 2021
This was rolled back in bazelbuild@36d228b
 because of depot breakages.

This version adds incompatible flags to safely introduce enforcement.
See bazelbuild#12932 and
bazelbuild#12933 for flag settings.

*** Original change description ***

Roll back bazelbuild#12877.

***

Fixes bazelbuild#12669.

RELNOTES: enforce config_setting visibility. See bazelbuild#12932 for details.
PiperOrigin-RevId: 354930807
katre pushed a commit that referenced this issue Jul 19, 2021
This was rolled back in 36d228b
 because of depot breakages.

This version adds incompatible flags to safely introduce enforcement.
See #12932 and
#12933 for flag settings.

*** Original change description ***

Roll back #12877.

***

Fixes #12669.

RELNOTES: enforce config_setting visibility. See #12932 for details.
PiperOrigin-RevId: 354930807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants