8000 Breaking Change: Add ASAN poisoning after clearing oneof messages on … · protocolbuffers/protobuf@54d068e · GitHub
[go: up one dir, main page]
Skip to content

Commit 54d068e

Browse files
tonyliaosscopybara-github
tonyliaoss
authored and
copybara-github
committed
Breaking Change: Add ASAN poisoning after clearing oneof messages on arena.
Note: This change primarily affects debug + ASAN builds using protobuf arenas. If this change causes a crash in your debug build, it probably means that there is a use-after-free bug in your program. This change has already been implemented and battle-tested within Google for some time. Oneof messages on the regular heap should not be affected because the memory they hold are already deleted. Users will already see use-after-free errors if they attempt to access heap-allocated oneof messages after calling Clear(). When a protobuf message is cleared, all raw pointers should be invalidated because undefined things may happen to any of the fields pointed to by mutable_foo() APIs. While destructors may not necessarily be invoked, Clear() should be considered a pointer invalidation event. #test-continuous PiperOrigin-RevId: 689569669
1 parent 923ee76 commit 54d068e

File tree

3 files changed

+54
-0
lines changed

3 files changed

+54
-0
lines changed

‎src/google/protobuf/arena_unittest.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1541,6 +1541,10 @@ TEST(ArenaTest, ClearOneofMessageOnArena) {
15411541

15421542
#ifndef PROTOBUF_ASAN
15431543
EXPECT_NE(child->moo_int(), 100);
1544+
#else
1545+
#if GTEST_HAS_DEATH_TEST
1546+
EXPECT_DEATH(EXPECT_EQ(child->moo_int(), 0), "use-after-poison");
1547+
#endif // !GTEST_HAS_DEATH_TEST
15441548
#endif // !PROTOBUF_ASAN
15451549
}
15461550

‎src/google/protobuf/generated_message_reflection.cc

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1330,7 +1330,53 @@ void Reflection::InternalSwap(Message* lhs, Message* rhs) const {
13301330
}
13311331
13321332
void Reflection::MaybePoisonAfterClear(Message& root) const {
1333+
struct MemBlock {
1334+
explicit MemBlock(Message& msg)
1335+
: ptr(static_cast<void*>(&msg)), size(GetSize(msg)) {}
1336+
1337+
static uint32_t GetSize(const Message& msg) {
1338+
return msg.GetReflection()->schema_.GetObjectSize();
1339+
}
1340+
1341+
void* ptr;
1342+
uint32_t size;
1343+
};
1344+
1345+
bool heap_alloc = root.GetArena() == nullptr;
1346+
std::vector<MemBlock> nodes;
1347+
1348+
#ifdef __cpp_if_constexpr
1349+
nodes.emplace_back(root);
1350+
1351+
std::queue<Message*> queue;
1352+
queue.push(&root);
1353+
1354+
while (!queue.empty() && !heap_alloc) {
1355+
Message* curr = queue.front();
1356+
queue.pop();
1357+
internal::VisitMutableMessageFields(*curr, [&](Message& msg) {
1358+
if (msg.GetArena() == nullptr) {
1359+
heap_alloc = true;
1360+
return;
1361+
}
1362+
1363+
nodes.emplace_back(msg);
1364+
// Also visits child messages.
1365+
queue.push(&msg);
1366+
});
1367+
}
1368+
#endif
1369+
13331370
root.Clear();
1371+
1372+
// Heap allocated oneof messages will be freed on clear. So, poisoning
1373+
// afterwards may cause use-after-free. Bailout.
1374+
if (heap_alloc) return;
1375+
1376+
for (auto it : nodes) {
1377+
(void)it;
1378+
PROTOBUF_POISON_MEMORY_REGION(it.ptr, it.size);
1379+
}
13341380
}
13351381
13361382
int Reflection::FieldSize(const Message& message,

‎src/google/protobuf/proto3_arena_unittest.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,10 @@ TEST(Proto3ArenaTest, CheckOneofMessageFieldIsCleared) {
289289

290290
#ifndef PROTOBUF_ASAN
291291
EXPECT_EQ(child->bb(), 0);
292+
#else
293+
#if GTEST_HAS_DEATH_TEST
294+
EXPECT_DEATH(EXPECT_EQ(child->bb(), 100), "use-after-poison");
295+
#endif // !GTEST_HAS_DEATH_TEST
292296
#endif // !PROTOBUF_ASAN
293297
}
294298

0 commit comments

Comments
 (0)
0