gh-141510: Raise TypeError in PyDict_SetItem() on frozendict (#145564)

If the following functions get an unexpected frozendict,
raise TypeError instead of SystemError:

* PyDict_DelItem()
* PyDict_DelItemString()
* PyDict_Merge()
* PyDict_MergeFromSeq2()
* PyDict_Pop()
* PyDict_PopString()
* PyDict_SetDefault()
* PyDict_SetDefaultRef()
* PyDict_SetItem()
* PyDict_SetItemString()
* _PyDict_SetItem_KnownHash()
* PyDict_Update()

Co-authored-by: mohsinm-dev <mohsin.mdev@gmail.com>

diff --git a/Lib/test/test_capi/test_dict.py b/Lib/test/test_capi/test_dict.py
index cd46fea5476ca6d45dbb3850095d561d550b4467..4cf404e9f563272d41073e5fb7678028f47dc002 100644 (file)
--- a/Lib/test/test_capi/test_dict.py
+++ b/Lib/test/test_capi/test_dict.py
@@ -1,3 +1,4 @@
+import contextlib
 import unittest
 from collections import OrderedDict, UserDict
 from types import MappingProxyType
@@ -258,6 +259,12 @@ class CAPITest(unittest.TestCase):
         # CRASHES contains({}, NULL)
         # CRASHES contains(NULL, b'a')
 
+    @contextlib.contextmanager
+    def frozendict_does_not_support(self, what):
+        errmsg = f'frozendict object does not support item {what}'
+        with self.assertRaisesRegex(TypeError, errmsg):
+            yield
+
     def test_dict_setitem(self):
         # Test PyDict_SetItem()
         setitem = _testlimitedcapi.dict_setitem
@@ -269,7 +276,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'a': 5, '\U0001f40d': 8})
 
         self.assertRaises(TypeError, setitem, {}, [], 5)  # unhashable
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                setitem(test_type(), 'a', 5)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, setitem, test_type(), 'a', 5)
         # CRASHES setitem({}, NULL, 5)
         # CRASHES setitem({}, 'a', NULL)
@@ -286,7 +296,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'a': 5, '\U0001f40d': 8})
 
         self.assertRaises(UnicodeDecodeError, setitemstring, {}, INVALID_UTF8, 5)
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                setitemstring(test_type(), b'a', 5)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, setitemstring, test_type(), b'a', 5)
         # CRASHES setitemstring({}, NULL, 5)
         # CRASHES setitemstring({}, b'a', NULL)
@@ -304,7 +317,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'c': 2})
 
         self.assertRaises(TypeError, delitem, {}, [])  # unhashable
-        for test_type in NOT_DICT_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('deletion'):
+                delitem(test_type({'a': 1}), 'a')
+        for test_type in MAPPING_TYPES:
             self.assertRaises(SystemError, delitem, test_type({'a': 1}), 'a')
         for test_type in OTHER_TYPES:
             self.assertRaises(SystemError, delitem, test_type(), 'a')
@@ -323,7 +339,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'c': 2})
 
         self.assertRaises(UnicodeDecodeError, delitemstring, {}, INVALID_UTF8)
-        for test_type in NOT_DICT_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('deletion'):
+                delitemstring(test_type({'a': 1}), b'a')
+        for test_type in MAPPING_TYPES:
             self.assertRaises(SystemError, delitemstring, test_type({'a': 1}), b'a')
         for test_type in OTHER_TYPES:
             self.assertRaises(SystemError, delitemstring, test_type(), b'a')
@@ -341,7 +360,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'a': 5})
 
         self.assertRaises(TypeError, setdefault, {}, [], 5)  # unhashable
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                setdefault(test_type(), 'a', 5)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, setdefault, test_type(), 'a', 5)
         # CRASHES setdefault({}, NULL, 5)
         # CRASHES setdefault({}, 'a', NULL)
