Skip to content

Commit 59db671

Browse files
zhangskzClaytonKnittelmkruskal-google
authored
Fix Any hasbit consistency issue in OSS. (#24400)
* 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 * update staleness * Upgrade setup-php to speed up PHP tests This build has become a severe bottleneck in our CI. To avoid this in the future, always use whatever pre-install version is on the mac runners. The linux tests will cover specific versions of PHP still. PiperOrigin-RevId: 818864695 --------- Co-authored-by: Clayton Knittel <cknittel@google.com> Co-authored-by: Mike Kruskal <mkruskal@google.com>
1 parent 39d437f commit 59db671

File tree

4 files changed

+68
-67
lines changed

4 files changed

+68
-67
lines changed

‎.github/workflows/test_php.yml‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ jobs:
189189
fail-fast: false # Don't cancel all jobs if one fails.
190190
matrix:
191191
include:
192-
- version: '8.3'
192+
- version: 'pre-installed'
193193

194194
name: ${{ matrix.continuous-only && inputs.continuous-prefix || '' }} MacOS PHP ${{ matrix.version }}
195195
# noop
@@ -201,24 +201,16 @@ jobs:
201201
with:
202202
ref: ${{ inputs.safe-checkout }}
203203

204-
- name: Uninstall problematic libgd
205-
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
206-
run: brew uninstall --ignore-dependencies gd
207-
208204
- name: Install dependencies
209205
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
210206
run: brew install coreutils gd
211207

212208
- name: Pin PHP version
213209
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
214-
uses: shivammathur/setup-php@c541c155eee45413f5b09a52248675b1a2575231 # 2.31.1
210+
uses: shivammathur/setup-php@bf6b4fbd49ca58e4608c9c89fba0b8d90bd2a39f # 2.35.5
215211
with:
216212
php-version: ${{ matrix.version }}
217213

218-
- name: Check PHP version
219-
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
220-
run: php --version | grep ${{ matrix.version }} || (echo "Invalid PHP version - $(php --version)" && exit 1)
221-
222214
- name: Setup composer
223215
if: ${{ !matrix.continuous-only || inputs.continuous-run }}
224216
uses: protocolbuffers/protobuf-ci/composer-setup@v4

‎src/google/protobuf/any.pb.h‎

Lines changed: 6 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎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
@@ -1770,15 +1770,14 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
17701770
R"cc(
17711771
bool PackFrom(const $pb$::Message& message) {
17721772
$DCHK$_NE(&message, this);
1773-
return $pbi$::InternalPackFrom(message, mutable_type_url(),
1774-
_internal_mutable_value());
1773+
return $pbi$::InternalPackFrom(message, mutable_type_url(), mutable_value());
17751774
}
17761775
bool PackFrom(const $pb$::Message& message,
17771776
::absl::string_view type_url_prefix) {
17781777
$DCHK$_NE(&message, this);
17791778
return $pbi$::InternalPackFrom(message, type_url_prefix,
17801779
mutable_type_url(),
1781-
_internal_mutable_value());
1780+
mutable_value());
17821781
}
17831782
bool UnpackTo($pb$::Message* $nonnull$ message) const {
17841783
return $pbi$::InternalUnpackTo(_internal_type_url(),
@@ -1796,17 +1795,17 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
17961795
T, const $pb$::Message&>::value>::type>
17971796
bool PackFrom(const T& message) {
17981797
return $pbi$::InternalPackFrom<T>(
1799-
message, mutable_type_url(), _internal_mutable_value());
1798+
message, mutable_type_url(), mutable_value());
18001799
}
18011800
template <
18021801
typename T,
18031802
class = typename std::enable_if<!std::is_convertible<
18041803
T, const $pb$::Message&>::value>::type>
18051804
bool PackFrom(const T& message,
18061805
::absl::string_view type_url_prefix) {
1807-
return $pbi$::InternalPackFrom<T>(
1808-
message, type_url_prefix, mutable_type_url(),
1809-
_internal_mutable_value());
1806+
return $pbi$::InternalPackFrom<T>(message, type_url_prefix,
1807+
mutable_type_url(),
1808+
mutable_value());
18101809
}
18111810
template <
18121811
typename T,
@@ -1822,15 +1821,14 @@ void MessageGenerator::GenerateAnyMethodDefinition(io::Printer* p) {
18221821
R"cc(
18231822
template <typename T>
18241823
bool PackFrom(const T& message) {
1825-
return $pbi$::InternalPackFrom(message, mutable_type_url(),
1826-
_internal_mutable_value());
1824+
return $pbi$::InternalPackFrom(message, mutable_type_url(), mutable_value());
18271825
}
18281826
template <typename T>
18291827
bool PackFrom(const T& message,
18301828
::absl::string_view type_url_prefix) {
18311829
return $pbi$::InternalPackFrom(message, type_url_prefix,
18321830
mutable_type_url(),
1833-
_internal_mutable_value());
1831+
mutable_value());
18341832
}
18351833
template <typename T>
18361834
bool UnpackTo(T* $nonnull$ message) const {

0 commit comments

Comments
 (0)