Skip to content

Commit 407323f

Browse files
ClaytonKnittelcopybara-github
ClaytonKnittel
authored and
copybara-github
committed
Fix Any hasbit consistency issue in OSS.
`Any` `value` field is a `bytes` field in OSS, and since cl/792909628 (#22956), `_internal_mutable_*()` accessors for string fields don't set hasbits. This can cause the hasbit to be missing for the `value` field of `Any` after calling `PackFrom`, which will serialize incorrectly. This change also includes fixes to `any_test`, which failed to catch this bug. We got unlucky, and every test which checked a roundtrip `PackFrom` -> `UnpackTo` for an `Any` field reused the same `Any` object, calling `MessageLite::ParseFromString` (which `Clear()`s it) on the same object. However, `Clear()` skips string fields whose hasbits are not set. This meant the `string value` field of the `Any` was not cleared, since its hasbit had not been properly set by `PackFrom`. So even though the `Any` value skipped serializing its value, the reused `Any` object still contained the expected `string value` serialization of the submessage, and `UnpackTo` worked correctly. The culprit change was adopted in release 33.0, so it will need to be patched to this version. See #24258. Fixes #24258 PiperOrigin-RevId: 828299368
1 parent 12a8943 commit 407323f

File tree

2 files changed

+60
-50
lines changed

2 files changed

+60
-50
lines changed

‎src/google/protobuf/any_test.cc‎

Lines changed: 52 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,20 @@ namespace protobuf {
2626
namespace {
2727

2828
TEST(AnyTest, TestPackAndUnpack) {
29-
proto2_unittest::TestAny submessage;
30-
submessage.set_int32_value(12345);
31-
proto2_unittest::TestAny message;
32-
ASSERT_TRUE(message.mutable_any_value()->PackFrom(submessage));
33-
34-
std::string data = message.SerializeAsString();
29+
std::string data;
30+
{
31+
proto2_unittest::TestAny submessage;
32+
submessage.set_int32_value(12345);
33+
proto2_unittest::TestAny message;
34+
ASSERT_TRUE(message.mutable_any_value()->PackFrom(submessage));
35+
36+
data = message.SerializeAsString();
37+
}
3538

39+
proto2_unittest::TestAny message;
3640
ASSERT_TRUE(message.ParseFromString(data));
3741
EXPECT_TRUE(message.has_any_value());
38-
submessage.Clear();
42+
proto2_unittest::TestAny submessage;
3943
ASSERT_TRUE(message.any_value().UnpackTo(&submessage));
4044
EXPECT_EQ(12345, submessage.int32_value());
4145
}
@@ -62,41 +66,47 @@ TEST(AnyTest, TestUnpackWithTypeMismatch) {
6266
}
6367

6468
TEST(AnyTest, TestPackAndUnpackAny) {
65-
// We can pack a Any message inside another Any message.
66-
proto2_unittest::TestAny submessage;
67-
submessage.set_int32_value(12345);
68-
google::protobuf::Any any;
69-
any.PackFrom(submessage);
70-
proto2_unittest::TestAny message;
71-
message.mutable_any_value()->PackFrom(any);
72-
73-
std::string data = message.SerializeAsString();
69+
std::string data;
70+
{
71+
// We can pack an Any message inside another Any message.
72+
proto2_unittest::TestAny submessage;
73+
submessage.set_int32_value(12345);
74+
google::protobuf::Any any;
75+
any.PackFrom(submessage);
76+
proto2_unittest::TestAny message;
77+
message.mutable_any_value()->PackFrom(any);
78+
79+
data = message.SerializeAsString();
80+
}
7481

82+
proto2_unittest::TestAny message;
7583
ASSERT_TRUE(message.ParseFromString(data));
7684
EXPECT_TRUE(message.has_any_value());
77-
any.Clear();
78-
submessage.Clear();
85+
google::protobuf::Any any;
7986
ASSERT_TRUE(message.any_value().UnpackTo(&any));
87+
proto2_unittest::TestAny submessage;
8088
ASSERT_TRUE(any.UnpackTo(&submessage));
8189
EXPECT_EQ(12345, submessage.int32_value());
8290
}
8391

8492
TEST(AnyTest, TestPackWithCustomTypeUrl) {
85-
proto2_unittest::TestAny submessage;
86-
submessage.set_int32_value(12345);
8793
google::protobuf::Any any;
88-
// Pack with a custom type URL prefix.
89-
any.PackFrom(submessage, "type.myservice.com");
90-
EXPECT_EQ("type.myservice.com/proto2_unittest.TestAny", any.type_url());
91-
// Pack with a custom type URL prefix ending with '/'.
92-
any.PackFrom(submessage, "type.myservice.com/");
93-
EXPECT_EQ("type.myservice.com/proto2_unittest.TestAny", any.type_url());
94-
// Pack with an empty type URL prefix.
95-
any.PackFrom(submessage, "");
96-
EXPECT_EQ("/proto2_unittest.TestAny", any.type_url());
94+
{
95+
proto2_unittest::TestAny submessage;
96+
submessage.set_int32_value(12345);
97+
// Pack with a custom type URL prefix.
98+
any.PackFrom(submessage, "type.myservice.com");
99+
EXPECT_EQ("type.myservice.com/proto2_unittest.TestAny", any.type_url());
100+
// Pack with a custom type URL prefix ending with '/'.
101+
any.PackFrom(submessage, "type.myservice.com/");
102+
EXPECT_EQ("type.myservice.com/proto2_unittest.TestAny", any.type_url());
103+
// Pack with an empty type URL prefix.
104+
any.PackFrom(submessage, "");
105+
EXPECT_EQ("/proto2_unittest.TestAny", any.type_url());
106+
}
97107

98108
// Test unpacking the type.
99-
submessage.Clear();
109+
proto2_unittest::TestAny submessage;
100110
EXPECT_TRUE(any.UnpackTo(&submessage));
101111
EXPECT_EQ(12345, submessage.int32_value());
102112
}
@@ -127,34 +137,36 @@ TEST(AnyTest, TestIs) {
127137
}
128138

129139
TEST(AnyTest, MoveConstructor) {
130-
proto2_unittest::TestAny payload;
131-
payload.set_int32_value(12345);
132-
133140
google::protobuf::Any src;
134-
src.PackFrom(payload);
141+
{
142+
proto2_unittest::TestAny payload;
143+
payload.set_int32_value(12345);
144+
src.PackFrom(payload);
145+
}
135146

136147
const char* type_url = src.type_url().data();
137148

138149
google::protobuf::Any dst(std::move(src));
139150
EXPECT_EQ(type_url, dst.type_url().data());
140-
payload.Clear();
151+
proto2_unittest::TestAny payload;
141152
ASSERT_TRUE(dst.UnpackTo(&payload));
142153
EXPECT_EQ(12345, payload.int32_value());
143154
}
144155

145156
TEST(AnyTest, MoveAssignment) {
146-
proto2_unittest::TestAny payload;
147-
payload.set_int32_value(12345);
148-
149157
google::protobuf::Any src;
150-
src.PackFrom(payload);
158+
{
159+
proto2_unittest::TestAny payload;
160+
payload.set_int32_value(12345);
161+
src.PackFrom(payload);
162+
}
151163

152164
const char* type_url = src.type_url().data();
153165

154166
google::protobuf::Any dst;
155167
dst = std::move(src);
156168
EXPECT_EQ(type_url, dst.type_url().data());
157-
payload.Clear();
169+
proto2_unittest::TestAny payload;
158170
ASSERT_TRUE(dst.UnpackTo(&payload));
159171
EXPECT_EQ(12345, payload.int32_value());
160172
}

‎src/google/protobuf/compiler/cpp/message.cc‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1799,15 +1799,14 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
17991799
R"cc(
18001800
bool PackFrom(const $pb$::Message& message) {
18011801
$DCHK$_NE(&message, this);
1802-
return $pbi$::InternalPackFrom(message, mutable_type_url(),
1803-
_internal_mutable_value());
1802+
return $pbi$::InternalPackFrom(message, mutable_type_url(), mutable_value());
18041803
}
18051804
bool PackFrom(const $pb$::Message& message,
18061805
::absl::string_view type_url_prefix) {
18071806
$DCHK$_NE(&message, this);
18081807
return $pbi$::InternalPackFrom(message, type_url_prefix,
18091808
mutable_type_url(),
1810-
_internal_mutable_value());
1809+
mutable_value());
18111810
}
18121811
bool UnpackTo($pb$::Message* $nonnull$ message) const {
18131812
return $pbi$::InternalUnpackTo(_internal_type_url(),
@@ -1825,17 +1824,17 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
18251824
T, const $pb$::Message&>::value>::type>
18261825
bool PackFrom(const T& message) {
18271826
return $pbi$::InternalPackFrom<T>(
1828-
message, mutable_type_url(), _internal_mutable_value());
1827+
message, mutable_type_url(), mutable_value());
18291828
}
18301829
template <
18311830
typename T,
18321831
class = typename std::enable_if<!std::is_convertible<
18331832
T, const $pb$::Message&>::value>::type>
18341833
bool PackFrom(const T& message,
18351834
::absl::string_view type_url_prefix) {
1836-
return $pbi$::InternalPackFrom<T>(
1837-
message, type_url_prefix, mutable_type_url(),
1838-
_internal_mutable_value());
1835+
return $pbi$::InternalPackFrom<T>(message, type_url_prefix,
1836+
mutable_type_url(),
1837+
mutable_value());
18391838
}
18401839
template <
18411840
typename T,
@@ -1851,15 +1850,14 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
18511850
R"cc(
18521851
template <typename T>
18531852
bool PackFrom(const T& message) {
1854-
return $pbi$::InternalPackFrom(message, mutable_type_url(),
1855-
_internal_mutable_value());
1853+
return $pbi$::InternalPackFrom(message, mutable_type_url(), mutable_value());
18561854
}
18571855
template <typename T>
18581856
bool PackFrom(const T& message,
18591857
::absl::string_view type_url_prefix) {
18601858
return $pbi$::InternalPackFrom(message, type_url_prefix,
18611859
mutable_type_url(),
1862-
_internal_mutable_value());
1860+
mutable_value());
18631861
}
18641862
template <typename T>
18651863
bool UnpackTo(T* $nonnull$ message) const {

0 commit comments

Comments
 (0)