From e613af9e876659ee9ad7571c416f67c62ff67f59 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sun, 16 Jan 2022 19:19:34 -0800 Subject: [PATCH 1/3] Fixed test by checking string argument properly. --- python/descriptor_pool.c | 18 +++++++++--------- .../descriptor_pool_test_wrapper.py | 1 - python/protobuf.c | 7 +++++++ python/protobuf.h | 1 + 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/python/descriptor_pool.c b/python/descriptor_pool.c index dcb9bc169a..a495c7f600 100644 --- a/python/descriptor_pool.c +++ b/python/descriptor_pool.c @@ -310,7 +310,7 @@ static PyObject* PyUpb_DescriptorPool_FindFileByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_FileDef* file = upb_DefPool_FindFileByName(self->symtab, name); @@ -335,7 +335,7 @@ static PyObject* PyUpb_DescriptorPool_FindExtensionByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_FieldDef* field = @@ -361,7 +361,7 @@ static PyObject* PyUpb_DescriptorPool_FindMessageTypeByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_MessageDef* m = upb_DefPool_FindMessageByName(self->symtab, name); @@ -397,7 +397,7 @@ static PyObject* PyUpb_DescriptorPool_FindFieldByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; size_t parent_size; @@ -433,7 +433,7 @@ static PyObject* PyUpb_DescriptorPool_FindEnumTypeByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_EnumDef* e = upb_DefPool_FindEnumByName(self->symtab, name); @@ -458,7 +458,7 @@ static PyObject* PyUpb_DescriptorPool_FindOneofByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; size_t parent_size; @@ -485,7 +485,7 @@ static PyObject* PyUpb_DescriptorPool_FindServiceByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_ServiceDef* s = upb_DefPool_FindServiceByName(self->symtab, name); @@ -504,7 +504,7 @@ static PyObject* PyUpb_DescriptorPool_FindMethodByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; size_t parent_size; const char* child = PyUpb_DescriptorPool_SplitSymbolName(name, &parent_size); @@ -530,7 +530,7 @@ static PyObject* PyUpb_DescriptorPool_FindFileContainingSymbol(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_GetStrData(arg); + const char* name = PyUpb_CheckStrData(arg); if (!name) return NULL; const upb_FileDef* f = diff --git a/python/pb_unit_tests/descriptor_pool_test_wrapper.py b/python/pb_unit_tests/descriptor_pool_test_wrapper.py index 31f40742ed..a4dd686c71 100644 --- a/python/pb_unit_tests/descriptor_pool_test_wrapper.py +++ b/python/pb_unit_tests/descriptor_pool_test_wrapper.py @@ -31,7 +31,6 @@ import copy # In the new extension we simply don't define them at all. descriptor_pool_test.AddDescriptorTest.testAddTypeError.__unittest_expecting_failure__ = True -descriptor_pool_test.CreateDescriptorPoolTest.testFindTypeErrors.__unittest_expecting_failure__ = True descriptor_pool_test.SecondaryDescriptorFromDescriptorDB.testErrorCollector.__unittest_expecting_failure__ = True if __name__ == '__main__': diff --git a/python/protobuf.c b/python/protobuf.c index 4debeb1342..f3c6fdf97b 100644 --- a/python/protobuf.c +++ b/python/protobuf.c @@ -302,6 +302,13 @@ const char* PyUpb_GetStrData(PyObject* obj) { } } +const char* PyUpb_CheckStrData(PyObject* obj) { + const char* ret = PyUpb_GetStrData(obj); + if (ret) return ret; + PyErr_Format(PyExc_TypeError, "Expected string: %S", obj); + return NULL; +} + PyObject* PyUpb_Forbidden_New(PyObject* cls, PyObject* args, PyObject* kwds) { PyObject* name = PyObject_GetAttrString(cls, "__name__"); PyErr_Format(PyExc_RuntimeError, diff --git a/python/protobuf.h b/python/protobuf.h index da8ace7baa..bae3530e77 100644 --- a/python/protobuf.h +++ b/python/protobuf.h @@ -207,5 +207,6 @@ static inline PyObject* PyUpb_NewRef(PyObject* obj) { } const char* PyUpb_GetStrData(PyObject* obj); +const char* PyUpb_CheckStrData(PyObject* obj); #endif // PYUPB_PROTOBUF_H__ From b38e4a4332bf8ce088dee679bc8e573e6502fcf8 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sun, 16 Jan 2022 19:27:55 -0800 Subject: [PATCH 2/3] Fixed a few cases where we were not checking GetStrData properly. --- python/message.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/message.c b/python/message.c index 5de428a2b5..9bede8d42b 100644 --- a/python/message.c +++ b/python/message.c @@ -926,8 +926,9 @@ __attribute__((flatten)) static PyObject* PyUpb_CMessage_GetAttr( // to pick up class attributes. But we have to special-case "Extensions" // which affirmatively returns AttributeError when a message is not // extendable. + const char* name; if (PyErr_ExceptionMatches(PyExc_AttributeError) && - strcmp(PyUpb_GetStrData(attr), "Extensions") != 0) { + (name = PyUpb_GetStrData(attr)) && strcmp(name, "Extensions") != 0) { PyErr_Clear(); return PyUpb_MessageMeta_GetAttr((PyObject*)Py_TYPE(_self), attr); } @@ -1720,6 +1721,7 @@ void PyUpb_MessageMeta_AddFieldNumber(PyObject* self, const upb_FieldDef* f) { static PyObject* PyUpb_MessageMeta_GetDynamicAttr(PyObject* self, PyObject* name) { const char* name_buf = PyUpb_GetStrData(name); + if (!name_buf) return NULL; const upb_MessageDef* msgdef = PyUpb_MessageMeta_GetMsgdef(self); const upb_FileDef* filedef = upb_MessageDef_File(msgdef); const upb_DefPool* symtab = upb_FileDef_Pool(filedef); From cfdd016beda7edc1f9f365306993acd554c632f1 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sun, 16 Jan 2022 21:32:49 -0800 Subject: [PATCH 3/3] PyUpb_CheckStrData() -> PyUpb_VerifyStrData() --- python/descriptor_pool.c | 18 +++++++++--------- python/protobuf.c | 2 +- python/protobuf.h | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/python/descriptor_pool.c b/python/descriptor_pool.c index a495c7f600..b016ee23c3 100644 --- a/python/descriptor_pool.c +++ b/python/descriptor_pool.c @@ -310,7 +310,7 @@ static PyObject* PyUpb_DescriptorPool_FindFileByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_FileDef* file = upb_DefPool_FindFileByName(self->symtab, name); @@ -335,7 +335,7 @@ static PyObject* PyUpb_DescriptorPool_FindExtensionByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_FieldDef* field = @@ -361,7 +361,7 @@ static PyObject* PyUpb_DescriptorPool_FindMessageTypeByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_MessageDef* m = upb_DefPool_FindMessageByName(self->symtab, name); @@ -397,7 +397,7 @@ static PyObject* PyUpb_DescriptorPool_FindFieldByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; size_t parent_size; @@ -433,7 +433,7 @@ static PyObject* PyUpb_DescriptorPool_FindEnumTypeByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_EnumDef* e = upb_DefPool_FindEnumByName(self->symtab, name); @@ -458,7 +458,7 @@ static PyObject* PyUpb_DescriptorPool_FindOneofByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; size_t parent_size; @@ -485,7 +485,7 @@ static PyObject* PyUpb_DescriptorPool_FindServiceByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_ServiceDef* s = upb_DefPool_FindServiceByName(self->symtab, name); @@ -504,7 +504,7 @@ static PyObject* PyUpb_DescriptorPool_FindMethodByName(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; size_t parent_size; const char* child = PyUpb_DescriptorPool_SplitSymbolName(name, &parent_size); @@ -530,7 +530,7 @@ static PyObject* PyUpb_DescriptorPool_FindFileContainingSymbol(PyObject* _self, PyObject* arg) { PyUpb_DescriptorPool* self = (PyUpb_DescriptorPool*)_self; - const char* name = PyUpb_CheckStrData(arg); + const char* name = PyUpb_VerifyStrData(arg); if (!name) return NULL; const upb_FileDef* f = diff --git a/python/protobuf.c b/python/protobuf.c index f3c6fdf97b..35600ae788 100644 --- a/python/protobuf.c +++ b/python/protobuf.c @@ -302,7 +302,7 @@ const char* PyUpb_GetStrData(PyObject* obj) { } } -const char* PyUpb_CheckStrData(PyObject* obj) { +const char* PyUpb_VerifyStrData(PyObject* obj) { const char* ret = PyUpb_GetStrData(obj); if (ret) return ret; PyErr_Format(PyExc_TypeError, "Expected string: %S", obj); diff --git a/python/protobuf.h b/python/protobuf.h index bae3530e77..a45632b6d5 100644 --- a/python/protobuf.h +++ b/python/protobuf.h @@ -207,6 +207,6 @@ static inline PyObject* PyUpb_NewRef(PyObject* obj) { } const char* PyUpb_GetStrData(PyObject* obj); -const char* PyUpb_CheckStrData(PyObject* obj); +const char* PyUpb_VerifyStrData(PyObject* obj); #endif // PYUPB_PROTOBUF_H__