Remove cc_shared_library_permissions

oquenchil · copybara-github · commit adfd4dff7731 · 2023-02-17T07:22:28.000-08:00
This mechanism came out of the design doc for cc_shared_library but people aren't using it. The idea was to prevent a cc_library target to be exported by cc_shared_libraries that aren't authorized. In reality though this cannot work on two accounts:
1. The mechanism would only prevent a cc_library from being "claimed to be exported" by the cc_shared_library, however a custom Starlark rule would easily bypass this.
2. The rule cannot control the version script the user passes to the linker so the symbols could in practice still be exported.

In general, this mechanism provides a false sense of security, no one is using it and currently the only thing it does is to slightly complicate the codebase and the documentation. In any case adding this functionality later if needed would be a compatible change.

RELNOTES:none
PiperOrigin-RevId: 510425434
Change-Id: Icf85050b57b1d8dae0d32614dc5951d982b3b3d0
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -99,7 +99,6 @@
 import com.google.devtools.build.lib.rules.android.databinding.DataBindingV2Provider;
 import com.google.devtools.build.lib.rules.config.ConfigRules;
 import com.google.devtools.build.lib.rules.core.CoreRules;
-import com.google.devtools.build.lib.rules.cpp.CcSharedLibraryPermissionsRule;
 import com.google.devtools.build.lib.rules.cpp.CcSharedLibraryRule;
 import com.google.devtools.build.lib.rules.cpp.CcStarlarkInternal;
 import com.google.devtools.build.lib.rules.cpp.GoogleLegacyStubs;
