Merge pull request #490 from haberman/python-stringargcheck

Fixed unit test by more rigorously testing argument type

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_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_GetStrData(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_GetStrData(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_GetStrData(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_GetStrData(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_GetStrData(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_GetStrData(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_GetStrData(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_GetStrData(arg);
+  const char* name = PyUpb_VerifyStrData(arg);
   if (!name) return NULL;
 
   const upb_FileDef* f =

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);

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__':

python/protobuf.c

@@ -302,6 +302,13 @@ const char* PyUpb_GetStrData(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);
+  return NULL;
+}
+
 PyObject* PyUpb_Forbidden_New(PyObject* cls, PyObject* args, PyObject* kwds) {
   PyObject* name = PyObject_GetAttrString(cls, "__name__");
   PyErr_Format(PyExc_RuntimeError,

python/protobuf.h

@@ -207,5 +207,6 @@ static inline PyObject* PyUpb_NewRef(PyObject* obj) {
 }
 
 const char* PyUpb_GetStrData(PyObject* obj);
+const char* PyUpb_VerifyStrData(PyObject* obj);
 
 #endif  // PYUPB_PROTOBUF_H__