Skip to content

Commit

Permalink
Remove cc_shared_library_permissions
Browse files Browse the repository at this point in the history
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
  • Loading branch information
oquenchil authored and copybara-github committed Feb 17, 2023
1 parent 3b7e233 commit adfd4df
Show file tree
Hide file tree
Showing 7 changed files with 6 additions and 162 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -337,7 +302,6 @@ def _filter_inputs(
linker_input.owner,
ctx.label,
ctx.attr.exports_filter,
_get_permissions(ctx),
):
exports[owner] = True

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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]),
Expand Down
3 changes: 1 addition & 2 deletions src/main/starlark/builtins_bzl/common/exports.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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 = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

0 comments on commit adfd4df

Please sign in to comment.