Skip to content

Commit 1229d4a

Browse files
protobuf-github-botcopybara-github
protobuf-github-bot
authored and
copybara-github
committed
Fix a bug in STRICT check of namespaced enums to properly check for 'reserved 1 to max'
Also refactor the visiblity checking logic into its own class and files. PiperOrigin-RevId: 821762001
1 parent 75196cd commit 1229d4a

12 files changed

+1063
-349
lines changed

‎cmake/installed_include_golden.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ google/protobuf/stubs/common.h
133133
google/protobuf/stubs/platform_macros.h
134134
google/protobuf/stubs/port.h
135135
google/protobuf/stubs/status_macros.h
136+
google/protobuf/symbol_checker.h
136137
google/protobuf/text_format.h
137138
google/protobuf/thread_safe_arena.h
138139
google/protobuf/timestamp.pb.h

‎pkg/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,7 @@ cc_dist_library(
343343
],
344344
tags = ["manual"],
345345
deps = [
346+
"//src/google/protobuf:descriptor_test_utils",
346347
"//src/google/protobuf:lite_test_util",
347348
"//src/google/protobuf:test_util",
348349
"//src/google/protobuf:test_util2",

‎src/google/protobuf/BUILD.bazel‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -794,6 +794,7 @@ cc_library(
794794
"reflection_mode.cc",
795795
"reflection_ops.cc",
796796
"service.cc",
797+
"symbol_checker.cc",
797798
"text_format.cc",
798799
"unknown_field_set.cc",
799800
"wire_format.cc",
@@ -810,6 +811,7 @@ cc_library(
810811
":micro_string",
811812
":port",
812813
":protobuf_lite",
814+
":symbol_checker",
813815
"//src/google/protobuf/io",
814816
"//src/google/protobuf/io:gzip_stream",
815817
"//src/google/protobuf/io:printer",
@@ -849,6 +851,16 @@ cc_library(
849851
],
850852
)
851853

854+
cc_library(
855+
name = "symbol_checker",
856+
hdrs = ["symbol_checker.h"],
857+
strip_include_prefix = "/src",
858+
visibility = ["//visibility:private"],
859+
deps = [
860+
"@abseil-cpp//absl/strings",
861+
],
862+
)
863+
852864
# This target exposes the headers for the protobuf runtime, and additionally
853865
# depends on the C++ well-known types and some other miscellaneous utilities.
854866
# The purpose is to preserve compatibility with projects that do not yet comply
@@ -1649,6 +1661,33 @@ cc_test(
16491661
],
16501662
)
16511663

1664+
cc_library(
1665+
name = "descriptor_test_utils",
1666+
testonly = True,
1667+
srcs = [
1668+
"descriptor_test_utils.cc",
1669+
],
1670+
hdrs = [
1671+
"descriptor_test_utils.h",
1672+
],
1673+
strip_include_prefix = "/src",
1674+
visibility = ["//:__subpackages__"],
1675+
deps = [
1676+
":port",
1677+
":protobuf",
1678+
"//src/google/protobuf/compiler:importer",
1679+
"//src/google/protobuf/io",
1680+
"//src/google/protobuf/io:tokenizer",
1681+
"@abseil-cpp//absl/log:absl_check",
1682+
"@abseil-cpp//absl/log:die_if_null",
1683+
"@abseil-cpp//absl/status",
1684+
"@abseil-cpp//absl/status:statusor",
1685+
"@abseil-cpp//absl/strings",
1686+
"@googletest//:gtest",
1687+
"@googletest//:gtest_main",
1688+
],
1689+
)
1690+
16521691
cc_test(
16531692
name = "descriptor_unittest",
16541693
srcs = ["descriptor_unittest.cc"],
@@ -1662,6 +1701,7 @@ cc_test(
16621701
":any_cc_proto",
16631702
":cc_test_protos",
16641703
":descriptor_legacy",
1704+
":descriptor_test_utils",
16651705
":port",
16661706
":protobuf",
16671707
":test_textproto",
@@ -1691,6 +1731,34 @@ cc_test(
16911731
],
16921732
)
16931733

1734+
cc_test(
1735+
name = "symbol_checker_test",
1736+
srcs = ["symbol_checker_test.cc"],
1737+
copts = COPTS + select({
1738+
"//build_defs:config_msvc": [],
1739+
"//conditions:default": [
1740+
"-Wno-error=sign-compare",
1741+
],
1742+
}),
1743+
deps = [
1744+
":descriptor_test_utils",
1745+
":port",
1746+
":protobuf",
1747+
":symbol_checker",
1748+
"//src/google/protobuf/compiler:importer",
1749+
"//src/google/protobuf/io",
1750+
"//src/google/protobuf/io:tokenizer",
1751+
"@abseil-cpp//absl/log:absl_check",
1752+
"@abseil-cpp//absl/log:absl_log",
1753+
"@abseil-cpp//absl/log:die_if_null",
1754+
"@abseil-cpp//absl/status",
1755+
"@abseil-cpp//absl/status:statusor",
1756+
"@abseil-cpp//absl/strings",
1757+
"@googletest//:gtest",
1758+
"@googletest//:gtest_main",
1759+
],
1760+
)
1761+
16941762
cc_test(
16951763
name = "lazily_build_dependencies_test",
16961764
srcs = ["lazily_build_dependencies_test.cc"],

‎src/google/protobuf/compiler/command_line_interface_unittest.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5039,9 +5039,8 @@ TEST_F(CommandLineInterfaceTest, VisibilityFeatureSetStrictBadNestedMessage) {
50395039
"--experimental_editions " // remove when edition 2024 is valid
50405040
"--include_source_info --proto_path=$tmpdir vis.proto");
50415041
ExpectErrorSubstring(
5042-
"vis.proto: \"Inner\" is a nested message and cannot be `export` with "
5043-
"STRICT "
5044-
"default_symbol_visibility");
5042+
"vis.proto: \"naming.LocalOuter.Inner\" is a nested message and cannot "
5043+
"be `export` with STRICT default_symbol_visibility");
50455044
}
50465045

50475046
// ===================================================================

‎src/google/protobuf/descriptor.cc‎

Lines changed: 13 additions & 186 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
#include "google/protobuf/parse_context.h"
8686
#include "google/protobuf/port.h"
8787
#include "google/protobuf/repeated_ptr_field.h"
88+
#include "google/protobuf/symbol_checker.h"
8889
#include "google/protobuf/text_format.h"
8990
#include "google/protobuf/unknown_field_set.h"
9091

@@ -2361,6 +2362,8 @@ absl::string_view DescriptorPool::ErrorCollector::ErrorLocationName(
23612362
return "IMPORT";
23622363
case EDITIONS:
23632364
return "EDITIONS";
2365+
case SYMBOL:
2366+
return "SYMBOL";
23642367
case OTHER:
23652368
return "OTHER";
23662369
}
@@ -4802,57 +4805,6 @@ class DescriptorBuilder {
48024805
const MethodDescriptorProto& proto);
48034806
void SuggestFieldNumbers(FileDescriptor* file,
48044807
const FileDescriptorProto& proto);
4805-
void CheckVisibilityRules(FileDescriptor* file,
4806-
const FileDescriptorProto& proto);
4807-
4808-
// Internal State used for checking visibility rules.
4809-
struct DescriptorAndProto {
4810-
const Descriptor* descriptor;
4811-
const DescriptorProto* proto;
4812-
};
4813-
4814-
struct EnumDescriptorAndProto {
4815-
const EnumDescriptor* descriptor;
4816-
const EnumDescriptorProto* proto;
4817-
};
4818-
4819-
struct VisibilityCheckerState {
4820-
FileDescriptor* containing_file;
4821-
4822-
std::vector<DescriptorAndProto> nested_messages;
4823-
std::vector<EnumDescriptorAndProto> nested_enums;
4824-
std::vector<EnumDescriptorAndProto> namespaced_enums;
4825-
};
4826-
4827-
void CheckVisibilityRulesVisit(const Descriptor& message,
4828-
const DescriptorProto& proto,
4829-
VisibilityCheckerState& state);
4830-
void CheckVisibilityRulesVisit(const EnumDescriptor& enm,
4831-
const EnumDescriptorProto& proto,
4832-
VisibilityCheckerState& state);
4833-
void CheckVisibilityRulesVisit(const FileDescriptor&,
4834-
const FileDescriptorProto& proto,
4835-
VisibilityCheckerState& state) {}
4836-
void CheckVisibilityRulesVisit(const FieldDescriptor&,
4837-
const FieldDescriptorProto& proto,
4838-
VisibilityCheckerState& state) {}
4839-
void CheckVisibilityRulesVisit(const EnumValueDescriptor&,
4840-
const EnumValueDescriptorProto& proto,
4841-
VisibilityCheckerState& state) {}
4842-
void CheckVisibilityRulesVisit(const OneofDescriptor&,
4843-
const OneofDescriptorProto& proto,
4844-
VisibilityCheckerState& state) {}
4845-
void CheckVisibilityRulesVisit(const Descriptor::ExtensionRange&,
4846-
const DescriptorProto::ExtensionRange& proto,
4847-
VisibilityCheckerState& state) {}
4848-
void CheckVisibilityRulesVisit(const MethodDescriptor&,
4849-
const MethodDescriptorProto& proto,
4850-
VisibilityCheckerState& state) {}
4851-
void CheckVisibilityRulesVisit(const ServiceDescriptor&,
4852-
const ServiceDescriptorProto& proto,
4853-
VisibilityCheckerState& state) {}
4854-
4855-
bool IsEnumNamespaceMessage(const EnumDescriptor& enm) const;
48564808

48574809

48584810
// Checks that the extension field matches what is declared.
@@ -6732,8 +6684,16 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
67326684
});
67336685
}
67346686
if (!had_errors_ && pool_->enforce_symbol_visibility_) {
6735-
// Check Symbol Visibility Rules.
6736-
CheckVisibilityRules(result, proto);
6687+
SymbolChecker symbol_checker(result, proto);
6688+
// Check Symbol Visibility and future co-location Rules.
6689+
auto errors = symbol_checker.CheckSymbolVisibilityRules();
6690+
if (!errors.empty()) {
6691+
for (const auto& error : errors) {
6692+
AddError(error.symbol_name(), *error.descriptor(),
6693+
DescriptorPool::ErrorCollector::SYMBOL,
6694+
error.message().data());
6695+
}
6696+
}
67376697
}
67386698

