Skip to content

Commit 2afb0dc

Browse files
sbenzaquencopybara-github
sbenzaquen
authored and
copybara-github
committed
Test that all entity names have some limit, and that passing that limit will
generate an error instead of breaking elsewhere. For those that didn't have a specific limit, set it to 64k as we have for other names. This prepares the code for having hard limits on name length in a future edition. PiperOrigin-RevId: 844801777
1 parent 8e3fb61 commit 2afb0dc

File tree

3 files changed

+230
-2
lines changed

3 files changed

+230
-2
lines changed

‎src/google/protobuf/descriptor.cc‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6281,8 +6281,7 @@ const FileDescriptor* DescriptorBuilder::BuildFile(
62816281
}
62826282
}
62836283

6284-
static const int kMaximumPackageLength = 511;
6285-
if (proto.package().size() > kMaximumPackageLength) {
6284+
if (proto.package().size() > internal::NameLimits::kPackageName) {
62866285
AddError(proto.package(), proto, DescriptorPool::ErrorCollector::NAME,
62876286
"Package name is too long");
62886287
return nullptr;
@@ -6840,6 +6839,10 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto,
68406839
result->reserved_names_ =
68416840
alloc.AllocateArray<const std::string*>(reserved_name_count);
68426841
for (int i = 0; i < reserved_name_count; ++i) {
6842+
if (proto.reserved_name(i).size() > internal::NameLimits::kReservedName) {
6843+
AddError(result->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
6844+
"Reserved name too long.");
6845+
}
68436846
result->reserved_names_[i] = alloc.AllocateStrings(proto.reserved_name(i));
68446847
}
68456848

@@ -7545,6 +7548,10 @@ void DescriptorBuilder::BuildEnum(const EnumDescriptorProto& proto,
75457548
result->reserved_names_ =
75467549
alloc.AllocateArray<const std::string*>(reserved_name_count);
75477550
for (int i = 0; i < reserved_name_count; ++i) {
7551+
if (proto.reserved_name(i).size() > internal::NameLimits::kReservedName) {
7552+
AddError(result->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
7553+
"Reserved name too long.");
7554+
}
75487555
result->reserved_names_[i] = alloc.AllocateStrings(proto.reserved_name(i));
75497556
}
75507557

@@ -7618,6 +7625,11 @@ void DescriptorBuilder::BuildEnumValue(const EnumValueDescriptorProto& proto,
76187625
full_name.append(parent->full_name().data(), scope_len);
76197626
full_name.append(proto.name());
76207627

7628+
if (full_name.size() > std::numeric_limits<uint16_t>::max()) {
7629+
AddError(full_name, proto, DescriptorPool::ErrorCollector::NAME,
7630+
"Name too long.");
7631+
}
7632+
76217633
result->all_names_ =
76227634
alloc.AllocateStrings(proto.name(), std::move(full_name));
76237635
result->number_ = proto.number();

‎src/google/protobuf/descriptor.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,11 @@ bool IsEnumFullySequential(const EnumDescriptor* enum_desc);
337337
const std::string& DefaultValueStringAsString(const FieldDescriptor* field);
338338
const std::string& NameOfEnumAsString(const EnumValueDescriptor* descriptor);
339339

340+
struct NameLimits {
341+
static constexpr int kPackageName = 511;
342+
static constexpr int kReservedName = std::numeric_limits<uint16_t>::max();
343+
};
344+
340345
} // namespace internal
341346

342347
// Provide an Abseil formatter for edition names.

‎src/google/protobuf/descriptor_unittest.cc‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,6 +1387,217 @@ TEST_F(DescriptorTest, AbslStringifyWorks) {
13871387
}
13881388

13891389

1390+
static void ExtendStringTo(std::string* str, int size) {
1391+
ABSL_CHECK_LE(str->size(), size);
1392+
str->replace(0, 0, size - str->size(), 'x');
1393+
ABSL_CHECK_EQ(str->size(), size);
1394+
}
1395+
1396+
static void TestBuildFileOnNameLimits(const FileDescriptorProto& proto,
1397+
std::string* str, int limit,
1398+
absl::string_view error) {
1399+
// Builds correctly.
1400+
{
1401+
ExtendStringTo(str, limit);
1402+
DescriptorPool pool;
1403+
MockErrorCollector error_collector;
1404+
EXPECT_NE(pool.BuildFileCollectingErrors(proto, &error_collector), nullptr);
1405+
EXPECT_EQ(error_collector.text_, "");
1406+
}
1407+
1408+
ExtendStringTo(str, limit + 1);
1409+
DescriptorPool pool;
1410+
MockErrorCollector error_collector;
1411+
EXPECT_EQ(pool.BuildFileCollectingErrors(proto, &error_collector), nullptr);
1412+
// Fails with the expected error.
1413+
EXPECT_THAT(error_collector.text_, HasSubstr(error));
1414+
}
1415+
1416+
static FileDescriptorProto MakeFile(absl::string_view in) {
1417+
FileDescriptorProto proto;
1418+
ABSL_CHECK(TextFormat::ParseFromString(in, &proto));
1419+
return proto;
1420+
}
1421+
1422+
TEST_F(DescriptorTest, AllSymbolNamesHaveLengthLimits) {
1423+
auto limits = internal::NameLimits();
1424+
FileDescriptorProto proto;
1425+
1426+
// This limit comes from how AllocateNames is implemented and the math for
1427+
// that leaks below.
1428+
// Ideally we would have hard and predictable limits on each kind of symbol
1429+
// instead.
1430+
constexpr int kNamesImplLimit = std::numeric_limits<uint16_t>::max();
1431+
1432+
// FileDescriptor::package
1433+
proto = MakeFile(R"pb(name: "foo.proto" package: "Package")pb");
1434+
TestBuildFileOnNameLimits(proto, proto.mutable_package(), limits.kPackageName,
1435+
"Package name is too long");
1436+
1437+
// Descriptor::name
1438+
proto = MakeFile(R"pb(name: "foo.proto"
1439+
message_type { name: "Message" })pb");
1440+
TestBuildFileOnNameLimits(proto,
1441+
proto.mutable_message_type(0)->mutable_name(),
1442+
kNamesImplLimit, "Name too long");
1443+
// Descriptor::full_name
1444+
proto = MakeFile(R"pb(name: "foo.proto"
1445+
package: "Package"
1446+
message_type { name: "Message" })pb");
1447+
TestBuildFileOnNameLimits(
1448+
proto, proto.mutable_message_type(0)->mutable_name(),
1449+
kNamesImplLimit - proto.package().size() - 1, "Name too long");
1450+
// Descriptor::reserved_name
1451+
proto = MakeFile(
1452+
R"pb(name: "foo.proto"
1453+
package: "Package"
1454+
message_type { name: "Message" reserved_name: "Reserved" })pb");
1455+
TestBuildFileOnNameLimits(
1456+
proto, proto.mutable_message_type(0)->mutable_reserved_name(0),
1457+
limits.kReservedName, "Reserved name too long");
1458+
// No FieldDescriptor::name, because that is always within a message.
1459+
// FieldDescriptor::full_name
1460+
proto = MakeFile(R"pb(name: "foo.proto"
1461+
package: "Package"
1462+
message_type {
1463+
name: "Message"
1464+
field { name: 'field' number: 1 type: TYPE_INT64 }
1465+
})pb");
1466+
TestBuildFileOnNameLimits(
1467+
proto, proto.mutable_message_type(0)->mutable_field(0)->mutable_name(),
1468+
kNamesImplLimit - proto.message_type(0).name().size() -
1469+
proto.package().size() - 2,
1470+
"Name too long");
1471+
// FieldDescriptor::json_name
1472+
proto = MakeFile(R"pb(name: "foo.proto"
1473+
package: "Package"
1474+
message_type {
1475+
name: "Message"
1476+
field {
1477+
name: 'field'
1478+
number: 1
1479+
type: TYPE_INT64
1480+
json_name: "other"
1481+
}
1482+
})pb");
1483+
TestBuildFileOnNameLimits(
1484+
proto,
1485+
proto.mutable_message_type(0)->mutable_field(0)->mutable_json_name(),
1486+
// the math here leaks the implementation details of AllocateFieldNames.
1487+
kNamesImplLimit - 3 * (1 + proto.message_type(0).field(0).name().size()) -
1488+
proto.message_type(0).name().size() - proto.package().size() - 3,
1489+
"Name too long");
1490+
// FieldDescriptor::name (extension)
1491+
proto = MakeFile(R"pb(name: "foo.proto"
1492+
message_type {
1493+
name: "Message"
1494+
extension_range { start: 1 end: 2 }
1495+
}
1496+
extension {
1497+
name: "extension"
1498+
number: 1
1499+
type: TYPE_INT32
1500+
extendee: "Message"
1501+
})pb");
1502+
TestBuildFileOnNameLimits(proto, proto.mutable_extension(0)->mutable_name(),
1503+
kNamesImplLimit - 1, "Name too long");
1504+
// FieldDescriptor::full_name (extension)
1505+
proto = MakeFile(R"pb(name: "foo.proto"
1506+
package: "Package"
1507+
message_type {
1508+
name: "Message"
1509+
extension_range { start: 1 end: 2 }
1510+
}
1511+
extension {
1512+
name: "extension"
1513+
number: 1
1514+
type: TYPE_INT32
1515+
extendee: "Message"
1516+
})pb");
1517+
TestBuildFileOnNameLimits(proto, proto.mutable_extension(0)->mutable_name(),
1518+
kNamesImplLimit - proto.package().size() - 1,
1519+
"Name too long");
1520+
// No OneofDescriptor::name, because that is always within a message.
1521+
// OneofDescriptor::full_name
1522+
proto = MakeFile(
1523+
R"pb(name: "foo.proto"
1524+
message_type {
1525+
name: "Message"
1526+
oneof_decl { name: "oneof" }
1527+
field { name: 'field' number: 1 type: TYPE_INT64 oneof_index: 0 }
1528+
}
1529+
)pb");
1530+
TestBuildFileOnNameLimits(
1531+
proto,
1532+
proto.mutable_message_type(0)->mutable_oneof_decl(0)->mutable_name(),
1533+
kNamesImplLimit - proto.message_type(0).name().size() - 1,
1534+
"Name too long");
1535+
// EnumDescriptor::name
1536+
proto = MakeFile(R"pb(name: "foo.proto"
1537+
enum_type {
1538+
name: "Enum"
1539+
value { name: "VALUE" number: 1 }
1540+
})pb");
1541+
TestBuildFileOnNameLimits(proto, proto.mutable_enum_type(0)->mutable_name(),
1542+
kNamesImplLimit, "Name too long");
1543+
// EnumDescriptor::full_name
1544+
proto = MakeFile(R"pb(name: "foo.proto"
1545+
package: "Package"
1546+
enum_type {
1547+
name: "Enum"
1548+
value { name: "VALUE" number: 1 }
1549+
})pb");
1550+
TestBuildFileOnNameLimits(proto, proto.mutable_enum_type(0)->mutable_name(),
1551+
kNamesImplLimit - proto.package().size() - 1,
1552+
"Name too long");
1553+
// EnumDescriptor::reserved_name
1554+
proto = MakeFile(R"pb(name: "foo.proto"
1555+
enum_type {
1556+
name: "Enum"
1557+
value { name: "VALUE" number: 1 }
1558+
reserved_name: "reserved"
1559+
})pb");
1560+
TestBuildFileOnNameLimits(
1561+
proto, proto.mutable_enum_type(0)->mutable_reserved_name(0),
1562+
limits.kReservedName, "Reserved name too long");
1563+
// EnumValueDescriptor::name
1564+
proto = MakeFile(R"pb(name: "foo.proto"
1565+
enum_type {
1566+
name: "Enum"
1567+
value { name: "VALUE" number: 1 }
1568+
})pb");
1569+
TestBuildFileOnNameLimits(
1570+
proto, proto.mutable_enum_type(0)->mutable_value(0)->mutable_name(),
1571+
kNamesImplLimit, "Name too long");
1572+
// EnumValueDescriptor::full_name
1573+
proto = MakeFile(R"pb(name: "foo.proto"
1574+
package: "Package"
1575+
enum_type {
1576+
name: "Enum"
1577+
value { name: "VALUE" number: 1 }
1578+
})pb");
1579+
TestBuildFileOnNameLimits(
1580+
proto, proto.mutable_enum_type(0)->mutable_value(0)->mutable_name(),
1581+
kNamesImplLimit - proto.package().size() - 1, "Name too long");
1582+
// ServiceDescriptor::full_name
1583+
proto = MakeFile(R"pb(name: "foo.proto"
1584+
package: "Package"
1585+
service { name: "Service" })pb");
1586+
TestBuildFileOnNameLimits(proto, proto.mutable_service(0)->mutable_name(),
1587+
kNamesImplLimit - proto.package().size() - 1,
1588+
"Name too long");
1589+
// MethodDescriptor::full_name
1590+
proto = MakeFile(R"pb(name: "foo.proto"
1591+
message_type { name: "M" }
1592+
service {
1593+
name: "Service"
1594+
method { name: "A" input_type: "M" output_type: "M" }
1595+
})pb");
1596+
TestBuildFileOnNameLimits(
1597+
proto, proto.mutable_service(0)->mutable_method(0)->mutable_name(),
1598+
kNamesImplLimit - proto.service(0).name().size() - 1, "Name too long");
1599+
}
1600+
13901601
// ===================================================================
13911602

13921603
// Test simple flat messages and fields.

0 commit comments

Comments
 (0)