Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vstinner wants to merge 1 commit into
base: main
Choose a base branch
from
+124 −32
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions Lib/test/test_capi/test_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import contextlib
import unittest
from collections import OrderedDict, UserDict
from types import MappingProxyType
Expand Down Expand Up @@ -258,6 +259,12 @@ def test_dict_contains_string(self):
# 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
Expand All @@ -269,7 +276,10 @@ def test_dict_setitem(self):
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)
Expand All @@ -286,7 +296,10 @@ def test_dict_setitemstring(self):
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)
Expand All @@ -304,7 +317,10 @@ def test_dict_delitem(self):
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')
Expand All @@ -323,7 +339,10 @@ def test_dict_delitemstring(self):
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')
Expand All @@ -341,7 +360,10 @@ def test_dict_setdefault(self):
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)
Expand All @@ -358,7 +380,10 @@ def test_dict_setdefaultref(self):
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)
Expand Down Expand Up @@ -424,7 +449,10 @@ def test_dict_update(self):

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, {})
Expand All @@ -443,7 +471,10 @@ def test_dict_merge(self):

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)
Expand All @@ -464,7 +495,10 @@ def test_dict_mergefromseq2(self):
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)
Expand Down Expand Up @@ -511,7 +545,10 @@ def test_dict_pop(self):
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")
Expand Down Expand Up @@ -560,7 +597,10 @@ def test_dict_popstring(self):
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")
Expand Down
94 changes: 73 additions & 21 deletions Objects/dictobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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));
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -4596,20 +4633,25 @@ 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;
}
return -1;
}
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);
Expand Down Expand Up @@ -7122,7 +7164,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;
}

Expand Down
Loading