67396699
if (had_errors_) {
@@ -8370,139 +8330,6 @@ void DescriptorBuilder::SuggestFieldNumbers(FileDescriptor* file,
83708330
}
83718331
}
83728332

8373-
// Populate VisibilityCheckerState for messages.
8374-
void DescriptorBuilder::CheckVisibilityRulesVisit(
8375-
const Descriptor& message, const DescriptorProto& proto,
8376-
VisibilityCheckerState& state) {
8377-
if (message.containing_type() != nullptr) {
8378-
state.nested_messages.push_back(DescriptorAndProto{&message, &proto});
8379-
}
8380-
}
8381-
8382-
// Populate VisibilityCheckerState for enums.
8383-
void DescriptorBuilder::CheckVisibilityRulesVisit(
8384-
const EnumDescriptor& enm, const EnumDescriptorProto& proto,
8385-
VisibilityCheckerState& state) {
8386-
if (enm.containing_type() != nullptr) {
8387-
if (IsEnumNamespaceMessage(enm)) {
8388-
state.namespaced_enums.push_back(EnumDescriptorAndProto{&enm, &proto});
8389-
} else {
8390-
state.nested_enums.push_back(EnumDescriptorAndProto{&enm, &proto});
8391-
}
8392-
}
8393-
}
8394-
8395-
// Returns true iff the message is a pure zero field message used only for Enum
8396-
// namespacing. AKA it is:
8397-
// * top-level
8398-
// * visibility local either explicitly or by file default
8399-
// * has reserved range of 1 to max.
8400-
bool DescriptorBuilder::IsEnumNamespaceMessage(
8401-
const EnumDescriptor& enm) const {
8402-
const Descriptor* container = enm.containing_type();
8403-
const FeatureSet::VisibilityFeature::DefaultSymbolVisibility
8404-
default_visibility = enm.features().default_symbol_visibility();
8405-
// Only allowed for top-level messages
8406-
if (container->containing_type() != nullptr) {
8407-
return false;
8408-
}
8409-
8410-
bool default_to_local =
8411-
default_visibility == FeatureSet::VisibilityFeature::STRICT ||
8412-
default_visibility == FeatureSet::VisibilityFeature::LOCAL_ALL;
8413-
8414-
bool is_local =
8415-
container->visibility_keyword() == SymbolVisibility::VISIBILITY_LOCAL ||
8416-
(container->visibility_keyword() == SymbolVisibility::VISIBILITY_UNSET &&
8417-
default_to_local);
8418-
8419-
// must either be marked local, or unset with file default making it local
8420-
if (!is_local) {
8421-
return false;
8422-
}
8423-
8424-
if (container->reserved_range_count() != 1) {
8425-
return false;
8426-
}
8427-
8428-
const Descriptor::ReservedRange* range = container->reserved_range(0);
8429-
if (range == nullptr ||
8430-
(range->start != 1 &&
8431-
range->end != FieldDescriptor::kLastReservedNumber)) {
8432-
return false;
8433-
}
8434-
8435-
return true;
8436-
}
8437-
8438-
// Enforce File-wide visibility and co-location rules.
8439-
void DescriptorBuilder::CheckVisibilityRules(FileDescriptor* file,
8440-
const FileDescriptorProto& proto) {
8441-
// Check DefaultSymbolVisibility first.
8442-
//
8443-
// For Edition 2024:
8444-
// If DefaultSymbolVisibility is STRICT enforce it with caveats for:
8445-
//
8446-
8447-
VisibilityCheckerState state;
8448-
8449-
// Build our state object so we can apply rules based on type.
8450-
internal::VisitDescriptors(
8451-
*file, proto, [&](const auto& descriptor, const auto& proto) {
8452-
CheckVisibilityRulesVisit(descriptor, proto, state);
8453-
});
8454-
8455-
// In edition 2024 we only enforce STRICT visibility rules. There are possibly
8456-
// more rules to come in future editions, but for now just apply the rule for
8457-
// enforcing nested symbol local visibility. There is a single caveat for,
8458-
// allowing nested enums to have visibility set only when
8459-
//
8460-
// local msg { export enum {} reserved 1 to max; }
8461-
for (auto& nested : state.nested_messages) {
8462-
if (nested.descriptor->visibility_keyword() ==
8463-
SymbolVisibility::VISIBILITY_EXPORT &&
8464-
nested.descriptor->features().default_symbol_visibility() ==
8465-
FeatureSet::VisibilityFeature::STRICT) {
8466-
AddError(
8467-
nested.descriptor->full_name(), *nested.proto,
8468-
DescriptorPool::ErrorCollector::INPUT_TYPE, [&] {
8469-
return absl::StrCat(
8470-
"\"", nested.descriptor->name(),
8471-
"\" is a nested message and cannot be `export` with STRICT "
8472-
"default_symbol_visibility. It must be moved to top-level, "
8473-
"ideally "
8474-
"in its own file in order to be `export`.");
8475-
});
8476-
}
8477-
}
8478-
8479-
for (auto& nested : state.nested_enums) {
8480-
if (nested.descriptor->visibility_keyword() ==
8481-
SymbolVisibility::VISIBILITY_EXPORT &&
8482-
nested.descriptor->features().default_symbol_visibility() ==
8483-
FeatureSet::VisibilityFeature::STRICT) {
8484-
// This list contains only enums not considered 'namespaced' by
8485-
// IsEnumNamespaceMessage
8486-
8487-
AddError(nested.descriptor->full_name(), *nested.proto,
8488-
DescriptorPool::ErrorCollector::INPUT_TYPE, [&] {
8489-
return absl::StrCat(
8490-
"\"", nested.descriptor->name(),
8491-
"\" is a nested enum and cannot be marked `export` with "
8492-
"STRICT "
8493-
"default_symbol_visibility. It must be moved to "
8494-
"top-level, ideally "
8495-
"in its own file in order to be `export`. For C++ "
8496-
"namespacing of enums in a messages use: `local "
8497-
"message <OuterNamespace> { export enum ",
8498-
nested.descriptor->name(), " {...} reserved 1 to max; }`");
8499-
});
8500-
}
8501-
8502-
// Enforce Future rules here:
8503-
}
8504-
}
8505-
85068333
// -------------------------------------------------------------------
85078334

85088335
// Determine if the file uses optimize_for = LITE_RUNTIME, being careful to

0 commit comments

Comments
 (0)