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
pull/18864/head
Tony Liao 4 months ago committed by Copybara-Service
parent 923ee767b9
commit 54d068e11c
  1. 4
      src/google/protobuf/arena_unittest.cc
  2. 46
      src/google/protobuf/generated_message_reflection.cc
  3. 4
      src/google/protobuf/proto3_arena_unittest.cc

@ -1541,6 +1541,10 @@ TEST(ArenaTest, ClearOneofMessageOnArena) {
#ifndef PROTOBUF_ASAN
EXPECT_NE(child->moo_int(), 100);
#else
#if GTEST_HAS_DEATH_TEST
EXPECT_DEATH(EXPECT_EQ(child->moo_int(), 0), "use-after-poison");
#endif // !GTEST_HAS_DEATH_TEST
#endif // !PROTOBUF_ASAN
}

@ -1330,7 +1330,53 @@ void Reflection::InternalSwap(Message* lhs, Message* rhs) const {
}
void Reflection::MaybePoisonAfterClear(Message& root) const {
struct MemBlock {
explicit MemBlock(Message& msg)
: ptr(static_cast<void*>(&msg)), size(GetSize(msg)) {}
static uint32_t GetSize(const Message& msg) {
return msg.GetReflection()->schema_.GetObjectSize();
}
void* ptr;
uint32_t size;
};
bool heap_alloc = root.GetArena() == nullptr;
std::vector<MemBlock> nodes;
#ifdef __cpp_if_constexpr
nodes.emplace_back(root);
std::queue<Message*> queue;
queue.push(&root);
while (!queue.empty() && !heap_alloc) {
Message* curr = queue.front();
queue.pop();
internal::VisitMutableMessageFields(*curr, [&](Message& msg) {
if (msg.GetArena() == nullptr) {
heap_alloc = true;
return;
}
nodes.emplace_back(msg);
// Also visits child messages.
queue.push(&msg);
});
}
#endif
root.Clear();
// Heap allocated oneof messages will be freed on clear. So, poisoning
// afterwards may cause use-after-free. Bailout.
if (heap_alloc) return;
for (auto it : nodes) {
(void)it;
PROTOBUF_POISON_MEMORY_REGION(it.ptr, it.size);
}
}
int Reflection::FieldSize(const Message& message,

@ -289,6 +289,10 @@ TEST(Proto3ArenaTest, CheckOneofMessageFieldIsCleared) {
#ifndef PROTOBUF_ASAN
EXPECT_EQ(child->bb(), 0);
#else
#if GTEST_HAS_DEATH_TEST
EXPECT_DEATH(EXPECT_EQ(child->bb(), 100), "use-after-poison");
#endif // !GTEST_HAS_DEATH_TEST
#endif // !PROTOBUF_ASAN
}

Loading…
Cancel
Save