@@ -358,7 +380,10 @@ class CAPITest(unittest.TestCase):
             self.assertEqual(dct, {'a': 5})
 
         self.assertRaises(TypeError, setdefault, {}, [], 5)  # unhashable
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                setdefault(test_type(), 'a', 5)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, setdefault, test_type(), 'a', 5)
         # CRASHES setdefault({}, NULL, 5)
         # CRASHES setdefault({}, 'a', NULL)
@@ -424,7 +449,10 @@ class CAPITest(unittest.TestCase):
 
         self.assertRaises(AttributeError, update, {}, [])
         self.assertRaises(AttributeError, update, {}, 42)
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                update(test_type(), {})
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, update, test_type(), {})
         self.assertRaises(SystemError, update, {}, NULL)
         self.assertRaises(SystemError, update, NULL, {})
@@ -443,7 +471,10 @@ class CAPITest(unittest.TestCase):
 
         self.assertRaises(AttributeError, merge, {}, [], 0)
         self.assertRaises(AttributeError, merge, {}, 42, 0)
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                merge(test_type(), {}, 0)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, merge, test_type(), {}, 0)
         self.assertRaises(SystemError, merge, {}, NULL, 0)
         self.assertRaises(SystemError, merge, NULL, {}, 0)
@@ -464,7 +495,10 @@ class CAPITest(unittest.TestCase):
         self.assertRaises(ValueError, mergefromseq2, {}, [(1, 2, 3)], 0)
         self.assertRaises(TypeError, mergefromseq2, {}, [1], 0)
         self.assertRaises(TypeError, mergefromseq2, {}, 42, 0)
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('assignment'):
+                mergefromseq2(test_type(), [], 0)
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             self.assertRaises(SystemError, mergefromseq2, test_type(), [], 0)
         # CRASHES mergefromseq2({}, NULL, 0)
         # CRASHES mergefromseq2(NULL, {}, 0)
@@ -511,7 +545,10 @@ class CAPITest(unittest.TestCase):
                 dict_pop(mydict, not_hashable_key)
 
         # wrong dict type
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('deletion'):
+                dict_pop(test_type(), "key")
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             not_dict = test_type()
             self.assertRaises(SystemError, dict_pop, not_dict, "key")
             self.assertRaises(SystemError, dict_pop_null, not_dict, "key")
@@ -560,7 +597,10 @@ class CAPITest(unittest.TestCase):
             self.assertRaises(UnicodeDecodeError, dict_popstring_null, mydict, INVALID_UTF8)
 
         # wrong dict type
-        for test_type in NOT_DICT_TYPES + OTHER_TYPES:
+        for test_type in FROZENDICT_TYPES:
+            with self.frozendict_does_not_support('deletion'):
+                dict_popstring(test_type(), "key")
+        for test_type in MAPPING_TYPES + OTHER_TYPES:
             not_dict = test_type()
             self.assertRaises(SystemError, dict_popstring, not_dict, "key")
             self.assertRaises(SystemError, dict_popstring_null, not_dict, "key")

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index b5f2a682c549821672bb78acc7f084926378b4c4..842d9be73b8792047c6cf28d2d88bf22c5b6e412 100644 (file)
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -2719,6 +2719,10 @@ _PyDict_LoadBuiltinsFromGlobals(PyObject *globals)
     return builtins;
 }
 
+#define frozendict_does_not_support(WHAT) \
+    PyErr_SetString(PyExc_TypeError, "frozendict object does " \
+                    "not support item " WHAT)
+
 /* Consumes references to key and value */
 static int
 setitem_take2_lock_held(PyDictObject *mp, PyObject *key, PyObject *value)
@@ -2762,12 +2766,19 @@ _PyDict_SetItem_Take2(PyDictObject *mp, PyObject *key, PyObject *value)
 int
 PyDict_SetItem(PyObject *op, PyObject *key, PyObject *value)
 {
+    assert(key);
+    assert(value);
+
     if (!PyDict_Check(op)) {
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(op)) {
+            frozendict_does_not_support("assignment");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }
-    assert(key);
-    assert(value);
+
     return _PyDict_SetItem_Take2((PyDictObject *)op,
                                  Py_NewRef(key), Py_NewRef(value));
 }
