[py/upb] Make GetOptions() return immutable options for scalar type. UPB will raise a TypeError when options returned GetOptions() by is mutated. PiperOrigin-RevId: 903578729
diff --git a/python/descriptor.c b/python/descriptor.c index a46ac1e..a176bca 100644 --- a/python/descriptor.c +++ b/python/descriptor.c
@@ -16,6 +16,9 @@ #include "upb/reflection/def.h" #include "upb/util/def_to_proto.h" +// Must be last. +#include "upb/port/def.inc" + // ----------------------------------------------------------------------------- // DescriptorBase // ----------------------------------------------------------------------------- @@ -77,7 +80,10 @@ static PyUpb_DescriptorBase* PyUpb_DescriptorBase_Check( PyObject* obj, PyUpb_DescriptorType type) { - PyUpb_ModuleState* state = PyUpb_ModuleState_Get(); + PyUpb_ModuleState* state = PyUpb_ModuleState_MaybeGet(); + if (!state) { + return (PyUpb_DescriptorBase*)obj; + } PyTypeObject* type_obj = state->descriptor_types[type]; if (!PyObject_TypeCheck(obj, type_obj)) { PyErr_Format(PyExc_TypeError, "Expected object of type %S, but got %R", @@ -132,6 +138,9 @@ upb_Message_ClearFieldByDef(opts2, field); } +#if UPB_FUTURE_FREEZE_OPTIONS + upb_Message_Freeze(opts2, opts2_layout); +#endif *cached = PyUpb_Message_Get(opts2, m, py_arena); Py_DECREF(py_arena); } @@ -255,6 +264,12 @@ PyObject* ret = PyUpb_ObjCache_Get(upb_MessageDef_MiniTable(m)); if (ret) return ret; + PyUpb_ModuleState* state = PyUpb_ModuleState_MaybeGet(); + if (!state) { + PyErr_SetString(PyExc_RuntimeError, "Interpreter is finalizing"); + return NULL; + } + // On demand create the clss if not exist. However, if users repeatedly // create and destroy a class, it could trigger a loop. This is not an // issue now, but if we see CPU waste for repeatedly create and destroy @@ -1909,3 +1924,5 @@ PyUpb_SetIntAttr(fd, "CPPTYPE_BYTES", CPPTYPE_STRING) && PyUpb_SetIntAttr(fd, "CPPTYPE_MESSAGE", CPPTYPE_MESSAGE); } + +#include "upb/port/undef.inc"
diff --git a/python/extension_dict.c b/python/extension_dict.c index c0cc63f..acb500f 100644 --- a/python/extension_dict.c +++ b/python/extension_dict.c
@@ -126,10 +126,14 @@ PyUpb_ExtensionDict* self = (PyUpb_ExtensionDict*)_self; const upb_FieldDef* f = PyUpb_Message_GetExtensionDef(self->msg, key); if (!f) return -1; + if (PyUpb_Message_IsFrozen(self->msg)) { + PyErr_SetString(PyExc_TypeError, "Message is immutable."); + return -1; + } if (val) { return PyUpb_Message_SetFieldValue(self->msg, f, val, PyExc_TypeError); } else { - PyUpb_Message_DoClearField(self->msg, f); + if (!PyUpb_Message_DoClearField(self->msg, f)) return -1; return 0; } }
diff --git a/python/google/protobuf/internal/descriptor_test.py b/python/google/protobuf/internal/descriptor_test.py index 56baf0c..325ffa9 100755 --- a/python/google/protobuf/internal/descriptor_test.py +++ b/python/google/protobuf/internal/descriptor_test.py
@@ -280,12 +280,101 @@ self.my_service.GetOptions(), descriptor_pb2.ServiceOptions() ) + @unittest.skipIf( + api_implementation.Type() == 'python', 'Not fixed yet in pure Python' + ) + @unittest.skipIf(api_implementation.Type() == 'cpp', 'Not fixed yet in C++') + @unittest.skipIf( + api_implementation.Type() == 'upb', + 'Needs to wait for a breaking change release in OSS' + ) + def testModifyFrozenMessage(self): + # At least upb raises TypeError Other 2 implementations will likely be + # fixed to be consistent with upb. + immutability_error = TypeError + message_options = self.my_message.GetOptions() + other_options = descriptor_pb2.MessageOptions() + other_options.deprecated = True + + # Singular field mutation + with self.assertRaises(immutability_error): + message_options.deprecated = True + + # Clear methods + with self.assertRaises(immutability_error): + message_options.Clear() + with self.assertRaises(immutability_error): + message_options.ClearField('deprecated') + + # Merge/Copy + with self.assertRaises(immutability_error): + message_options.MergeFrom(other_options) + with self.assertRaises(immutability_error): + message_options.CopyFrom(other_options) + + # Repeated field mutations + repeated_field = message_options.uninterpreted_option + with self.assertRaises(immutability_error): + repeated_field.add() + with self.assertRaises(immutability_error): + repeated_field.append(descriptor_pb2.UninterpretedOption()) + with self.assertRaises(immutability_error): + repeated_field.extend([descriptor_pb2.UninterpretedOption()]) + with self.assertRaises(immutability_error): + repeated_field.insert(0, descriptor_pb2.UninterpretedOption()) + with self.assertRaises(immutability_error): + repeated_field.remove(descriptor_pb2.UninterpretedOption()) + with self.assertRaises(immutability_error): + repeated_field.pop() + with self.assertRaises(immutability_error): + repeated_field.sort() + with self.assertRaises(immutability_error): + repeated_field.reverse() + with self.assertRaises(immutability_error): + del repeated_field[:] + + # Unset submessage mutation + complex_opt1 = unittest_custom_options_pb2.complex_opt1 + stub_submsg = unittest_pb2.TestAllTypes.DESCRIPTOR.GetOptions().Extensions[complex_opt1] + with self.assertRaises(immutability_error): + stub_submsg.foo = 5 + + # Non-empty repeated field mutation + complex_options_msg = unittest_custom_options_pb2.VariousComplexOptions.DESCRIPTOR.GetOptions() + non_empty_repeated = complex_options_msg.Extensions[complex_opt1].foo4 + self.assertEqual(len(non_empty_repeated), 2) + with self.assertRaises(immutability_error): + non_empty_repeated.clear() + with self.assertRaises(immutability_error): + non_empty_repeated.sort() + + # Extension dict mutation + with self.assertRaises(immutability_error): + message_options.Extensions[complex_opt1] = descriptor_pb2.MessageOptions() + with self.assertRaises(immutability_error): + del message_options.Extensions[complex_opt1] + + # Map field mutations + map_field = stub_submsg.my_map + with self.assertRaises(immutability_error): + map_field['key'] = 123 + with self.assertRaises(immutability_error): + del map_field['key'] + with self.assertRaises(immutability_error): + map_field.setdefault('key', 123) + with self.assertRaises(immutability_error): + map_field.update({'key': 123}) + with self.assertRaises(immutability_error): + map_field.MergeFrom(map_field) + def testModifyOptions(self): # We unfortunately allow modification of options returned from GetOptions(). # This is not intended, and has negative consequences: # - It makes the results of GetOptions() invalid if the options are # modified. # - It has an efficiency cost from copying the options. + # + # This will be fixed in a breaking change release. message_options = self.my_message.GetOptions() message_options.deprecated = True self.assertTrue(
diff --git a/python/map.c b/python/map.c index 6496e0d..063d84f 100644 --- a/python/map.c +++ b/python/map.c
@@ -66,7 +66,8 @@ static PyTypeObject* PyUpb_MapContainer_GetClass(const upb_FieldDef* f) { assert(upb_FieldDef_IsMap(f)); - PyUpb_ModuleState* state = PyUpb_ModuleState_Get(); + PyUpb_ModuleState* state = PyUpb_ModuleState_MaybeGet(); + if (!state) return NULL; const upb_FieldDef* val = upb_MessageDef_Field(upb_FieldDef_MessageSubDef(f), 1); assert(upb_FieldDef_Number(val) == 2); @@ -76,10 +77,11 @@ PyObject* PyUpb_MapContainer_NewStub(PyObject* parent, const upb_FieldDef* f, PyObject* arena) { - // We only create stubs when the parent is reified, by convention. However - // this is not an invariant: the parent could become reified at any time. - assert(PyUpb_Message_GetIfReified(parent) == NULL); PyTypeObject* cls = PyUpb_MapContainer_GetClass(f); + if (!cls) { + PyErr_SetString(PyExc_RuntimeError, "Interpreter is finalizing"); + return NULL; + } PyUpb_MapContainer* map = (void*)PyType_GenericAlloc(cls, 0); map->arena = arena; map->field = (uintptr_t)f | 1; @@ -107,7 +109,9 @@ } else { const upb_FieldDef* f = PyUpb_MapContainer_GetField(self); upb_MessageValue msgval = {.map_val = map}; - PyUpb_Message_SetConcreteSubobj(self->ptr.parent, f, msgval); + if (!PyUpb_Message_SetConcreteSubobj(self->ptr.parent, f, msgval)) { + return NULL; + } } PyUpb_ObjCache_Add(map, &self->ob_base); Py_DECREF(self->ptr.parent); @@ -122,8 +126,21 @@ self->version++; } -upb_Map* PyUpb_MapContainer_EnsureReified(PyObject* _self) { +bool PyUpb_MapContainer_IsFrozen(PyUpb_MapContainer* self) { + if (PyUpb_MapContainer_IsStub(self)) { + return PyUpb_Message_IsFrozen(self->ptr.parent); + } else { + return upb_Map_IsFrozen(self->ptr.map); + } +} + +upb_Map* PyUpb_MapContainer_AssureWritable(PyObject* _self) { PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; + if (PyUpb_MapContainer_IsFrozen(self)) { + PyErr_SetString(PyExc_TypeError, "Map is read-only"); + return NULL; + } + self->version++; upb_Map* map = PyUpb_MapContainer_GetIfReified(self); if (map) return map; // Already writable. @@ -151,7 +168,8 @@ static int PyUpb_MapContainer_AssignSubscript(PyObject* _self, PyObject* key, PyObject* val) { PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; - upb_Map* map = PyUpb_MapContainer_EnsureReified(_self); + upb_Map* map = PyUpb_MapContainer_AssureWritable(_self); + if (!map) return -1; const upb_FieldDef* f = PyUpb_MapContainer_GetField(self); const upb_MessageDef* entry_m = upb_FieldDef_MessageSubDef(f); const upb_FieldDef* key_f = upb_MessageDef_Field(entry_m, 0); @@ -182,7 +200,8 @@ upb_MessageValue u_key, u_val; if (!PyUpb_PyToUpb(key, key_f, &u_key, NULL)) return NULL; if (!map || !upb_Map_Get(map, u_key, &u_val)) { - map = PyUpb_MapContainer_EnsureReified(_self); + map = PyUpb_MapContainer_AssureWritable(_self); + if (!map) return NULL; upb_Arena* arena = PyUpb_Arena_Get(self->arena); if (upb_FieldDef_IsSubMessage(val_f)) { const upb_MessageDef* m = upb_FieldDef_MessageSubDef(val_f); @@ -213,7 +232,12 @@ } static PyObject* PyUpb_MapContainer_Clear(PyObject* _self, PyObject* key) { - upb_Map* map = PyUpb_MapContainer_EnsureReified(_self); + PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; + upb_Map* map = PyUpb_MapContainer_GetIfReified(self); + if (!map || upb_Map_Size(map) == 0) Py_RETURN_NONE; + + map = PyUpb_MapContainer_AssureWritable(_self); + if (!map) return NULL; upb_Map_Clear(map); Py_RETURN_NONE; } @@ -234,7 +258,8 @@ } PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; - upb_Map* map = PyUpb_MapContainer_EnsureReified(_self); + upb_Map* map = PyUpb_MapContainer_AssureWritable(_self); + if (!map) return NULL; const upb_FieldDef* f = PyUpb_MapContainer_GetField(self); const upb_MessageDef* entry_m = upb_FieldDef_MessageSubDef(f); const upb_FieldDef* key_f = upb_MessageDef_Field(entry_m, 0); @@ -307,6 +332,8 @@ static PyObject* PyUpb_MapContainer_MergeFrom(PyObject* _self, PyObject* _arg) { PyUpb_MapContainer* self = (PyUpb_MapContainer*)_self; + upb_Map* map = PyUpb_MapContainer_AssureWritable(_self); + if (!map) return NULL; const upb_FieldDef* f = PyUpb_MapContainer_GetField(self); if (PyDict_Check(_arg)) { @@ -357,6 +384,10 @@ if (ret) return &ret->ob_base; PyTypeObject* cls = PyUpb_MapContainer_GetClass(f); + if (!cls) { + PyErr_SetString(PyExc_RuntimeError, "Interpreter is finalizing"); + return NULL; + } ret = (void*)PyType_GenericAlloc(cls, 0); ret->arena = arena; ret->field = (uintptr_t)f;
diff --git a/python/map.h b/python/map.h index 7aa57a2..33088a7 100644 --- a/python/map.h +++ b/python/map.h
@@ -32,8 +32,10 @@ upb_Map* PyUpb_MapContainer_Reify(PyObject* self, upb_Map* map, PyUpb_WeakMap* subobj_map, intptr_t iter); -// Reifies this map object if it is not already reified. -upb_Map* PyUpb_MapContainer_EnsureReified(PyObject* self); +// Reifies this map object if it is not already reified, and ensures it is +// mutable. If the parent message (for stubs) or the map itself (for reified +// maps) is frozen, this function will set a Python TypeError and return NULL. +upb_Map* PyUpb_MapContainer_AssureWritable(PyObject* self); // Invalidates any existing iterators for the map `obj`. void PyUpb_MapContainer_Invalidate(PyObject* obj);
diff --git a/python/message.c b/python/message.c index 0497d39..6de66ab 100644 --- a/python/message.c +++ b/python/message.c
@@ -228,7 +228,8 @@ } bool PyUpb_Message_TryCheck(PyObject* self) { - PyUpb_ModuleState* state = PyUpb_ModuleState_Get(); + PyUpb_ModuleState* state = PyUpb_ModuleState_MaybeGet(); + if (!state) return false; PyObject* type = (PyObject*)Py_TYPE(self); return Py_TYPE(type) == state->message_meta_type; } @@ -373,7 +374,7 @@ return ret; } -void PyUpb_Message_EnsureReified(PyUpb_Message* self); +bool PyUpb_Message_AssureWritable(PyUpb_Message* self); static bool PyUpb_Message_InitMapAttribute(PyObject* _self, PyObject* name, const upb_FieldDef* f, @@ -530,7 +531,7 @@ Py_ssize_t pos = 0; PyObject* name; PyObject* value; - PyUpb_Message_EnsureReified(self); + PyUpb_Message_AssureWritable(self); upb_Message* msg = PyUpb_Message_GetMsg(self); upb_Arena* arena = PyUpb_Arena_Get(self->arena); @@ -576,6 +577,7 @@ PyObject* arena) { const upb_MessageDef* sub_m = upb_FieldDef_MessageSubDef(f); PyObject* cls = PyUpb_Descriptor_GetClass(sub_m); + if (!cls) return NULL; PyUpb_Message* msg = (void*)PyType_GenericAlloc((PyTypeObject*)cls, 0); msg->def = (uintptr_t)f | 1; @@ -647,7 +649,7 @@ } /* - * PyUpb_Message_EnsureReified() + * PyUpb_Message_AssureWritable() * * This implements the "expando" behavior of Python protos: * foo = FooProto() @@ -659,13 +661,24 @@ * foo.bar.bar.bar.bar.bar.baz = 5 * * This function should be called before performing any mutation of a protobuf - * object. + * object. It will check if the message is frozen, and if not, it will + * reify the message (and any parent stubs) so that they have underlying + * upb_Message objects. + * + * Returns true if writable. If the message is frozen, it sets a Python + * TypeError and returns false. Note that we mandate that * * Post-condition: - * PyUpb_Message_IsStub(self) is false + * If true is returned, PyUpb_Message_IsStub(self) is false */ -void PyUpb_Message_EnsureReified(PyUpb_Message* self) { - if (!PyUpb_Message_IsStub(self)) return; +bool PyUpb_Message_AssureWritable(PyUpb_Message* self) { + if (PyUpb_Message_IsFrozen((PyObject*)self)) { + PyErr_SetString(PyExc_TypeError, "Message is immutable."); + return false; + } + + if (!PyUpb_Message_IsStub(self)) return true; + upb_Arena* arena = PyUpb_Arena_Get(self->arena); // This is a non-present message. We need to create a real upb_Message for @@ -690,6 +703,7 @@ // Releases ref previously owned by child->ptr.parent of our child. Py_DECREF(child); self->version++; + return true; } static void PyUpb_Message_SyncSubobjs(PyUpb_Message* self); @@ -825,16 +839,19 @@ void PyUpb_Message_CacheDelete(PyObject* _self, const upb_FieldDef* f) { PyUpb_Message* self = (void*)_self; - PyUpb_WeakMap_Delete(self->unset_subobj_map, f); + if (self->unset_subobj_map && f) { + PyUpb_WeakMap_TryDelete(self->unset_subobj_map, f); + } } -void PyUpb_Message_SetConcreteSubobj(PyObject* _self, const upb_FieldDef* f, +bool PyUpb_Message_SetConcreteSubobj(PyObject* _self, const upb_FieldDef* f, upb_MessageValue subobj) { PyUpb_Message* self = (void*)_self; - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return false; PyUpb_Message_CacheDelete(_self, f); upb_Message_SetFieldByDef(self->ptr.msg, f, subobj, PyUpb_Arena_Get(self->arena)); + return true; } static void PyUpb_Message_Dealloc(PyObject* _self) { @@ -862,6 +879,7 @@ if (ret) return ret; PyObject* cls = PyUpb_Descriptor_GetClass(m); + if (!cls) return NULL; // It is not safe to use PyObject_{,GC}_New() due to: // https://bugs.python.org/issue35810 PyUpb_Message* py_msg = (void*)PyType_GenericAlloc((PyTypeObject*)cls, 0); @@ -883,21 +901,14 @@ * Non-present messages return "stub" objects that point to their parent, but * will materialize into real upb objects if they are mutated. * - * Note: we do *not* create stubs for repeated/map fields unless the parent - * is a stub: - * * msg = TestMessage() * msg.submessage # (A) Creates a stub - * msg.repeated_foo # (B) Does *not* create a stub + * msg.repeated_foo # (B) Creates a stub * msg.submessage.repeated_bar # (C) Creates a stub * * In case (B) we have some freedom: we could either create a stub, or create - * a reified object with underlying data. It appears that either could work - * equally well, with no observable change to users. There isn't a clear - * advantage to either choice. We choose to follow the behavior of the - * pre-existing C++ behavior for consistency, but if it becomes apparent that - * there would be some benefit to reversing this decision, it should be totally - * within the realm of possibility. + * a reified object with underlying data. We choose to create a stub in all + * cases to simplify the model. */ PyObject* PyUpb_Message_GetStub(PyUpb_Message* self, const upb_FieldDef* field) { @@ -916,23 +927,33 @@ } else { subobj = PyUpb_Message_NewStub(&self->ob_base, field, self->arena); } + if (!subobj) return NULL; PyUpb_WeakMap_Add(self->unset_subobj_map, field, subobj); - assert(!PyErr_Occurred()); return subobj; } PyObject* PyUpb_Message_GetPresentWrapper(PyUpb_Message* self, const upb_FieldDef* field) { assert(!PyUpb_Message_IsStub(self)); - upb_MutableMessageValue mutval = - upb_Message_Mutable(self->ptr.msg, field, PyUpb_Arena_Get(self->arena)); + upb_MessageValue val; + + if (PyUpb_Message_IsFrozen((PyObject*)self)) { + val = upb_Message_GetFieldByDef(self->ptr.msg, field); + } else { + upb_MutableMessageValue mutval = + upb_Message_Mutable(self->ptr.msg, field, PyUpb_Arena_Get(self->arena)); + val.array_val = mutval.array; + } + if (upb_FieldDef_IsMap(field)) { - return PyUpb_MapContainer_GetOrCreateWrapper(mutval.map, field, + assert(val.map_val); + return PyUpb_MapContainer_GetOrCreateWrapper((upb_Map*)val.map_val, field, self->arena); } else { - return PyUpb_RepeatedContainer_GetOrCreateWrapper(mutval.array, field, - self->arena); + assert(val.array_val); + return PyUpb_RepeatedContainer_GetOrCreateWrapper((upb_Array*)val.array_val, + field, self->arena); } } @@ -966,14 +987,22 @@ bool submsg = upb_FieldDef_IsSubMessage(field); bool seq = upb_FieldDef_IsRepeated(field); - if ((PyUpb_Message_IsStub(self) && (submsg || seq)) || - (submsg && !seq && !upb_Message_HasFieldByDef(self->ptr.msg, field))) { + if (PyUpb_Message_IsStub(self) && (submsg || seq)) { return PyUpb_Message_GetStub(self, field); - } else if (seq) { - return PyUpb_Message_GetPresentWrapper(self, field); - } else { - return PyUpb_Message_GetScalarValue(self, field); } + + if (seq) { + upb_MessageValue val = upb_Message_GetFieldByDef(self->ptr.msg, field); + bool is_unset = upb_FieldDef_IsMap(field) ? !val.map_val : !val.array_val; + if (is_unset) return PyUpb_Message_GetStub(self, field); + return PyUpb_Message_GetPresentWrapper(self, field); + } + + if (submsg && !upb_Message_HasFieldByDef(self->ptr.msg, field)) { + return PyUpb_Message_GetStub(self, field); + } + + return PyUpb_Message_GetScalarValue(self, field); } int PyUpb_Message_SetFieldValue(PyObject* _self, const upb_FieldDef* field, @@ -989,7 +1018,7 @@ return -1; } - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return -1; if (upb_FieldDef_IsSubMessage(field)) { const upb_MessageDef* msgdef = upb_FieldDef_MessageSubDef(field); @@ -1027,6 +1056,36 @@ return self->version; } +bool PyUpb_Message_IsFrozen(PyObject* _self) { + PyUpb_Message* self = (void*)_self; + while (PyUpb_Message_IsStub(self)) { + self = self->ptr.parent; + } + bool is_frozen = upb_Message_IsFrozen(self->ptr.msg); + // Invariant: Since we only store the parent *message* (and not any parent + // repeated field or map) in a stub's `ptr.parent`, we wouldn't correctly + // handle the case where a parent message is unfrozen but a containing + // repeated field or map is frozen. Therefore, we enforce the invariant that + // every unfrozen message must also have unfrozen repeated fields and maps. +#ifndef NDEBUG + if (!is_frozen) { + const upb_MessageDef* msgdef = _PyUpb_Message_GetMsgdef(self); + int n = upb_MessageDef_FieldCount(msgdef); + for (int i = 0; i < n; i++) { + const upb_FieldDef* f = upb_MessageDef_Field(msgdef, i); + if (upb_FieldDef_IsMap(f)) { + upb_MessageValue val = upb_Message_GetFieldByDef(self->ptr.msg, f); + assert(!val.map_val || !upb_Map_IsFrozen(val.map_val)); + } else if (upb_FieldDef_IsRepeated(f)) { + upb_MessageValue val = upb_Message_GetFieldByDef(self->ptr.msg, f); + assert(!val.array_val || !upb_Array_IsFrozen(val.array_val)); + } + } + } +#endif + return is_frozen; +} + /* * PyUpb_Message_GetAttr() * @@ -1295,11 +1354,13 @@ if (!serialized) return NULL; PyObject* ret = PyUpb_Message_MergeFromString(self, serialized); Py_DECREF(serialized); - Py_XDECREF(ret); + if (!ret) return NULL; + Py_DECREF(ret); Py_RETURN_NONE; } static PyObject* PyUpb_Message_CopyFrom(PyObject* _self, PyObject* arg) { + PyUpb_Message* self = (void*)_self; if (_self->ob_type != arg->ob_type) { PyErr_Format(PyExc_TypeError, "Parameter to CopyFrom() must be instance of same class: " @@ -1310,9 +1371,8 @@ if (_self == arg) { Py_RETURN_NONE; } - PyUpb_Message* self = (void*)_self; PyUpb_Message* other = (void*)arg; - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; const upb_Message* other_msg = PyUpb_Message_GetIfReified((PyObject*)other); if (other_msg) { @@ -1331,7 +1391,7 @@ static PyObject* PyUpb_Message_SetInParent(PyObject* _self, PyObject* arg) { PyUpb_Message* self = (void*)_self; - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; Py_RETURN_NONE; } @@ -1344,6 +1404,7 @@ PyObject* PyUpb_Message_MergeFromString(PyObject* _self, PyObject* arg) { PyUpb_Message* self = (void*)_self; + if (!PyUpb_Message_AssureWritable(self)) return NULL; char* buf; Py_ssize_t size; PyObject* bytes = NULL; @@ -1361,7 +1422,6 @@ return NULL; } - PyUpb_Message_EnsureReified(self); const upb_MessageDef* msgdef = _PyUpb_Message_GetMsgdef(self); const upb_FileDef* file = upb_MessageDef_File(msgdef); const upb_ExtensionRegistry* extreg = @@ -1404,7 +1464,7 @@ } static PyObject* PyUpb_Message_Clear(PyUpb_Message* self) { - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; const upb_MessageDef* msgdef = _PyUpb_Message_GetMsgdef(self); PyUpb_WeakMap* subobj_map = self->unset_subobj_map; @@ -1436,56 +1496,61 @@ Py_RETURN_NONE; } -void PyUpb_Message_DoClearField(PyObject* _self, const upb_FieldDef* f) { +bool PyUpb_Message_DoClearField(PyObject* _self, const upb_FieldDef* f) { PyUpb_Message* self = (void*)_self; - PyUpb_Message_EnsureReified((PyUpb_Message*)self); + if (!PyUpb_Message_AssureWritable((PyUpb_Message*)self)) return false; // We must ensure that any stub object is reified so its parent no longer - // points to us. + // points to us. Otherwise, if the user later mutates the stub, it will + // re-attach itself to the parent message, which we just cleared. PyObject* sub = self->unset_subobj_map ? PyUpb_WeakMap_Get(self->unset_subobj_map, f) : NULL; - + // The goal is to ensure child is reified here, but that functionality is + // encapsulated in the AssureWritable() calls for now. Therefore assuring the + // child is writable is a side-effect we rely on here but could be reverted in + // the future if needed. if (upb_FieldDef_IsMap(f)) { // For maps we additionally have to invalidate any iterators. So we need // to get an object even if it's reified. if (!sub) { sub = PyUpb_Message_GetFieldValue(_self, f); } - PyUpb_MapContainer_EnsureReified(sub); + PyUpb_MapContainer_AssureWritable(sub); PyUpb_MapContainer_Invalidate(sub); } else if (upb_FieldDef_IsRepeated(f)) { if (sub) { - PyUpb_RepeatedContainer_EnsureReified(sub); + PyUpb_RepeatedContainer_AssureWritable(sub); } } else if (upb_FieldDef_IsSubMessage(f)) { if (sub) { - PyUpb_Message_EnsureReified((PyUpb_Message*)sub); + PyUpb_Message_AssureWritable((PyUpb_Message*)sub); } } Py_XDECREF(sub); upb_Message_ClearFieldByDef(self->ptr.msg, f); + return true; } static PyObject* PyUpb_Message_ClearExtension(PyObject* _self, PyObject* arg) { PyUpb_Message* self = (void*)_self; - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; const upb_FieldDef* f = PyUpb_Message_GetExtensionDef(_self, arg); if (!f) return NULL; - PyUpb_Message_DoClearField(_self, f); + if (!PyUpb_Message_DoClearField(_self, f)) return NULL; Py_RETURN_NONE; } static PyObject* PyUpb_Message_ClearField(PyObject* _self, PyObject* arg) { PyUpb_Message* self = (void*)_self; - // We always need EnsureReified() here (even for an unset message) to + // We always need AssureWritable() here (even for an unset message) to // preserve behavior like: // msg = FooMessage() // msg.foo.Clear() // assert msg.HasField("foo") - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; const upb_FieldDef* f; const upb_OneofDef* o; @@ -1494,13 +1559,15 @@ } if (o) f = upb_Message_WhichOneofByDef(self->ptr.msg, o); - if (f) PyUpb_Message_DoClearField(_self, f); + if (f) { + if (!PyUpb_Message_DoClearField(_self, f)) return NULL; + } Py_RETURN_NONE; } static PyObject* PyUpb_Message_DiscardUnknownFields(PyUpb_Message* self, PyObject* arg) { - PyUpb_Message_EnsureReified(self); + if (!PyUpb_Message_AssureWritable(self)) return NULL; const upb_MessageDef* msgdef = _PyUpb_Message_GetMsgdef(self); const upb_DefPool* ext_pool = upb_FileDef_Pool(upb_MessageDef_File(msgdef)); upb_Message_DiscardUnknown(self->ptr.msg, msgdef, ext_pool, 64);
diff --git a/python/message.h b/python/message.h index 85e70a6..ee65cae 100644 --- a/python/message.h +++ b/python/message.h
@@ -19,7 +19,7 @@ // Sets the field value for `f` to `subobj`, evicting the wrapper object from // the "unset subobject" cache now that real data exists for it. The caller // must also update the wrapper associated with `f` to point to `subobj` also. -void PyUpb_Message_SetConcreteSubobj(PyObject* _self, const upb_FieldDef* f, +bool PyUpb_Message_SetConcreteSubobj(PyObject* _self, const upb_FieldDef* f, upb_MessageValue subobj); // Gets a Python wrapper object for message `u_msg` of type `m`, returning a @@ -57,7 +57,7 @@ PyObject* key); // Clears the given field in this message. -void PyUpb_Message_DoClearField(PyObject* _self, const upb_FieldDef* f); +bool PyUpb_Message_DoClearField(PyObject* _self, const upb_FieldDef* f); // Clears the ExtensionDict from the message. The message must have an // ExtensionDict set. @@ -81,6 +81,9 @@ // incremented when the message changes. int PyUpb_Message_GetVersion(PyObject* _self); +// Returns true if the message or any of its parents is frozen. +bool PyUpb_Message_IsFrozen(PyObject* self); + // Module-level init. bool PyUpb_InitMessage(PyObject* m);
diff --git a/python/repeated.c b/python/repeated.c index 645ceb8..4e2caf5 100644 --- a/python/repeated.c +++ b/python/repeated.c
@@ -69,8 +69,10 @@ if (subobj_map) { PyUpb_WeakMap_DeleteIter(subobj_map, &iter); } else { - PyUpb_Message_SetConcreteSubobj(self->ptr.parent, f, - (upb_MessageValue){.array_val = arr}); + if (!PyUpb_Message_SetConcreteSubobj( + self->ptr.parent, f, (upb_MessageValue){.array_val = arr})) { + return NULL; + } } PyUpb_ObjCache_Add(arr, &self->ob_base); Py_DECREF(self->ptr.parent); @@ -80,8 +82,21 @@ return arr; } -upb_Array* PyUpb_RepeatedContainer_EnsureReified(PyObject* _self) { +bool PyUpb_RepeatedContainer_IsFrozen(PyUpb_RepeatedContainer* self) { + if (PyUpb_RepeatedContainer_IsStub(self)) { + return PyUpb_Message_IsFrozen(self->ptr.parent); + } else { + return upb_Array_IsFrozen(self->ptr.arr); + } +} + +upb_Array* PyUpb_RepeatedContainer_AssureWritable(PyObject* _self) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; + if (PyUpb_RepeatedContainer_IsFrozen(self)) { + PyErr_SetString(PyExc_TypeError, "Container is read-only"); + return NULL; + } + upb_Array* arr = PyUpb_RepeatedContainer_GetIfReified(self); if (arr) return arr; // Already writable. @@ -104,7 +119,8 @@ static PyTypeObject* PyUpb_RepeatedContainer_GetClass(const upb_FieldDef* f) { assert(upb_FieldDef_IsRepeated(f) && !upb_FieldDef_IsMap(f)); - PyUpb_ModuleState* state = PyUpb_ModuleState_Get(); + PyUpb_ModuleState* state = PyUpb_ModuleState_MaybeGet(); + if (!state) return NULL; return upb_FieldDef_IsSubMessage(f) ? state->repeated_composite_container_type : state->repeated_scalar_container_type; } @@ -118,10 +134,11 @@ PyObject* PyUpb_RepeatedContainer_NewStub(PyObject* parent, const upb_FieldDef* f, PyObject* arena) { - // We only create stubs when the parent is reified, by convention. However - // this is not an invariant: the parent could become reified at any time. - assert(PyUpb_Message_GetIfReified(parent) == NULL); PyTypeObject* cls = PyUpb_RepeatedContainer_GetClass(f); + if (!cls) { + PyErr_SetString(PyExc_RuntimeError, "Interpreter is finalizing"); + return NULL; + } PyUpb_RepeatedContainer* repeated = (void*)PyType_GenericAlloc(cls, 0); repeated->arena = arena; repeated->field = (uintptr_t)PyUpb_FieldDescriptor_Get(f) | 1; @@ -138,6 +155,10 @@ if (ret) return ret; PyTypeObject* cls = PyUpb_RepeatedContainer_GetClass(f); + if (!cls) { + PyErr_SetString(PyExc_RuntimeError, "Interpreter is finalizing"); + return NULL; + } PyUpb_RepeatedContainer* repeated = (void*)PyType_GenericAlloc(cls, 0); repeated->arena = arena; repeated->field = (uintptr_t)PyUpb_FieldDescriptor_Get(f); @@ -173,7 +194,8 @@ PyObject* PyUpb_RepeatedContainer_Extend(PyObject* _self, PyObject* value) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; size_t start_size = upb_Array_Size(arr); PyObject* it = PyObject_GetIter(value); if (!it) { @@ -409,7 +431,8 @@ PyObject* value) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; const upb_FieldDef* f = PyUpb_RepeatedContainer_GetField(self); - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return -1; Py_ssize_t size = arr ? upb_Array_Size(arr) : 0; Py_ssize_t idx, count, step; if (!PyUpb_IndexToRange(key, size, &idx, &count, &step)) return -1; @@ -425,7 +448,8 @@ PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; Py_ssize_t index = -1; if (!PyArg_ParseTuple(args, "|n", &index)) return NULL; - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; size_t size = upb_Array_Size(arr); if (index < 0) index += size; #if UPB_FUTURE_REMOVE_POP_CLAMP @@ -440,7 +464,8 @@ static PyObject* PyUpb_RepeatedContainer_Remove(PyObject* _self, PyObject* value) { - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; Py_ssize_t match_index = -1; Py_ssize_t n = PyUpb_RepeatedContainer_Length(_self); for (Py_ssize_t i = 0; i < n; ++i) { @@ -467,7 +492,7 @@ static bool PyUpb_RepeatedContainer_Assign(PyObject* _self, PyObject* list) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; const upb_FieldDef* f = PyUpb_RepeatedContainer_GetField(self); - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); Py_ssize_t size = PyList_Size(list); bool submsg = upb_FieldDef_IsSubMessage(f); upb_Arena* arena = PyUpb_Arena_Get(self->arena); @@ -499,8 +524,15 @@ } } + if (PyUpb_RepeatedContainer_IsFrozen((PyUpb_RepeatedContainer*)pself)) { + PyErr_SetString(PyExc_TypeError, "Container is read-only"); + return NULL; + } if (PyUpb_RepeatedContainer_Length(pself) == 0) Py_RETURN_NONE; + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(pself); + if (!arr) return NULL; + PyObject* ret = NULL; PyObject* full_slice = NULL; PyObject* list = NULL; @@ -524,7 +556,8 @@ } static PyObject* PyUpb_RepeatedContainer_Reverse(PyObject* _self) { - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; size_t n = upb_Array_Size(arr); size_t half = n / 2; // Rounds down. for (size_t i = 0; i < half; i++) { @@ -538,11 +571,13 @@ } static PyObject* PyUpb_RepeatedContainer_Clear(PyObject* _self) { - PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; Py_ssize_t size = PyUpb_RepeatedContainer_Length(_self); - if (size > 0) { - upb_Array_Delete(self->ptr.arr, 0, size); - } + if (size == 0) Py_RETURN_NONE; + + PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; + upb_Array_Delete(self->ptr.arr, 0, size); Py_RETURN_NONE; } @@ -557,7 +592,7 @@ static PyObject* PyUpb_RepeatedCompositeContainer_AppendNew(PyObject* _self) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); if (!arr) return NULL; const upb_FieldDef* f = PyUpb_RepeatedContainer_GetField(self); upb_Arena* arena = PyUpb_Arena_Get(self->arena); @@ -602,7 +637,7 @@ Py_ssize_t index; PyObject* value; if (!PyArg_ParseTuple(args, "nO", &index, &value)) return NULL; - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); if (!arr) return NULL; // Normalize index. @@ -689,7 +724,8 @@ static PyObject* PyUpb_RepeatedScalarContainer_Append(PyObject* _self, PyObject* value) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; - upb_Array* arr = PyUpb_RepeatedContainer_EnsureReified(_self); + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return NULL; upb_Arena* arena = PyUpb_Arena_Get(self->arena); const upb_FieldDef* f = PyUpb_RepeatedContainer_GetField(self); upb_MessageValue msgval; @@ -704,8 +740,9 @@ Py_ssize_t index, PyObject* item) { PyUpb_RepeatedContainer* self = (PyUpb_RepeatedContainer*)_self; - upb_Array* arr = PyUpb_RepeatedContainer_GetIfReified(self); - Py_ssize_t size = arr ? upb_Array_Size(arr) : 0; + upb_Array* arr = PyUpb_RepeatedContainer_AssureWritable(_self); + if (!arr) return -1; + Py_ssize_t size = upb_Array_Size(arr); if (index < 0 || index >= size) { PyErr_Format(PyExc_IndexError, "list index (%zd) out of range", index); return -1;
diff --git a/python/repeated.h b/python/repeated.h index 62d5726..86dd57a 100644 --- a/python/repeated.h +++ b/python/repeated.h
@@ -34,8 +34,11 @@ PyUpb_WeakMap* subobj_map, intptr_t iter); -// Reifies this repeated object if it is not already reified. -upb_Array* PyUpb_RepeatedContainer_EnsureReified(PyObject* self); +// Reifies this repeated object if it is not already reified, and ensures it is +// mutable. If the parent message (for stubs) or the repeated array itself (for +// reified arrays) is frozen, this function will set a Python TypeError and +// return NULL. +upb_Array* PyUpb_RepeatedContainer_AssureWritable(PyObject* self); // Implements repeated_field.extend(iterable). `_self` must be a repeated // field (either repeated composite or repeated scalar).
diff --git a/src/google/protobuf/unittest_custom_options.proto b/src/google/protobuf/unittest_custom_options.proto index 581251d..3532611 100644 --- a/src/google/protobuf/unittest_custom_options.proto +++ b/src/google/protobuf/unittest_custom_options.proto
@@ -282,6 +282,7 @@ optional int32 foo2 = 2; optional int32 foo3 = 3; repeated int32 foo4 = 4; + map<string, int32> my_map = 5; extensions 100 to max; }
diff --git a/upb/port/def.inc b/upb/port/def.inc index fca5f6f..13edab9 100644 --- a/upb/port/def.inc +++ b/upb/port/def.inc
@@ -764,10 +764,16 @@ // Owner: runze@ #define UPB_FUTURE_CONTAINER_EQ_RETURNS_NOTIMPLEMENTED 1 +// Make GetOptions() return immutable options. +// Owner: runze@ +#define UPB_FUTURE_FREEZE_OPTIONS 1 + #else #define UPB_FUTURE_REMOVE_POP_CLAMP 0 #define UPB_FUTURE_CONTAINER_EQ_RETURNS_NOTIMPLEMENTED 0 +#define UPB_FUTURE_FREEZE_OPTIONS 0 + #endif
diff --git a/upb/port/undef.inc b/upb/port/undef.inc index 6c39040..4f95de9 100644 --- a/upb/port/undef.inc +++ b/upb/port/undef.inc
@@ -77,6 +77,7 @@ #undef UPB_FUTURE_BREAKING_CHANGES #undef UPB_FUTURE_REMOVE_POP_CLAMP #undef UPB_FUTURE_CONTAINER_EQ_RETURNS_NOTIMPLEMENTED +#undef UPB_FUTURE_FREEZE_OPTIONS #undef UPB_HAS_ATTRIBUTE #undef UPB_HAS_CPP_ATTRIBUTE #undef UPB_HAS_BUILTIN
diff --git a/upb/reflection/internal/def_builder.h b/upb/reflection/internal/def_builder.h index dd65cad..3dfd938 100644 --- a/upb/reflection/internal/def_builder.h +++ b/upb/reflection/internal/def_builder.h
@@ -30,6 +30,13 @@ // We want to copy the options verbatim into the destination options proto. // We use serialize+parse as our deep copy. +#if UPB_FUTURE_FREEZE_OPTIONS +#define _UPB_FREEZE_OPTIONS(target, options_type) \ + upb_Message_Freeze((upb_Message*)target, UPB_DESC_MINITABLE(options_type)) +#else +#define _UPB_FREEZE_OPTIONS(target, options_type) +#endif + #define UPB_DEF_SET_OPTIONS(target, desc_type, options_type, proto) \ if (google_protobuf_##desc_type##_has_options(proto)) { \ size_t size; \ @@ -40,6 +47,7 @@ pb, size, _upb_DefPool_GeneratedExtensionRegistry(ctx->symtab), 0, \ _upb_DefBuilder_Arena(ctx)); \ if (!target) _upb_DefBuilder_OomErr(ctx); \ + _UPB_FREEZE_OPTIONS(target, options_type); \ } else { \ target = (const google_protobuf_##options_type*)kUpbDefOptDefault; \ }