From f55cb44359821e71c29903f2152b4658509dac0d Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 7 Nov 2023 15:58:04 +0200 Subject: [PATCH] gh-106672: C API: Report indiscriminately ignored errors (GH-106674) Functions which indiscriminately ignore all errors now report them as unraisable errors. --- Doc/whatsnew/3.13.rst | 9 ++ Lib/test/test_capi/test_abstract.py | 104 +++++++++++++++--- Lib/test/test_capi/test_dict.py | 19 +++- Lib/test/test_capi/test_sys.py | 6 +- ...-07-12-12-14-52.gh-issue-106672.fkRjmi.rst | 5 + Objects/abstract.c | 58 +++++++--- Objects/dictobject.c | 26 ++++- Objects/object.c | 10 +- Python/sysmodule.c | 3 + 9 files changed, 195 insertions(+), 45 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index ef83f662788..cca282727ab 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -980,6 +980,15 @@ Changes in the Python API The result is now the same if ``wantobjects`` is set to ``0``. (Contributed by Serhiy Storchaka in :gh:`97928`.) +* Functions :c:func:`PyDict_GetItem`, :c:func:`PyDict_GetItemString`, + :c:func:`PyMapping_HasKey`, :c:func:`PyMapping_HasKeyString`, + :c:func:`PyObject_HasAttr`, :c:func:`PyObject_HasAttrString`, and + :c:func:`PySys_GetObject`, which clear all errors occurred during calling + the function, report now them using :func:`sys.unraisablehook`. + You can consider to replace these functions with other functions as + recomended in the documentation. + (Contributed by Serhiy Storchaka in :gh:`106672`.) + Build Changes ============= diff --git a/Lib/test/test_capi/test_abstract.py b/Lib/test/test_capi/test_abstract.py index 5b5dfb3cc21..26152c30498 100644 --- a/Lib/test/test_capi/test_abstract.py +++ b/Lib/test/test_capi/test_abstract.py @@ -1,5 +1,6 @@ import unittest from collections import OrderedDict +from test import support from test.support import import_helper _testcapi = import_helper.import_module('_testcapi') @@ -109,8 +110,18 @@ class CAPITest(unittest.TestCase): self.assertFalse(xhasattr(obj, 'b')) self.assertTrue(xhasattr(obj, '\U0001f40d')) - self.assertFalse(xhasattr(obj, 'evil')) - self.assertFalse(xhasattr(obj, 1)) + with support.catch_unraisable_exception() as cm: + self.assertFalse(xhasattr(obj, 'evil')) + self.assertEqual(cm.unraisable.exc_type, RuntimeError) + self.assertEqual(str(cm.unraisable.exc_value), + 'do not get evil') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(xhasattr(obj, 1)) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "attribute name must be string, not 'int'") + # CRASHES xhasattr(obj, NULL) # CRASHES xhasattr(NULL, 'a') @@ -123,8 +134,18 @@ class CAPITest(unittest.TestCase): self.assertFalse(hasattrstring(obj, b'b')) self.assertTrue(hasattrstring(obj, '\U0001f40d'.encode())) - self.assertFalse(hasattrstring(obj, b'evil')) - self.assertFalse(hasattrstring(obj, b'\xff')) + with support.catch_unraisable_exception() as cm: + self.assertFalse(hasattrstring(obj, b'evil')) + self.assertEqual(cm.unraisable.exc_type, RuntimeError) + self.assertEqual(str(cm.unraisable.exc_value), + 'do not get evil') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(hasattrstring(obj, b'\xff')) + self.assertEqual(cm.unraisable.exc_type, UnicodeDecodeError) + self.assertRegex(str(cm.unraisable.exc_value), + "'utf-8' codec can't decode") + # CRASHES hasattrstring(obj, NULL) # CRASHES hasattrstring(NULL, b'a') @@ -342,12 +363,41 @@ class CAPITest(unittest.TestCase): self.assertTrue(haskey(['a', 'b', 'c'], 1)) - self.assertFalse(haskey(42, 'a')) - self.assertFalse(haskey({}, [])) # unhashable - self.assertFalse(haskey({}, NULL)) - self.assertFalse(haskey([], 1)) - self.assertFalse(haskey([], 'a')) - self.assertFalse(haskey(NULL, 'a')) + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey(42, 'a')) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "'int' object is not subscriptable") + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey({}, [])) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "unhashable type: 'list'") + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey([], 1)) + self.assertEqual(cm.unraisable.exc_type, IndexError) + self.assertEqual(str(cm.unraisable.exc_value), + 'list index out of range') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey([], 'a')) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + 'list indices must be integers or slices, not str') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey({}, NULL)) + self.assertEqual(cm.unraisable.exc_type, SystemError) + self.assertEqual(str(cm.unraisable.exc_value), + 'null argument to internal routine') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskey(NULL, 'a')) + self.assertEqual(cm.unraisable.exc_type, SystemError) + self.assertEqual(str(cm.unraisable.exc_value), + 'null argument to internal routine') def test_mapping_haskeystring(self): haskeystring = _testcapi.mapping_haskeystring @@ -360,11 +410,35 @@ class CAPITest(unittest.TestCase): self.assertTrue(haskeystring(dct2, b'a')) self.assertFalse(haskeystring(dct2, b'b')) - self.assertFalse(haskeystring(42, b'a')) - self.assertFalse(haskeystring({}, b'\xff')) - self.assertFalse(haskeystring({}, NULL)) - self.assertFalse(haskeystring([], b'a')) - self.assertFalse(haskeystring(NULL, b'a')) + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskeystring(42, b'a')) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "'int' object is not subscriptable") + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskeystring({}, b'\xff')) + self.assertEqual(cm.unraisable.exc_type, UnicodeDecodeError) + self.assertRegex(str(cm.unraisable.exc_value), + "'utf-8' codec can't decode") + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskeystring({}, NULL)) + self.assertEqual(cm.unraisable.exc_type, SystemError) + self.assertEqual(str(cm.unraisable.exc_value), + "null argument to internal routine") + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskeystring([], b'a')) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + 'list indices must be integers or slices, not str') + + with support.catch_unraisable_exception() as cm: + self.assertFalse(haskeystring(NULL, b'a')) + self.assertEqual(cm.unraisable.exc_type, SystemError) + self.assertEqual(str(cm.unraisable.exc_value), + "null argument to internal routine") def test_mapping_haskeywitherror(self): haskey = _testcapi.mapping_haskeywitherror diff --git a/Lib/test/test_capi/test_dict.py b/Lib/test/test_capi/test_dict.py index 11b2ca91070..67f12a56313 100644 --- a/Lib/test/test_capi/test_dict.py +++ b/Lib/test/test_capi/test_dict.py @@ -1,6 +1,7 @@ import unittest from collections import OrderedDict, UserDict from types import MappingProxyType +from test import support import _testcapi @@ -30,7 +31,7 @@ class CAPITest(unittest.TestCase): self.assertFalse(check(UserDict({1: 2}))) self.assertFalse(check([1, 2])) self.assertFalse(check(object())) - #self.assertFalse(check(NULL)) + # CRASHES check(NULL) def test_dict_checkexact(self): check = _testcapi.dict_checkexact @@ -39,7 +40,7 @@ class CAPITest(unittest.TestCase): self.assertFalse(check(UserDict({1: 2}))) self.assertFalse(check([1, 2])) self.assertFalse(check(object())) - #self.assertFalse(check(NULL)) + # CRASHES check(NULL) def test_dict_new(self): dict_new = _testcapi.dict_new @@ -118,7 +119,12 @@ class CAPITest(unittest.TestCase): self.assertEqual(getitem(dct2, 'a'), 1) self.assertIs(getitem(dct2, 'b'), KeyError) - self.assertIs(getitem({}, []), KeyError) # unhashable + with support.catch_unraisable_exception() as cm: + self.assertIs(getitem({}, []), KeyError) # unhashable + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "unhashable type: 'list'") + self.assertIs(getitem(42, 'a'), KeyError) self.assertIs(getitem([1], 0), KeyError) # CRASHES getitem({}, NULL) @@ -135,7 +141,12 @@ class CAPITest(unittest.TestCase): self.assertEqual(getitemstring(dct2, b'a'), 1) self.assertIs(getitemstring(dct2, b'b'), KeyError) - self.assertIs(getitemstring({}, INVALID_UTF8), KeyError) + with support.catch_unraisable_exception() as cm: + self.assertIs(getitemstring({}, INVALID_UTF8), KeyError) + self.assertEqual(cm.unraisable.exc_type, UnicodeDecodeError) + self.assertRegex(str(cm.unraisable.exc_value), + "'utf-8' codec can't decode") + self.assertIs(getitemstring(42, b'a'), KeyError) self.assertIs(getitemstring([], b'a'), KeyError) # CRASHES getitemstring({}, NULL) diff --git a/Lib/test/test_capi/test_sys.py b/Lib/test/test_capi/test_sys.py index b83a0a1604d..08bf0370fc5 100644 --- a/Lib/test/test_capi/test_sys.py +++ b/Lib/test/test_capi/test_sys.py @@ -30,7 +30,11 @@ class CAPITest(unittest.TestCase): self.assertEqual(getobject('\U0001f40d'.encode()), 42) self.assertIs(getobject(b'nonexisting'), AttributeError) - self.assertIs(getobject(b'\xff'), AttributeError) + with support.catch_unraisable_exception() as cm: + self.assertIs(getobject(b'\xff'), AttributeError) + self.assertEqual(cm.unraisable.exc_type, UnicodeDecodeError) + self.assertRegex(str(cm.unraisable.exc_value), + "'utf-8' codec can't decode") # CRASHES getobject(NULL) @support.cpython_only diff --git a/Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst b/Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst new file mode 100644 index 00000000000..420f43175e5 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst @@ -0,0 +1,5 @@ +Functions :c:func:`PyDict_GetItem`, :c:func:`PyDict_GetItemString`, +:c:func:`PyMapping_HasKey`, :c:func:`PyMapping_HasKeyString`, +:c:func:`PyObject_HasAttr`, :c:func:`PyObject_HasAttrString`, and +:c:func:`PySys_GetObject`, which clear all errors occurred during calling +the function, report now them using :func:`sys.unraisablehook`. diff --git a/Objects/abstract.c b/Objects/abstract.c index 070e762c58a..b6762893b8f 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2468,31 +2468,53 @@ PyMapping_HasKeyWithError(PyObject *obj, PyObject *key) } int -PyMapping_HasKeyString(PyObject *o, const char *key) +PyMapping_HasKeyString(PyObject *obj, const char *key) { - PyObject *v; - - v = PyMapping_GetItemString(o, key); - if (v) { - Py_DECREF(v); - return 1; + PyObject *value; + int rc; + if (obj == NULL) { + // For backward compatibility. + // PyMapping_GetOptionalItemString() crashes if obj is NULL. + null_error(); + rc = -1; } - PyErr_Clear(); - return 0; + else { + rc = PyMapping_GetOptionalItemString(obj, key, &value); + } + if (rc < 0) { + PyErr_FormatUnraisable( + "Exception ignored in PyMapping_HasKeyString(); consider using " + "PyMapping_HasKeyStringWithError(), " + "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); + return 0; + } + Py_XDECREF(value); + return rc; } int -PyMapping_HasKey(PyObject *o, PyObject *key) +PyMapping_HasKey(PyObject *obj, PyObject *key) { - PyObject *v; - - v = PyObject_GetItem(o, key); - if (v) { - Py_DECREF(v); - return 1; + PyObject *value; + int rc; + if (obj == NULL || key == NULL) { + // For backward compatibility. + // PyMapping_GetOptionalItem() crashes if any of them is NULL. + null_error(); + rc = -1; } - PyErr_Clear(); - return 0; + else { + rc = PyMapping_GetOptionalItem(obj, key, &value); + } + if (rc < 0) { + PyErr_FormatUnraisable( + "Exception ignored in PyMapping_HasKey(); consider using " + "PyMapping_HasKeyWithError(), " + "PyMapping_GetOptionalItem() or PyObject_GetItem()"); + return 0; + } + Py_XDECREF(value); + return rc; } /* This function is quite similar to PySequence_Fast(), but specialized to be diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 5aa2c0d027f..719d438897c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1663,8 +1663,8 @@ _PyDict_FromItems(PyObject *const *keys, Py_ssize_t keys_offset, * function hits a stack-depth error, which can cause this to return NULL * even if the key is present. */ -PyObject * -PyDict_GetItem(PyObject *op, PyObject *key) +static PyObject * +dict_getitem(PyObject *op, PyObject *key, const char *warnmsg) { if (!PyDict_Check(op)) { return NULL; @@ -1675,7 +1675,7 @@ PyDict_GetItem(PyObject *op, PyObject *key) if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) { hash = PyObject_Hash(key); if (hash == -1) { - PyErr_Clear(); + PyErr_FormatUnraisable(warnmsg); return NULL; } } @@ -1696,12 +1696,24 @@ PyDict_GetItem(PyObject *op, PyObject *key) ix = _Py_dict_lookup(mp, key, hash, &value); /* Ignore any exception raised by the lookup */ + PyObject *exc2 = _PyErr_Occurred(tstate); + if (exc2 && !PyErr_GivenExceptionMatches(exc2, PyExc_KeyError)) { + PyErr_FormatUnraisable(warnmsg); + } _PyErr_SetRaisedException(tstate, exc); assert(ix >= 0 || value == NULL); return value; // borrowed reference } +PyObject * +PyDict_GetItem(PyObject *op, PyObject *key) +{ + return dict_getitem(op, key, + "Exception ignored in PyDict_GetItem(); consider using " + "PyDict_GetItemRef() or PyDict_GetItemWithError()"); +} + Py_ssize_t _PyDict_LookupIndex(PyDictObject *mp, PyObject *key) { @@ -3925,10 +3937,14 @@ PyDict_GetItemString(PyObject *v, const char *key) PyObject *kv, *rv; kv = PyUnicode_FromString(key); if (kv == NULL) { - PyErr_Clear(); + PyErr_FormatUnraisable( + "Exception ignored in PyDict_GetItemString(); consider using " + "PyDict_GetItemRefString()"); return NULL; } - rv = PyDict_GetItem(v, kv); + rv = dict_getitem(v, kv, + "Exception ignored in PyDict_GetItemString(); consider using " + "PyDict_GetItemRefString()"); Py_DECREF(kv); return rv; // borrowed reference } diff --git a/Objects/object.c b/Objects/object.c index 1cc3f914646..b561da7fca3 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1040,7 +1040,10 @@ PyObject_HasAttrString(PyObject *obj, const char *name) { int rc = PyObject_HasAttrStringWithError(obj, name); if (rc < 0) { - PyErr_Clear(); + PyErr_FormatUnraisable( + "Exception ignored in PyObject_HasAttrString(); consider using " + "PyObject_HasAttrStringWithError(), " + "PyObject_GetOptionalAttrString() or PyObject_GetAttrString()"); return 0; } return rc; @@ -1275,7 +1278,10 @@ PyObject_HasAttr(PyObject *obj, PyObject *name) { int rc = PyObject_HasAttrWithError(obj, name); if (rc < 0) { - PyErr_Clear(); + PyErr_FormatUnraisable( + "Exception ignored in PyObject_HasAttr(); consider using " + "PyObject_HasAttrWithError(), " + "PyObject_GetOptionalAttr() or PyObject_GetAttr()"); return 0; } return rc; diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 4008a28ad7b..e28523284f1 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -110,6 +110,9 @@ PySys_GetObject(const char *name) PyObject *value = _PySys_GetObject(tstate->interp, name); /* XXX Suppress a new exception if it was raised and restore * the old one. */ + if (_PyErr_Occurred(tstate)) { + PyErr_FormatUnraisable("Exception ignored in PySys_GetObject()"); + } _PyErr_SetRaisedException(tstate, exc); return value; }