@@ -507,7 +506,6 @@ public void init(ConfiguredRuleClassProvider.Builder builder) {
           builder.addRuleDefinition(new AndroidNdkRepositoryRule());
           builder.addRuleDefinition(new LocalConfigPlatformRule());
           builder.addRuleDefinition(new CcSharedLibraryRule());
-          builder.addRuleDefinition(new CcSharedLibraryPermissionsRule());
 
           try {
             builder.addWorkspaceFileSuffix(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSharedLibraryPermissionsRule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSharedLibraryPermissionsRule.java
diff --git a/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl b/src/main/starlark/builtins_bzl/common/cc/experimental_cc_shared_library.bzl
@@ -43,12 +43,6 @@ LINKABLE_MORE_THAN_ONCE = "LINKABLE_MORE_THAN_ONCE"
 # be dropped by the linker since there are no incoming symbol references.
 NO_EXPORTING = "NO_EXPORTING"
 
-CcSharedLibraryPermissionsInfo = provider(
-    "Permissions for a cc shared library.",
-    fields = {
-        "targets": "Matches targets that can be exported.",
-    },
-)
 GraphNodeInfo = provider(
     "Nodes in the graph of shared libraries.",
     fields = {
@@ -195,47 +189,18 @@ def _check_if_target_under_path(value, pattern):
 
     return pattern.package == value.package and pattern.name == value.name
 
-def _check_if_target_can_be_exported(target, current_label, permissions):
-    if permissions == None:
-        return True
-
-    if (target.workspace_name != current_label.workspace_name or
-        _same_package_or_above(current_label, target)):
-        return True
-
-    matched_by_target = False
-    for permission in permissions:
-        for permission_target in permission[CcSharedLibraryPermissionsInfo].targets:
-            if _check_if_target_under_path(target, permission_target):
-                return True
-
-    return False
-
-def _check_if_target_should_be_exported_without_filter(target, current_label, permissions):
-    return _check_if_target_should_be_exported_with_filter(target, current_label, None, permissions)
+def _check_if_target_should_be_exported_without_filter(target, current_label):
+    return _check_if_target_should_be_exported_with_filter(target, current_label, None)
 
-def _check_if_target_should_be_exported_with_filter(target, current_label, exports_filter, permissions):
+def _check_if_target_should_be_exported_with_filter(target, current_label, exports_filter):
     should_be_exported = False
     if exports_filter == None:
         should_be_exported = True
     else:
         for export_filter in exports_filter:
             export_filter_label = current_label.relative(export_filter)
             if _check_if_target_under_path(target, export_filter_label):
-                should_be_exported = True
-                break
-
-    if should_be_exported:
-        if _check_if_target_can_be_exported(target, current_label, permissions):
-            return True
-        else:
-            matched_by_filter_text = ""
-            if exports_filter:
-                matched_by_filter_text = " (matched by filter) "
-            fail(str(target) + matched_by_filter_text +
-                 " cannot be exported from " + str(current_label) +
-                 " because it's not in the same package/subpackage and the library " +
-                 "doesn't have the necessary permissions. Use cc_shared_library_permissions.")
+                return True
 
     return False
 
@@ -337,7 +302,6 @@ def _filter_inputs(
                 linker_input.owner,
                 ctx.label,
                 ctx.attr.exports_filter,
-                _get_permissions(ctx),
             ):
                 exports[owner] = True
 
@@ -389,11 +353,6 @@ def _same_package_or_above(label_a, label_b):
 
     return True
 
-def _get_permissions(ctx):
-    if ctx.fragments.cpp.experimental_enable_target_export_check():
-        return ctx.attr.permissions
-    return None
-
 def _get_deps(ctx):
     if len(ctx.attr.deps) and len(ctx.attr.roots):
         fail(
@@ -460,7 +419,7 @@ def _cc_shared_library_impl(ctx):
             fail("Trying to export a library already exported by a different shared library: " +
                  str(export.label))
 
-        _check_if_target_should_be_exported_without_filter(export.label, ctx.label, _get_permissions(ctx))
+        _check_if_target_should_be_exported_without_filter(export.label, ctx.label)
 
     link_once_static_libs_map = _build_link_once_static_libs_map(merged_cc_shared_library_info)
 
@@ -634,41 +593,20 @@ def _graph_structure_aspect_impl(target, ctx):
         no_exporting = no_exporting,
     )]
 
-def _cc_shared_library_permissions_impl(ctx):
-    targets = []
-    for target_filter in ctx.attr.targets:
-        target_filter_label = ctx.label.relative(target_filter)
-        if not _check_if_target_under_path(target_filter_label, ctx.label.relative(":__subpackages__")):
-            fail("A cc_shared_library_permissions rule can only list " +
-                 "targets that are in the same package or a sub-package")
-        targets.append(target_filter_label)
-
-    return [CcSharedLibraryPermissionsInfo(
-        targets = targets,
-    )]
-
 graph_structure_aspect = aspect(
     attr_aspects = ["*"],
     required_providers = [[CcInfo], [ProtoInfo]],
     required_aspect_providers = [[CcInfo]],
     implementation = _graph_structure_aspect_impl,
 )
 
-cc_shared_library_permissions = rule(
-    implementation = _cc_shared_library_permissions_impl,
-    attrs = {
-        "targets": attr.string_list(),
-    },
-)
-
 cc_shared_library = rule(
     implementation = _cc_shared_library_impl,
     attrs = {
         "additional_linker_inputs": attr.label_list(allow_files = True),
         "shared_lib_name": attr.string(),
         "dynamic_deps": attr.label_list(providers = [CcSharedLibraryInfo]),
         "exports_filter": attr.string_list(),
-        "permissions": attr.label_list(providers = [CcSharedLibraryPermissionsInfo]),
         "win_def_file": attr.label(allow_single_file = [".def"]),
         "roots": attr.label_list(providers = [CcInfo], aspects = [graph_structure_aspect]),
         "deps": attr.label_list(providers = [CcInfo], aspects = [graph_structure_aspect]),
diff --git a/src/main/starlark/builtins_bzl/common/exports.bzl b/src/main/starlark/builtins_bzl/common/exports.bzl
@@ -17,7 +17,7 @@
 load("@_builtins//:common/cc/cc_import.bzl", "cc_import")
 load("@_builtins//:common/cc/cc_binary_wrapper.bzl", "cc_binary")
 load("@_builtins//:common/cc/cc_test_wrapper.bzl", cc_test = "cc_test_wrapper")
-load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "cc_shared_library", "cc_shared_library_permissions")
+load("@_builtins//:common/cc/experimental_cc_shared_library.bzl", "CcSharedLibraryInfo", "cc_shared_library")
 load("@_builtins//:common/objc/objc_import.bzl", "objc_import")
 load("@_builtins//:common/objc/objc_library.bzl", "objc_library")
 load("@_builtins//:common/objc/compilation_support.bzl", "compilation_support")
@@ -58,7 +58,6 @@ exported_rules = {
     "objc_library": objc_library,
     "proto_library": proto_library,
     "+cc_shared_library": cc_shared_library,
-    "+cc_shared_library_permissions": cc_shared_library_permissions,
     "+cc_binary": cc_binary,
     "+cc_test": cc_test,
     "+cc_library": cc_library,
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/BUILD.builtin_test
@@ -194,9 +194,6 @@ cc_shared_library(
         "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:bar",
     ],
     features = ["windows_export_all_symbols"],
-    permissions = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:permissions",
-    ],
     deps = [
         "bar",
         "bar2",
@@ -319,18 +316,6 @@ paths_test(
     name = "path_matching_test",
 )
 
-build_failure_test(
-    name = "export_without_permissions_test",
-    message = "doesn't have the necessary permissions",
-    target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:permissions_fail_so",
-)
-
-build_failure_test(
-    name = "forbidden_target_permissions_test",
-    message = "can only list targets that are in the same package or a sub-package",
-    target_under_test = "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets:permissions_fail",
-)
-
 cc_library(
     name = "prebuilt",
     hdrs = ["direct_so_file_cc_lib.h"],
@@ -391,15 +376,6 @@ cc_library(
     ],
 )
 
-cc_shared_library_permissions(
-    name = "permissions",
-    targets = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:a_suffix",
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:qux",
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:qux2",
-    ],
-)
-
 cc_library(
     name = "static_lib_no_exporting",
     srcs = [
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library/failing_targets/BUILD.builtin_test
@@ -32,22 +32,6 @@ cc_library(
     ],
 )
 
-cc_shared_library(
-    name = "permissions_fail_so",
-    deps = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:bar",
-    ],
-    tags = TAGS,
-)
-
-cc_shared_library_permissions(
-    name = "permissions_fail",
-    tags = TAGS,
-    targets = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library:foo",
-    ],
-)
-
 cc_library(
     name = "a",
     srcs = ["a.cc"],
diff --git a/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3/BUILD.builtin_test b/src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3/BUILD.builtin_test
@@ -21,10 +21,3 @@ cc_shared_library(
         ":diff_pkg",
     ],
 )
-
-cc_shared_library_permissions(
-    name = "permissions",
-    targets = [
-        "//src/main/starlark/tests/builtins_bzl/cc/cc_shared_library/test_cc_shared_library3:bar",
-    ],
-)
