From 9fa1f4f9bb74776ee65847ca960a1f023eac3ebe Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 11 Sep 2024 08:27:29 -0700 Subject: [PATCH] Fix ContainerBase::DeepCopy to not modify the source object. In the worst case, the source object is a constant (eg a global default instance) and modifying them has undefined behavior. PiperOrigin-RevId: 673402981 --- .../protobuf/internal/reflection_cpp_test.py | 35 +++++++++++++++++++ python/google/protobuf/pyext/message.cc | 16 ++++++--- 2 files changed, 46 insertions(+), 5 deletions(-) create mode 100644 python/google/protobuf/internal/reflection_cpp_test.py diff --git a/python/google/protobuf/internal/reflection_cpp_test.py b/python/google/protobuf/internal/reflection_cpp_test.py new file mode 100644 index 0000000000..71e7b18cdd --- /dev/null +++ b/python/google/protobuf/internal/reflection_cpp_test.py @@ -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() diff --git a/python/google/protobuf/pyext/message.cc b/python/google/protobuf/pyext/message.cc index 650195f573..aadb67e6b5 100644 --- a/python/google/protobuf/pyext/message.cc +++ b/python/google/protobuf/pyext/message.cc @@ -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 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);