bpo-32787: Better error handling in ctypes. (#3727)

* bpo-31572: Get rid of PyObject_HasAttrString() in ctypes.

* Fix error handling for _pack_.

* Don't silence errors when look up in a dict.

* Use _PyObject_LookupAttrId().

* More changes.

Modules/_ctypes/callbacks.c

@@ -107,9 +107,14 @@ static void
 TryAddRef(StgDictObject *dict, CDataObject *obj)
 {
     IUnknown *punk;
+    _Py_IDENTIFIER(_needs_com_addref_);
 
-    if (NULL == PyDict_GetItemString((PyObject *)dict, "_needs_com_addref_"))
+    if (!_PyDict_GetItemIdWithError((PyObject *)dict, &PyId__needs_com_addref_)) {
+        if (PyErr_Occurred()) {
+            PrintError("getting _needs_com_addref_");
+        }
         return;
+    }
 
     punk = *(IUnknown **)obj->b_ptr;
     if (punk)

Modules/_ctypes/callproc.c

@@ -146,7 +146,7 @@ _ctypes_get_errobj(int **pspace)
         if (error_object_name == NULL)
             return NULL;
     }
-    errobj = PyDict_GetItem(dict, error_object_name);
+    errobj = PyDict_GetItemWithError(dict, error_object_name);
     if (errobj) {
         if (!PyCapsule_IsValid(errobj, CTYPES_CAPSULE_NAME_PYMEM)) {
             PyErr_SetString(PyExc_RuntimeError,
@@ -155,7 +155,7 @@ _ctypes_get_errobj(int **pspace)
         }
         Py_INCREF(errobj);
     }
-    else {
+    else if (!PyErr_Occurred()) {
         void *space = PyMem_Malloc(sizeof(int) * 2);
         if (space == NULL)
             return NULL;
@@ -171,6 +171,9 @@ _ctypes_get_errobj(int **pspace)
             return NULL;
         }
     }
+    else {
+        return NULL;
+    }
     *pspace = (int *)PyCapsule_GetPointer(errobj, CTYPES_CAPSULE_NAME_PYMEM);
     return errobj;
 }
@@ -685,8 +688,11 @@ static int ConvParam(PyObject *obj, Py_ssize_t index, struct argument *pa)
 #endif
 
     {
+        _Py_IDENTIFIER(_as_parameter_);
         PyObject *arg;
-        arg = PyObject_GetAttrString(obj, "_as_parameter_");
+        if (_PyObject_LookupAttrId(obj, &PyId__as_parameter_, &arg) < 0) {
+            return -1;
+        }
         /* Which types should we exactly allow here?
            integers are required for using Python classes
            as parameters (they have to expose the '_as_parameter_'
@@ -1685,11 +1691,14 @@ POINTER(PyObject *self, PyObject *cls)
     PyObject *key;
     char *buf;
 
-    result = PyDict_GetItem(_ctypes_ptrtype_cache, cls);
+    result = PyDict_GetItemWithError(_ctypes_ptrtype_cache, cls);
     if (result) {
         Py_INCREF(result);
         return result;
     }
+    else if (PyErr_Occurred()) {
+        return NULL;
+    }
     if (PyUnicode_CheckExact(cls)) {
         const char *name = PyUnicode_AsUTF8(cls);
         if (name == NULL)
@@ -1745,12 +1754,16 @@ pointer(PyObject *self, PyObject *arg)
     PyObject *result;
     PyObject *typ;
 
-    typ = PyDict_GetItem(_ctypes_ptrtype_cache, (PyObject *)Py_TYPE(arg));
-    if (typ)
+    typ = PyDict_GetItemWithError(_ctypes_ptrtype_cache, (PyObject *)Py_TYPE(arg));
+    if (typ) {
         return PyObject_CallFunctionObjArgs(typ, arg, NULL);
+    }
+    else if (PyErr_Occurred()) {
+        return NULL;
+    }
     typ = POINTER(NULL, (PyObject *)Py_TYPE(arg));
     if (typ == NULL)
-                    return NULL;
+        return NULL;
     result = PyObject_CallFunctionObjArgs(typ, arg, NULL);
     Py_DECREF(typ);
     return result;

Modules/_ctypes/cfield.c

@@ -1147,7 +1147,7 @@ c_set(void *ptr, PyObject *value, Py_ssize_t size)
     }
     if (PyLong_Check(value))
     {
-        long longval = PyLong_AS_LONG(value);
+        long longval = PyLong_AsLong(value);
         if (longval < 0 || longval >= 256)
             goto error;
         *(char *)ptr = (char)longval;

Modules/_ctypes/stgdict.c

@@ -281,13 +281,15 @@ MakeFields(PyObject *type, CFieldObject *descr,
 static int
 MakeAnonFields(PyObject *type)
 {
+    _Py_IDENTIFIER(_anonymous_);
     PyObject *anon;
     PyObject *anon_names;
     Py_ssize_t i;
 
-    anon = PyObject_GetAttrString(type, "_anonymous_");
+    if (_PyObject_LookupAttrId(type, &PyId__anonymous_, &anon) < 0) {
+        return -1;
+    }
     if (anon == NULL) {
-        PyErr_Clear();
         return 0;
     }
     anon_names = PySequence_Fast(anon, "_anonymous_ must be a sequence");
@@ -335,13 +337,17 @@ MakeAnonFields(PyObject *type)
 int
 PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct)
 {
+    _Py_IDENTIFIER(_swappedbytes_);
+    _Py_IDENTIFIER(_use_broken_old_ctypes_structure_semantics_);
+    _Py_IDENTIFIER(_pack_);
     StgDictObject *stgdict, *basedict;
     Py_ssize_t len, offset, size, align, i;
     Py_ssize_t union_size, total_align;
     Py_ssize_t field_size = 0;
     int bitofs;
-    PyObject *isPacked;
-    int pack = 0;
+    PyObject *tmp;
+    int isPacked;
+    int pack;
     Py_ssize_t ffi_ofs;
     int big_endian;
 
@@ -356,32 +362,59 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct
     if (fields == NULL)
         return 0;
 
-#ifdef WORDS_BIGENDIAN
-    big_endian = PyObject_HasAttrString(type, "_swappedbytes_") ? 0 : 1;
-#else
-    big_endian = PyObject_HasAttrString(type, "_swappedbytes_") ? 1 : 0;
-#endif
+    if (_PyObject_LookupAttrId(type, &PyId__swappedbytes_, &tmp) < 0) {
+        return -1;
+    }
+    if (tmp) {
+        Py_DECREF(tmp);
+        big_endian = !PY_BIG_ENDIAN;
+    }
+    else {
+        big_endian = PY_BIG_ENDIAN;
+    }
 
-    use_broken_old_ctypes_semantics = \
-        PyObject_HasAttrString(type, "_use_broken_old_ctypes_structure_semantics_");
+    if (_PyObject_LookupAttrId(type,
+                &PyId__use_broken_old_ctypes_structure_semantics_, &tmp) < 0)
+    {
+        return -1;
+    }
+    if (tmp) {
+        Py_DECREF(tmp);
+        use_broken_old_ctypes_semantics = 1;
+    }
+    else {
+        use_broken_old_ctypes_semantics = 0;
+    }
 
-    isPacked = PyObject_GetAttrString(type, "_pack_");
-    if (isPacked) {
-        pack = _PyLong_AsInt(isPacked);
-        if (pack < 0 || PyErr_Occurred()) {
-            Py_XDECREF(isPacked);
-            PyErr_SetString(PyExc_ValueError,
-                            "_pack_ must be a non-negative integer");
+    if (_PyObject_LookupAttrId(type, &PyId__pack_, &tmp) < 0) {
+        return -1;
+    }
+    if (tmp) {
+        isPacked = 1;
+        pack = _PyLong_AsInt(tmp);
+        Py_DECREF(tmp);
+        if (pack < 0) {
+            if (!PyErr_Occurred() ||
+                PyErr_ExceptionMatches(PyExc_TypeError) ||
+                PyErr_ExceptionMatches(PyExc_OverflowError))
+            {
+                PyErr_SetString(PyExc_ValueError,
+                                "_pack_ must be a non-negative integer");
+            }
             return -1;
         }
-        Py_DECREF(isPacked);
-    } else
-        PyErr_Clear();
+    }
+    else {
+        isPacked = 0;
+        pack = 0;
+    }
 
-    len = PySequence_Length(fields);
+    len = PySequence_Size(fields);
     if (len == -1) {
-        PyErr_SetString(PyExc_TypeError,
-                        "'_fields_' must be a sequence of pairs");
+        if (PyErr_ExceptionMatches(PyExc_TypeError)) {
+            PyErr_SetString(PyExc_TypeError,
+                            "'_fields_' must be a sequence of pairs");
+        }
         return -1;
     }