-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-142831: Fix UAF in _json module
#142851
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
d69416d
b153c2a
64fcd75
e385958
211551a
5d6b2a8
a5a76d6
66c3af1
aa92e34
adfeee2
948ef90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,39 @@ def __lt__(self, o): | |
| d[1337] = "true.dat" | ||
| self.assertEqual(self.dumps(d, sort_keys=True), '{"1337": "true.dat"}') | ||
|
|
||
| def test_mutate_items_during_encode(self): | ||
| c_make_encoder = getattr(self.json.encoder, 'c_make_encoder', None) | ||
| if c_make_encoder is None: | ||
| self.skipTest("c_make_encoder not available") | ||
|
|
||
| cache = [] | ||
|
|
||
| class BadDict(dict): | ||
| def __init__(self): | ||
| super().__init__(real=1) | ||
|
Comment on lines
+76
to
+77
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is not needed, you can just pass arguments to the BadDict constructor at line 97. |
||
|
|
||
| def items(self): | ||
| entries = [("boom", object())] | ||
| cache.append(entries) | ||
| return entries | ||
|
|
||
| def encode_str(obj): | ||
| if cache: | ||
| cache.pop().clear() | ||
| return '"x"' | ||
|
|
||
| encoder = c_make_encoder( | ||
| None, lambda o: "null", | ||
| encode_str, None, | ||
| ": ", ", ", False, | ||
| False, True | ||
| ) | ||
|
|
||
| try: | ||
| encoder(BadDict(), 0) | ||
| except (ValueError, RuntimeError): | ||
| pass | ||
|
|
||
|
|
||
| class TestPyDump(TestDump, PyTest): pass | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix a crash in the :mod:`json` module where a use-after-free could occur if | ||
| the object being encoded is modified during serialization. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1733,15 +1733,14 @@ _encoder_iterate_mapping_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, | |
| PyObject *key, *value; | ||
| for (Py_ssize_t i = 0; i < PyList_GET_SIZE(items); i++) { | ||
| PyObject *item = PyList_GET_ITEM(items, i); | ||
| #ifdef Py_GIL_DISABLED | ||
| // gh-119438: in the free-threading build the critical section on items can get suspended | ||
|
|
||
| // GH-142831: The item must be strong-referenced to avoid | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is better to do without such comments. They are trivial and only distract from other code. Each Py_INCREF() has reasons, but we do not write it ot for them. |
||
| // use-after-free if the user code modifies the list during iteration. | ||
| Py_INCREF(item); | ||
| #endif | ||
|
|
||
| if (!PyTuple_Check(item) || PyTuple_GET_SIZE(item) != 2) { | ||
| PyErr_SetString(PyExc_ValueError, "items must return 2-tuples"); | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(item); | ||
| #endif | ||
| return -1; | ||
| } | ||
|
|
||
|
|
@@ -1750,14 +1749,10 @@ _encoder_iterate_mapping_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, | |
| if (encoder_encode_key_value(s, writer, first, dct, key, value, | ||
| indent_level, indent_cache, | ||
| separator) < 0) { | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(item); | ||
| #endif | ||
| return -1; | ||
| } | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(item); | ||
| #endif | ||
| } | ||
|
|
||
| return 0; | ||
|
|
@@ -1772,24 +1767,20 @@ _encoder_iterate_dict_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, | |
| PyObject *key, *value; | ||
| Py_ssize_t pos = 0; | ||
| while (PyDict_Next(dct, &pos, &key, &value)) { | ||
| #ifdef Py_GIL_DISABLED | ||
| // gh-119438: in the free-threading build the critical section on dct can get suspended | ||
| // GH-142831: The key and value must be strong-referenced to avoid | ||
| // use-after-free if the user code modifies the dict during iteration. | ||
| Py_INCREF(key); | ||
| Py_INCREF(value); | ||
| #endif | ||
|
|
||
| if (encoder_encode_key_value(s, writer, first, dct, key, value, | ||
| indent_level, indent_cache, | ||
| separator) < 0) { | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(key); | ||
| Py_DECREF(value); | ||
| #endif | ||
| return -1; | ||
| } | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(key); | ||
| Py_DECREF(value); | ||
| #endif | ||
| } | ||
| return 0; | ||
| } | ||
|
|
@@ -1893,28 +1884,23 @@ _encoder_iterate_fast_seq_lock_held(PyEncoderObject *s, PyUnicodeWriter *writer, | |
| { | ||
| for (Py_ssize_t i = 0; i < PySequence_Fast_GET_SIZE(s_fast); i++) { | ||
| PyObject *obj = PySequence_Fast_GET_ITEM(s_fast, i); | ||
| #ifdef Py_GIL_DISABLED | ||
| // gh-119438: in the free-threading build the critical section on s_fast can get suspended | ||
|
|
||
| // GH-142831: The object must be strong-referenced to avoid use-after-free | ||
| // if the user code modifies the sequence during iteration. | ||
| Py_INCREF(obj); | ||
| #endif | ||
|
|
||
| if (i) { | ||
| if (PyUnicodeWriter_WriteStr(writer, separator) < 0) { | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(obj); | ||
| #endif | ||
| return -1; | ||
| } | ||
| } | ||
| if (encoder_listencode_obj(s, writer, obj, indent_level, indent_cache)) { | ||
| _PyErr_FormatNote("when serializing %T item %zd", seq, i); | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(obj); | ||
| #endif | ||
| return -1; | ||
| } | ||
| #ifdef Py_GIL_DISABLED | ||
| Py_DECREF(obj); | ||
| #endif | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not be better to move the test to
test_speedups.py?