From e134bade02e2af868e0b9c9eaf45fc7a6ad02d28 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 12 Jul 2023 12:15:26 +0300 Subject: [PATCH 01/12] gh-106672: C API: Report indiscriminately ignored errors Functions which indiscriminately ignore all errors now report them as unraisable errors. --- Lib/test/test_capi/test_misc.py | 89 ++++++++++++++++--- ...-07-12-12-14-52.gh-issue-106672.fkRjmi.rst | 5 ++ Modules/_testcapimodule.c | 34 +++---- Objects/abstract.c | 42 +++++---- Objects/dictobject.c | 8 +- Objects/object.c | 40 +++------ Python/sysmodule.c | 3 + 7 files changed, 142 insertions(+), 79 deletions(-) create mode 100644 Misc/NEWS.d/next/C API/2023-07-12-12-14-52.gh-issue-106672.fkRjmi.rst diff --git a/Lib/test/test_capi/test_misc.py b/Lib/test/test_capi/test_misc.py index 9c14a501875b6d..a2c66617646619 100644 --- a/Lib/test/test_capi/test_misc.py +++ b/Lib/test/test_capi/test_misc.py @@ -51,6 +51,8 @@ import _testinternalcapi +NULL = None + def decode_stderr(err): return err.decode('utf-8', 'replace').replace('\r', '') @@ -338,16 +340,83 @@ def items(self): self.assertRaises(TypeError, _testcapi.get_mapping_items, bad_mapping) def test_mapping_has_key(self): - dct = {'a': 1} - self.assertTrue(_testcapi.mapping_has_key(dct, 'a')) - self.assertFalse(_testcapi.mapping_has_key(dct, 'b')) - - class SubDict(dict): - pass - - dct2 = SubDict({'a': 1}) - self.assertTrue(_testcapi.mapping_has_key(dct2, 'a')) - self.assertFalse(_testcapi.mapping_has_key(dct2, 'b')) + has_key = _testcapi.mapping_has_key + dct = {'a': 1, '\U0001f40d': 2} + self.assertTrue(has_key(dct, 'a')) + self.assertFalse(has_key(dct, 'b')) + self.assertTrue(has_key(dct, '\U0001f40d')) + + class M: + def __getitem__(self, key): + return dct[key] + + dct2 = M() + self.assertTrue(has_key(dct2, 'a')) + self.assertFalse(has_key(dct2, 'b')) + + with support.catch_unraisable_exception() as cm: + self.assertFalse(has_key(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(has_key({}, [])) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + "unhashable type: 'list'") + + self.assertTrue(has_key(['a', 'b'], 1)) + with support.catch_unraisable_exception() as cm: + self.assertFalse(has_key([], 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(has_key([], 'a')) + self.assertEqual(cm.unraisable.exc_type, TypeError) + self.assertEqual(str(cm.unraisable.exc_value), + 'list indices must be integers or slices, not str') + + def test_mapping_has_key_string(self): + has_key_string = _testcapi.mapping_has_key_string + dct = {'a': 1, '\U0001f40d': 2} + self.assertTrue(has_key_string(dct, b'a')) + self.assertFalse(has_key_string(dct, b'b')) + self.assertTrue(has_key_string(dct, '\U0001f40d'.encode())) + + class M: + def __getitem__(self, key): + return dct[key] + + dct2 = M() + self.assertTrue(has_key_string(dct2, b'a')) + self.assertFalse(has_key_string(dct2, b'b')) + + with support.catch_unraisable_exception() as cm: + self.assertFalse(has_key_string(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(has_key_string({}, 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(has_key_string({}, 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(has_key_string([], 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') def test_sequence_set_slice(self): # Correct case: 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 00000000000000..420f43175e595a --- /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/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index dd2c9c72e53787..29575bc779bdbe 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2078,37 +2078,25 @@ get_mapping_items(PyObject* self, PyObject *obj) } static PyObject * -test_mapping_has_key_string(PyObject *self, PyObject *Py_UNUSED(args)) +mapping_has_key(PyObject* self, PyObject *args) { - PyObject *context = PyDict_New(); - PyObject *val = PyLong_FromLong(1); - - // Since this uses `const char*` it is easier to test this in C: - PyDict_SetItemString(context, "a", val); - if (!PyMapping_HasKeyString(context, "a")) { - PyErr_SetString(PyExc_RuntimeError, - "Existing mapping key does not exist"); - return NULL; - } - if (PyMapping_HasKeyString(context, "b")) { - PyErr_SetString(PyExc_RuntimeError, - "Missing mapping key exists"); + PyObject *context, *key; + if (!PyArg_ParseTuple(args, "OO", &context, &key)) { return NULL; } - - Py_DECREF(val); - Py_DECREF(context); - Py_RETURN_NONE; + return PyLong_FromLong(PyMapping_HasKey(context, key)); } static PyObject * -mapping_has_key(PyObject* self, PyObject *args) +mapping_has_key_string(PyObject* self, PyObject *args) { - PyObject *context, *key; - if (!PyArg_ParseTuple(args, "OO", &context, &key)) { + PyObject *context; + const char *key; + Py_ssize_t size; + if (!PyArg_ParseTuple(args, "Oz#", &context, &key, &size)) { return NULL; } - return PyLong_FromLong(PyMapping_HasKey(context, key)); + return PyLong_FromLong(PyMapping_HasKeyString(context, key)); } static PyObject * @@ -3548,8 +3536,8 @@ static PyMethodDef TestMethods[] = { {"get_mapping_keys", get_mapping_keys, METH_O}, {"get_mapping_values", get_mapping_values, METH_O}, {"get_mapping_items", get_mapping_items, METH_O}, - {"test_mapping_has_key_string", test_mapping_has_key_string, METH_NOARGS}, {"mapping_has_key", mapping_has_key, METH_VARARGS}, + {"mapping_has_key_string", mapping_has_key_string, METH_VARARGS}, {"sequence_set_slice", sequence_set_slice, METH_VARARGS}, {"sequence_del_slice", sequence_del_slice, METH_VARARGS}, {"test_pythread_tss_key_state", test_pythread_tss_key_state, METH_VARARGS}, diff --git a/Objects/abstract.c b/Objects/abstract.c index b4edcec6007710..4b913f17517a63 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2426,31 +2426,37 @@ PyMapping_SetItemString(PyObject *o, const char *key, PyObject *value) } 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 *dummy; + int rc = PyMapping_GetOptionalItemString(obj, key, &dummy); + if (rc < 0) { + _PyErr_WriteUnraisableMsg("on testing a mapping key", obj); + return 0; } - PyErr_Clear(); - return 0; + if (rc == 0 && PyErr_Occurred()) { + _PyErr_WriteUnraisableMsg("before testing a mapping key", obj); + return 0; + } + Py_XDECREF(dummy); + 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 *dummy; + int rc = PyMapping_GetOptionalItem(obj, key, &dummy); + if (rc < 0) { + _PyErr_WriteUnraisableMsg("on testing a mapping key", obj); + return 0; } - PyErr_Clear(); - return 0; + if (rc == 0 && PyErr_Occurred()) { + _PyErr_WriteUnraisableMsg("before testing a mapping key", obj); + return 0; + } + Py_XDECREF(dummy); + 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 013c21884032aa..5ee41060f44879 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1673,7 +1673,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_WriteUnraisableMsg("on getting a dict key", op); return NULL; } } @@ -1694,6 +1694,10 @@ 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_WriteUnraisableMsg("on getting a dict key", op); + } _PyErr_SetRaisedException(tstate, exc); @@ -3889,7 +3893,7 @@ PyDict_GetItemString(PyObject *v, const char *key) PyObject *kv, *rv; kv = PyUnicode_FromString(key); if (kv == NULL) { - PyErr_Clear(); + _PyErr_WriteUnraisableMsg("on getting a dict key", v); return NULL; } rv = PyDict_GetItem(v, kv); diff --git a/Objects/object.c b/Objects/object.c index d30e048335ab63..7e079b7dbfe30a 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -904,26 +904,16 @@ PyObject_GetAttrString(PyObject *v, const char *name) } int -PyObject_HasAttrString(PyObject *v, const char *name) +PyObject_HasAttrString(PyObject *obj, const char *name) { - if (Py_TYPE(v)->tp_getattr != NULL) { - PyObject *res = (*Py_TYPE(v)->tp_getattr)(v, (char*)name); - if (res != NULL) { - Py_DECREF(res); - return 1; - } - PyErr_Clear(); - return 0; - } - - PyObject *attr_name = PyUnicode_FromString(name); - if (attr_name == NULL) { - PyErr_Clear(); + PyObject *dummy; + int rc = PyObject_GetOptionalAttrString(obj, name, &dummy); + if (rc < 0) { + _PyErr_WriteUnraisableMsg("on testing an object attribute", obj); return 0; } - int ok = PyObject_HasAttr(v, attr_name); - Py_DECREF(attr_name); - return ok; + Py_XDECREF(dummy); + return rc; } int @@ -1142,18 +1132,16 @@ PyObject_GetOptionalAttrString(PyObject *obj, const char *name, PyObject **resul } int -PyObject_HasAttr(PyObject *v, PyObject *name) +PyObject_HasAttr(PyObject *obj, PyObject *name) { - PyObject *res; - if (PyObject_GetOptionalAttr(v, name, &res) < 0) { - PyErr_Clear(); + PyObject *dummy; + int rc = PyObject_GetOptionalAttr(obj, name, &dummy); + if (rc < 0) { + _PyErr_WriteUnraisableMsg("on testing an object attribute", obj); return 0; } - if (res == NULL) { - return 0; - } - Py_DECREF(res); - return 1; + Py_XDECREF(dummy); + return rc; } int diff --git a/Python/sysmodule.c b/Python/sysmodule.c index fea3f61ee01762..9df0b3aaeb3371 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -98,6 +98,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_WriteUnraisableMsg("on getting the sys module attribute", NULL); + } _PyErr_SetRaisedException(tstate, exc); return value; } From 00ce038a31acc88b11b11e33c3fbc3dd0172d28b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 12 Jul 2023 18:42:59 +0300 Subject: [PATCH 02/12] Update output messsages. --- Objects/abstract.c | 22 ++++++++++++++++++---- Objects/dictobject.c | 18 ++++++++++++++---- Objects/object.c | 9 +++++++-- Python/sysmodule.c | 2 +- 4 files changed, 40 insertions(+), 11 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index 4b913f17517a63..90060989d28858 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2431,11 +2431,19 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) PyObject *dummy; int rc = PyMapping_GetOptionalItemString(obj, key, &dummy); if (rc < 0) { - _PyErr_WriteUnraisableMsg("on testing a mapping key", obj); + _PyErr_WriteUnraisableMsg( + "in PyMapping_HasKeyString(); consider using " + "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()", + NULL); return 0; } + // PyMapping_HasKeyString() also clears the error set before it's call + // if the key is not found. if (rc == 0 && PyErr_Occurred()) { - _PyErr_WriteUnraisableMsg("before testing a mapping key", obj); + _PyErr_WriteUnraisableMsg( + "in PyMapping_HasKeyString(); consider using " + "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()", + NULL); return 0; } Py_XDECREF(dummy); @@ -2448,11 +2456,17 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) PyObject *dummy; int rc = PyMapping_GetOptionalItem(obj, key, &dummy); if (rc < 0) { - _PyErr_WriteUnraisableMsg("on testing a mapping key", obj); + _PyErr_WriteUnraisableMsg( + "in PyMapping_HasKey(); consider using " + "PyMapping_GetOptionalItem() or PyObject_GetItem()", NULL); return 0; } + // PyMapping_HasKey() also clears the error set before it's call + // if the key is not found. if (rc == 0 && PyErr_Occurred()) { - _PyErr_WriteUnraisableMsg("before testing a mapping key", obj); + _PyErr_WriteUnraisableMsg( + "in PyMapping_HasKey(); consider using " + "PyMapping_GetOptionalItem() or PyObject_GetItem()", NULL); return 0; } Py_XDECREF(dummy); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 5ee41060f44879..b5f41882278346 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1662,7 +1662,7 @@ _PyDict_FromItems(PyObject *const *keys, Py_ssize_t keys_offset, * even if the key is present. */ PyObject * -PyDict_GetItem(PyObject *op, PyObject *key) +dict_getitem(PyObject *op, PyObject *key, const char *warnmsg) { if (!PyDict_Check(op)) { return NULL; @@ -1673,7 +1673,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_WriteUnraisableMsg("on getting a dict key", op); + _PyErr_WriteUnraisableMsg(warnmsg, NULL); return NULL; } } @@ -1696,7 +1696,7 @@ PyDict_GetItem(PyObject *op, PyObject *key) /* Ignore any exception raised by the lookup */ PyObject *exc2 = _PyErr_Occurred(tstate); if (exc2 && !PyErr_GivenExceptionMatches(exc2, PyExc_KeyError)) { - _PyErr_WriteUnraisableMsg("on getting a dict key", op); + _PyErr_WriteUnraisableMsg(warnmsg, NULL); } _PyErr_SetRaisedException(tstate, exc); @@ -1705,6 +1705,14 @@ PyDict_GetItem(PyObject *op, PyObject *key) return value; } +PyObject * +PyDict_GetItem(PyObject *op, PyObject *key) +{ + return dict_getitem(op, key, + "in PyDict_GetItem(); consider using " + "PyDict_GetItemWithError()"); +} + Py_ssize_t _PyDict_LookupIndex(PyDictObject *mp, PyObject *key) { @@ -3893,10 +3901,12 @@ PyDict_GetItemString(PyObject *v, const char *key) PyObject *kv, *rv; kv = PyUnicode_FromString(key); if (kv == NULL) { - _PyErr_WriteUnraisableMsg("on getting a dict key", v); + _PyErr_WriteUnraisableMsg( + "in PyDict_GetItemString()", NULL); return NULL; } rv = PyDict_GetItem(v, kv); + rv = dict_getitem(v, kv, "in PyDict_GetItemString()"); Py_DECREF(kv); return rv; } diff --git a/Objects/object.c b/Objects/object.c index 7e079b7dbfe30a..6f250037347b6f 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -909,7 +909,10 @@ PyObject_HasAttrString(PyObject *obj, const char *name) PyObject *dummy; int rc = PyObject_GetOptionalAttrString(obj, name, &dummy); if (rc < 0) { - _PyErr_WriteUnraisableMsg("on testing an object attribute", obj); + _PyErr_WriteUnraisableMsg( + "in PyObject_HasAttrString(); consider using " + "PyObject_GetOptionalAttrString() or PyObject_GetAttrString()", + NULL); return 0; } Py_XDECREF(dummy); @@ -1137,7 +1140,9 @@ PyObject_HasAttr(PyObject *obj, PyObject *name) PyObject *dummy; int rc = PyObject_GetOptionalAttr(obj, name, &dummy); if (rc < 0) { - _PyErr_WriteUnraisableMsg("on testing an object attribute", obj); + _PyErr_WriteUnraisableMsg( + "in PyObject_HasAttr(); consider using " + "PyObject_GetOptionalAttr() or PyObject_GetAttr()", NULL); return 0; } Py_XDECREF(dummy); diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 9df0b3aaeb3371..98da7660685ff9 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -99,7 +99,7 @@ PySys_GetObject(const char *name) /* XXX Suppress a new exception if it was raised and restore * the old one. */ if (_PyErr_Occurred(tstate)) { - _PyErr_WriteUnraisableMsg("on getting the sys module attribute", NULL); + _PyErr_WriteUnraisableMsg("in PySys_GetObject()", NULL); } _PyErr_SetRaisedException(tstate, exc); return value; From 168f92af5667bdd57f9b075b7f0537ca5a9a25d9 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 12 Jul 2023 18:50:33 +0300 Subject: [PATCH 03/12] Add an entry in What's New. --- Doc/whatsnew/3.13.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index dc020682ac1edd..cc93df7c2ab016 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -700,6 +700,15 @@ that may require changes to your code. Note that ``Py_TRASHCAN_BEGIN`` has a second argument which should be the deallocation function it is in. +* 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 ============= From 07c8250340bdd5d18ed6f5fbb138a1e60c4fa906 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 7 Aug 2023 21:53:28 +0300 Subject: [PATCH 04/12] Fix smelly symbol. --- Objects/dictobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index ca78f45cb197b3..1f281ab0b2c410 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1662,7 +1662,7 @@ _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 * +static PyObject * dict_getitem(PyObject *op, PyObject *key, const char *warnmsg) { if (!PyDict_Check(op)) { From 56e3a493a9a4abb15d654b9dfb08b8715de7a8b8 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 7 Aug 2023 21:57:23 +0300 Subject: [PATCH 05/12] Remove PyDict_GetItem(dict, invalid_key) from test_dict_capi(). --- Modules/_testcapimodule.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 23dd2cc9ad40f5..af37c7a1198534 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3454,10 +3454,6 @@ test_dict_capi(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args)) assert(PyErr_ExceptionMatches(PyExc_TypeError)); PyErr_Clear(); - // test PyDict_GetItem(), invalid key - assert(PyDict_GetItem(dict, invalid_key) == NULL); - assert(!PyErr_Occurred()); - // test PyDict_GetItemWithError(), invalid key assert(PyDict_GetItemWithError(dict, invalid_key) == NULL); assert(PyErr_ExceptionMatches(PyExc_TypeError)); From b5a661bbd929bb3d066e56c56f1825eb6c187aed Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 7 Aug 2023 23:03:21 +0300 Subject: [PATCH 06/12] Fix regressions in handling NULLs. --- Lib/test/test_capi/test_abstract.py | 19 ++++++++++++++++--- Objects/abstract.c | 22 ++++++++++++++++++++-- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_capi/test_abstract.py b/Lib/test/test_capi/test_abstract.py index da6e54dcc969e6..51a86413628c5b 100644 --- a/Lib/test/test_capi/test_abstract.py +++ b/Lib/test/test_capi/test_abstract.py @@ -322,8 +322,17 @@ def test_mapping_haskey(self): self.assertEqual(str(cm.unraisable.exc_value), 'list indices must be integers or slices, not str') - # CRASHES haskey({}, NULL) - # CRASHES haskey(NULL, 'a') + 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,7 +369,11 @@ def test_mapping_haskeystring(self): self.assertEqual(str(cm.unraisable.exc_value), 'list indices must be integers or slices, not str') - # CRASHES haskeystring(NULL, b'a') + 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_object_setitem(self): setitem = _testcapi.object_setitem diff --git a/Objects/abstract.c b/Objects/abstract.c index 90060989d28858..88a51001590981 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2429,7 +2429,16 @@ int PyMapping_HasKeyString(PyObject *obj, const char *key) { PyObject *dummy; - int rc = PyMapping_GetOptionalItemString(obj, key, &dummy); + int rc; + if (obj == NULL) { + // For backward compatibility. + // PyMapping_GetOptionalItemString() crashes if it is NULL. + null_error(); + rc = -1; + } + else { + rc = PyMapping_GetOptionalItemString(obj, key, &dummy); + } if (rc < 0) { _PyErr_WriteUnraisableMsg( "in PyMapping_HasKeyString(); consider using " @@ -2454,7 +2463,16 @@ int PyMapping_HasKey(PyObject *obj, PyObject *key) { PyObject *dummy; - int rc = PyMapping_GetOptionalItem(obj, key, &dummy); + int rc; + if (obj == NULL || key == NULL) { + // For backward compatibility. + // PyMapping_GetOptionalItem() crashes if any of them is NULL. + null_error(); + rc = -1; + } + else { + rc = PyMapping_GetOptionalItem(obj, key, &dummy); + } if (rc < 0) { _PyErr_WriteUnraisableMsg( "in PyMapping_HasKey(); consider using " From 82efd1cee9e6d8ec39f258ae52f3d7ececd3b4df Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 26 Aug 2023 10:34:39 +0300 Subject: [PATCH 07/12] Fix merging error. --- Lib/test/test_capi/test_dict.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_capi/test_dict.py b/Lib/test/test_capi/test_dict.py index c8b64b9a214828..18827c2433b6b2 100644 --- a/Lib/test/test_capi/test_dict.py +++ b/Lib/test/test_capi/test_dict.py @@ -144,12 +144,11 @@ def test_dict_getitemstring(self): self.assertIs(getitemstring(dct2, b'b'), KeyError) with support.catch_unraisable_exception() as cm: - self.assertIs(getitemstring({}, b'\xff'), KeyError) + 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({}, INVALID_UTF8), KeyError) self.assertIs(getitemstring(42, b'a'), KeyError) self.assertIs(getitemstring([], b'a'), KeyError) # CRASHES getitemstring({}, NULL) From 310ae5108eefa699498d60c2f35750881c403676 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 5 Nov 2023 11:05:44 +0200 Subject: [PATCH 08/12] Add more suggestions. --- Objects/abstract.c | 4 ++++ Objects/dictobject.c | 15 +++++++++------ Objects/object.c | 2 ++ 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index 4d4b0923919afc..a0826ef4aa364f 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2484,6 +2484,7 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) if (rc < 0) { PyErr_FormatUnraisable( "Exception ignored in PyMapping_HasKeyString(); consider using " + "PyMapping_HasKeyStringWithError(), " "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } @@ -2492,6 +2493,7 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) if (rc == 0 && PyErr_Occurred()) { PyErr_FormatUnraisable( "Exception ignored in PyMapping_HasKeyString(); consider using " + "PyMapping_HasKeyStringWithError(), " "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } @@ -2516,6 +2518,7 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) if (rc < 0) { PyErr_FormatUnraisable( "Exception ignored in PyMapping_HasKey(); consider using " + "PyMapping_HasKeyWithError(), " "PyMapping_GetOptionalItem() or PyObject_GetItem()"); return 0; } @@ -2524,6 +2527,7 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) if (rc == 0 && PyErr_Occurred()) { PyErr_FormatUnraisable( "Exception ignored in PyMapping_HasKey(); consider using " + "PyMapping_HasKeyWithError(), " "PyMapping_GetOptionalItem() or PyObject_GetItem()"); return 0; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 6b728e3e12e12f..5b7a5e12c4ded3 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1675,7 +1675,7 @@ dict_getitem(PyObject *op, PyObject *key, const char *warnmsg) if (!PyUnicode_CheckExact(key) || (hash = unicode_get_hash(key)) == -1) { hash = PyObject_Hash(key); if (hash == -1) { - PyErr_FormatUnraisable("Exception ignored %s", warnmsg); + PyErr_FormatUnraisable(warnmsg); return NULL; } } @@ -1698,7 +1698,7 @@ dict_getitem(PyObject *op, PyObject *key, const char *warnmsg) /* Ignore any exception raised by the lookup */ PyObject *exc2 = _PyErr_Occurred(tstate); if (exc2 && !PyErr_GivenExceptionMatches(exc2, PyExc_KeyError)) { - PyErr_FormatUnraisable("Exception ignored %s", warnmsg); + PyErr_FormatUnraisable(warnmsg); } _PyErr_SetRaisedException(tstate, exc); @@ -1710,8 +1710,8 @@ PyObject * PyDict_GetItem(PyObject *op, PyObject *key) { return dict_getitem(op, key, - "in PyDict_GetItem(); consider using " - "PyDict_GetItemWithError()"); + "Exception ignored in PyDict_GetItem(); consider using " + "PyDict_GetItemRef() or PyDict_GetItemWithError()"); } Py_ssize_t @@ -3938,11 +3938,14 @@ PyDict_GetItemString(PyObject *v, const char *key) kv = PyUnicode_FromString(key); if (kv == NULL) { PyErr_FormatUnraisable( - "Exception ignored in PyDict_GetItemString()"); + "Exception ignored in PyDict_GetItemString(); consider using " + "PyDict_GetItemRefString()"); return NULL; } rv = PyDict_GetItem(v, kv); - rv = dict_getitem(v, kv, "in PyDict_GetItemString()"); + 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 688f5bb85a8b41..b561da7fca3aa0 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -1042,6 +1042,7 @@ PyObject_HasAttrString(PyObject *obj, const char *name) if (rc < 0) { PyErr_FormatUnraisable( "Exception ignored in PyObject_HasAttrString(); consider using " + "PyObject_HasAttrStringWithError(), " "PyObject_GetOptionalAttrString() or PyObject_GetAttrString()"); return 0; } @@ -1279,6 +1280,7 @@ PyObject_HasAttr(PyObject *obj, PyObject *name) if (rc < 0) { PyErr_FormatUnraisable( "Exception ignored in PyObject_HasAttr(); consider using " + "PyObject_HasAttrWithError(), " "PyObject_GetOptionalAttr() or PyObject_GetAttr()"); return 0; } From 241576a570416648ed31727268a7d9dad217eae4 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 5 Nov 2023 11:32:32 +0200 Subject: [PATCH 09/12] Improve error messages for ignoring errors set before the calls. --- Objects/abstract.c | 8 ++++---- Objects/dictobject.c | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index a0826ef4aa364f..3bf5ef1ba582c1 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2492,8 +2492,8 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) // if the key is not found. if (rc == 0 && PyErr_Occurred()) { PyErr_FormatUnraisable( - "Exception ignored in PyMapping_HasKeyString(); consider using " - "PyMapping_HasKeyStringWithError(), " + "Ignore exception set before calling in PyMapping_HasKeyString(); " + "consider using PyMapping_HasKeyStringWithError(), " "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } @@ -2526,8 +2526,8 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) // if the key is not found. if (rc == 0 && PyErr_Occurred()) { PyErr_FormatUnraisable( - "Exception ignored in PyMapping_HasKey(); consider using " - "PyMapping_HasKeyWithError(), " + "Ignore exception set before calling in PyMapping_HasKey(); " + "consider using PyMapping_HasKeyWithError(), " "PyMapping_GetOptionalItem() or PyObject_GetItem()"); return 0; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 5b7a5e12c4ded3..719d438897ca6c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -3942,7 +3942,6 @@ PyDict_GetItemString(PyObject *v, const char *key) "PyDict_GetItemRefString()"); return NULL; } - rv = PyDict_GetItem(v, kv); rv = dict_getitem(v, kv, "Exception ignored in PyDict_GetItemString(); consider using " "PyDict_GetItemRefString()"); From fd50ea94e845ff093d5e5e475f7bcee12fdd5ab8 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 6 Nov 2023 17:44:40 +0200 Subject: [PATCH 10/12] Apply suggestions from code review Co-authored-by: Victor Stinner --- Objects/abstract.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index 3bf5ef1ba582c1..cec293c6706099 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2474,7 +2474,7 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) int rc; if (obj == NULL) { // For backward compatibility. - // PyMapping_GetOptionalItemString() crashes if it is NULL. + // PyMapping_GetOptionalItemString() crashes if obj is NULL. null_error(); rc = -1; } @@ -2488,7 +2488,7 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } - // PyMapping_HasKeyString() also clears the error set before it's call + // PyMapping_HasKeyString() also clears the error set before it's called // if the key is not found. if (rc == 0 && PyErr_Occurred()) { PyErr_FormatUnraisable( From 587b3ccd842317e3d097eb7ce63953b29b94fcb8 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 6 Nov 2023 17:50:59 +0200 Subject: [PATCH 11/12] Address review comments. --- Objects/abstract.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index cec293c6706099..62bc3a2469d870 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2470,7 +2470,7 @@ PyMapping_HasKeyWithError(PyObject *obj, PyObject *key) int PyMapping_HasKeyString(PyObject *obj, const char *key) { - PyObject *dummy; + PyObject *value; int rc; if (obj == NULL) { // For backward compatibility. @@ -2479,7 +2479,7 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) rc = -1; } else { - rc = PyMapping_GetOptionalItemString(obj, key, &dummy); + rc = PyMapping_GetOptionalItemString(obj, key, &value); } if (rc < 0) { PyErr_FormatUnraisable( @@ -2497,14 +2497,14 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } - Py_XDECREF(dummy); + Py_XDECREF(value); return rc; } int PyMapping_HasKey(PyObject *obj, PyObject *key) { - PyObject *dummy; + PyObject *value; int rc; if (obj == NULL || key == NULL) { // For backward compatibility. @@ -2513,7 +2513,7 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) rc = -1; } else { - rc = PyMapping_GetOptionalItem(obj, key, &dummy); + rc = PyMapping_GetOptionalItem(obj, key, &value); } if (rc < 0) { PyErr_FormatUnraisable( @@ -2531,7 +2531,7 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) "PyMapping_GetOptionalItem() or PyObject_GetItem()"); return 0; } - Py_XDECREF(dummy); + Py_XDECREF(value); return rc; } From bfb206a30aef4ba234412e763b371bc2da712783 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 6 Nov 2023 18:56:39 +0200 Subject: [PATCH 12/12] Remove non-existing cases. --- Objects/abstract.c | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/Objects/abstract.c b/Objects/abstract.c index 62bc3a2469d870..b6762893b8fd5d 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -2488,15 +2488,6 @@ PyMapping_HasKeyString(PyObject *obj, const char *key) "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); return 0; } - // PyMapping_HasKeyString() also clears the error set before it's called - // if the key is not found. - if (rc == 0 && PyErr_Occurred()) { - PyErr_FormatUnraisable( - "Ignore exception set before calling in PyMapping_HasKeyString(); " - "consider using PyMapping_HasKeyStringWithError(), " - "PyMapping_GetOptionalItemString() or PyMapping_GetItemString()"); - return 0; - } Py_XDECREF(value); return rc; } @@ -2522,15 +2513,6 @@ PyMapping_HasKey(PyObject *obj, PyObject *key) "PyMapping_GetOptionalItem() or PyObject_GetItem()"); return 0; } - // PyMapping_HasKey() also clears the error set before it's call - // if the key is not found. - if (rc == 0 && PyErr_Occurred()) { - PyErr_FormatUnraisable( - "Ignore exception set before calling in PyMapping_HasKey(); " - "consider using PyMapping_HasKeyWithError(), " - "PyMapping_GetOptionalItem() or PyObject_GetItem()"); - return 0; - } Py_XDECREF(value); return rc; }