@@ -2807,14 +2818,20 @@ int
 _PyDict_SetItem_KnownHash(PyObject *op, PyObject *key, PyObject *value,
                           Py_hash_t hash)
 {
-    if (!PyDict_Check(op)) {
-        PyErr_BadInternalCall();
-        return -1;
-    }
     assert(key);
     assert(value);
     assert(hash != -1);
 
+    if (!PyDict_Check(op)) {
+        if (PyFrozenDict_Check(op)) {
+            frozendict_does_not_support("assignment");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
+        return -1;
+    }
+
     int res;
     Py_BEGIN_CRITICAL_SECTION(op);
     res = _PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)op, key, value, hash);
@@ -2899,13 +2916,18 @@ PyDict_DelItem(PyObject *op, PyObject *key)
 int
 _PyDict_DelItem_KnownHash_LockHeld(PyObject *op, PyObject *key, Py_hash_t hash)
 {
-    Py_ssize_t ix;
-    PyObject *old_value;
-
     if (!PyDict_Check(op)) {
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(op)) {
+            frozendict_does_not_support("deletion");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }
+
+    Py_ssize_t ix;
+    PyObject *old_value;
     PyDictObject *mp = (PyDictObject *)op;
     assert(can_modify_dict(mp));
 
@@ -3206,7 +3228,12 @@ pop_lock_held(PyObject *op, PyObject *key, PyObject **result)
         if (result) {
             *result = NULL;
         }
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(op)) {
+            frozendict_does_not_support("deletion");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }
     PyDictObject *dict = (PyDictObject *)op;
@@ -4017,7 +4044,12 @@ PyDict_MergeFromSeq2(PyObject *d, PyObject *seq2, int override)
     assert(d != NULL);
     assert(seq2 != NULL);
     if (!PyDict_Check(d)) {
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(d)) {
+            frozendict_does_not_support("assignment");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }
 
@@ -4220,7 +4252,12 @@ dict_merge_api(PyObject *a, PyObject *b, int override)
      * PyMapping_Keys() and PyObject_GetItem() be supported.
      */
     if (a == NULL || !PyDict_Check(a) || b == NULL) {
-        PyErr_BadInternalCall();
+        if (a != NULL && PyFrozenDict_Check(a)) {
+            frozendict_does_not_support("assignment");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }
     return dict_merge(a, b, override);
@@ -4596,13 +4633,13 @@ static int
 dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_value,
                     PyObject **result, int incref_result)
 {
-    PyDictObject *mp = (PyDictObject *)d;
-    PyObject *value;
-    Py_hash_t hash;
-    Py_ssize_t ix;
-
     if (!PyDict_Check(d)) {
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(d)) {
+            frozendict_does_not_support("assignment");
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         if (result) {
             *result = NULL;
         }
@@ -4610,6 +4647,11 @@ dict_setdefault_ref_lock_held(PyObject *d, PyObject *key, PyObject *default_valu
     }
     assert(can_modify_dict((PyDictObject*)d));
 
+    PyDictObject *mp = (PyDictObject *)d;
+    PyObject *value;
+    Py_hash_t hash;
+    Py_ssize_t ix;
+
     hash = _PyObject_HashFast(key);
     if (hash == -1) {
         dict_unhashable_type(d, key);
@@ -7154,7 +7196,17 @@ int
 _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value)
 {
     if (!PyDict_Check(dict)) {
-        PyErr_BadInternalCall();
+        if (PyFrozenDict_Check(dict)) {
+            if (value == NULL) {
+                frozendict_does_not_support("deletion");
+            }
+            else {
+                frozendict_does_not_support("assignment");
+            }
+        }
+        else {
+            PyErr_BadInternalCall();
+        }
         return -1;
     }