Skip to content

Commit

Permalink
Fix bug in reflection based Swap of map fields.
Browse files Browse the repository at this point in the history
It was swapping the arena pointers too when they differed. In that case a full
copy must be made instead. The instances can't change arenas.

PiperOrigin-RevId: 569166797
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Sep 28, 2023
1 parent 804573f commit bef5b75
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 4 deletions.
6 changes: 4 additions & 2 deletions src/google/protobuf/map.h
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ class PROTOBUF_EXPORT UntypedMapBase {
public:
Arena* arena() const { return this->alloc_.arena(); }

void Swap(UntypedMapBase* other) {
void InternalSwap(UntypedMapBase* other) {
std::swap(num_elements_, other->num_elements_);
std::swap(num_buckets_, other->num_buckets_);
std::swap(seed_, other->seed_);
Expand Down Expand Up @@ -1501,7 +1501,9 @@ class Map : private internal::KeyMapBase<internal::KeyForBase<Key>> {
}
}

void InternalSwap(Map* other) { this->Swap(other); }
void InternalSwap(Map* other) {
internal::UntypedMapBase::InternalSwap(other);
}

hasher hash_function() const { return {}; }

Expand Down
2 changes: 1 addition & 1 deletion src/google/protobuf/map_field_inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ template <typename Key, typename T>
void TypeDefinedMapFieldBase<Key, T>::Swap(MapFieldBase* other) {
MapFieldBase::Swap(other);
auto* other_field = DownCast<TypeDefinedMapFieldBase*>(other);
map_.Swap(&other_field->map_);
map_.swap(other_field->map_);
}

template <typename Key, typename T>
Expand Down
7 changes: 6 additions & 1 deletion src/google/protobuf/map_test.inc
Original file line number Diff line number Diff line change
Expand Up @@ -1270,7 +1270,6 @@ TEST_F(MapImplTest, SwapArena) {

TEST_F(MapImplTest, SwapFieldArenaReflection) {
MapReflectionTester reflection_tester(UNITTEST::TestMap::descriptor());
Arena arena;

{
// Tests filled lfs and empty rhs.
Expand All @@ -1289,9 +1288,15 @@ TEST_F(MapImplTest, SwapFieldArenaReflection) {
reflection->SwapFields(lhs, &rhs, fields);

reflection_tester.ExpectClearViaReflection(*lhs);

// Add an entry to make sure it is using the right arena.
(*lhs->mutable_map_int32_int32())[1234] = 1234;
}

reflection_tester.ExpectMapFieldsSetViaReflection(rhs);

// Add an entry to make sure it is using the right arena.
(*rhs.mutable_map_int32_int32())[1234] = 1234;
}
}

Expand Down

0 comments on commit bef5b75

Please sign in to comment.