Skip to content

Commit

Permalink
Fix ContainerBase::DeepCopy to not modify the source object.
Browse files Browse the repository at this point in the history
In the worst case, the source object is a constant (eg a global default instance) and modifying them has undefined behavior.

PiperOrigin-RevId: 673402981
  • Loading branch information
protobuf-github-bot authored and copybara-github committed Sep 11, 2024
1 parent 5a68ddd commit 9fa1f4f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 5 deletions.
35 changes: 35 additions & 0 deletions python/google/protobuf/internal/reflection_cpp_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# -*- coding: utf-8 -*-
# Protocol Buffers - Google's data interchange format
# Copyright 2008 Google Inc. All rights reserved.
#
# Use of this source code is governed by a BSD-style
# license that can be found in the LICENSE file or at
# https://developers.google.com/open-source/licenses/bsd

"""Unittest for reflection.py, with C++ protos linked in."""

import copy
import unittest

from google.protobuf.internal import testing_refleaks
from google.protobuf.internal import _parameterized
from google.protobuf import unittest_pb2
from google.protobuf import unittest_proto3_arena_pb2


@_parameterized.named_parameters(
('_proto2', unittest_pb2), ('_proto3', unittest_proto3_arena_pb2)
)
@testing_refleaks.TestCase
class ReflectionTest(unittest.TestCase):

def testEmptyCompositeContainerDeepCopy(self, message_module):
proto1 = message_module.NestedTestAllTypes(
payload=message_module.TestAllTypes(optional_string='foo')
)
nested2 = copy.deepcopy(proto1.child.repeated_child)
self.assertEqual(0, len(nested2))


if __name__ == '__main__':
unittest.main()
16 changes: 11 additions & 5 deletions python/google/protobuf/pyext/message.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2651,11 +2651,17 @@ PyObject* ContainerBase::DeepCopy() {
cmessage::NewEmptyMessage(this->parent->GetMessageClass());
new_parent->message = this->parent->message->New(nullptr);

// Copy the map field into the new message.
this->parent->message->GetReflection()->SwapFields(
this->parent->message, new_parent->message,
{this->parent_field_descriptor});
this->parent->message->MergeFrom(*new_parent->message);
// There is no API to copy a single field. The closest operation we have is
// SwapFields.
// So, we copy the source into a disposable message and then swap the one
// field we care about from it.
// If the performance of this operation matters we can do a copy of the single
// field, but that would require huge switches for each type+cardinality to
// call the right read/write field functions.
std::unique_ptr<Message> tmp(this->parent->message->New(nullptr));
tmp->MergeFrom(*this->parent->message);
tmp->GetReflection()->SwapFields(tmp.get(), new_parent->message,
{this->parent_field_descriptor});

PyObject* result =
cmessage::GetFieldValue(new_parent, this->parent_field_descriptor);
Expand Down

0 comments on commit 9fa1f4f

Please sign in to comment.