-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-146152: Fix memory leak in _json encoder error paths #146164
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
Changes from 11 commits
0597100
9ec9348
cd9cc6f
5428b5e
b68b52b
cafe9ba
a4be61f
f7bdb6f
f09ce9a
5359ecd
753a88e
6498f26
6b69fea
81332c7
a9c895e
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 |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import weakref | ||
| import sys | ||
| from test import support | ||
| from test.test_json import PyTest, CTest | ||
|
|
||
|
|
||
|
picnixz marked this conversation as resolved.
|
||
| class JSONTestObject: | ||
| pass | ||
|
|
||
|
okiemute04 marked this conversation as resolved.
|
||
|
|
@@ -115,6 +116,36 @@ def default(self, o): | |
| with support.infinite_recursion(1000): | ||
| EndlessJSONEncoder(check_circular=False).encode(5j) | ||
|
|
||
| @support.skip_if_unlimited_stack_size | ||
| @support.skip_emscripten_stack_overflow() | ||
| @support.skip_wasi_stack_overflow() | ||
| def test_memory_leak_on_recursion_error(self): | ||
| # Test that no memory leak occurs when a RecursionError is raised. | ||
| class LeakTestObj: | ||
| pass | ||
|
|
||
| weak_refs = [] | ||
| def default(obj): | ||
| if isinstance(obj, LeakTestObj): | ||
| new_obj = LeakTestObj() | ||
| weak_refs.append(weakref.ref(new_obj)) | ||
| return [new_obj] | ||
| raise TypeError | ||
|
|
||
| depth = min(500, sys.getrecursionlimit() - 10) | ||
|
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. You should move this code inside infinite_recursion() context, since infinite_recursion() changes the recursion limit. |
||
| obj = LeakTestObj() | ||
| for _ in range(depth): | ||
| obj = [obj] | ||
|
|
||
| try: | ||
| self.dumps(obj, default=default) | ||
| except Exception: | ||
| pass | ||
|
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. Why not use the infinite recursion pattern + catching the RecursionError explicitly?
Contributor
Author
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. okay, that's a good suggestion, it makes more sense, I will do that |
||
|
|
||
| support.gc_collect() | ||
| self.assertTrue(weak_refs, "No objects were created to track") | ||
| for i, ref in enumerate(weak_refs): | ||
| self.assertIsNone(ref(), f"object {i} still alive") | ||
|
picnixz marked this conversation as resolved.
okiemute04 marked this conversation as resolved.
|
||
|
|
||
| class TestPyRecursion(TestRecursion, PyTest): pass | ||
| class TestCRecursion(TestRecursion, CTest): pass | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||||
| Fix a memory leak in the :mod:`json` module when a RecursionError occurs | ||||||||||||||
| during encoding. Previously, objects created in the ``default()`` function | ||||||||||||||
| while recursively encoding JSON could remain alive due to being tracked | ||||||||||||||
| indefinitely in the encoder's internal circular-reference dictionary. This | ||||||||||||||
| change ensures such objects are properly freed after the exception. | ||||||||||||||
|
Comment on lines
+1
to
+5
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.
Suggested change
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1632,25 +1632,38 @@ encoder_listencode_obj(PyEncoderObject *s, PyUnicodeWriter *writer, | |||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if (_Py_EnterRecursiveCall(" while encoding a JSON object")) { | ||||||||||||||||||||||||||
| if (ident != NULL) { | ||||||||||||||||||||||||||
| int del_rv = PyDict_DelItem(s->markers, ident); | ||||||||||||||||||||||||||
| Py_DECREF(ident); | ||||||||||||||||||||||||||
| if (del_rv < 0) { | ||||||||||||||||||||||||||
| Py_DECREF(newobj); | ||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+1635
to
+1637
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.
Suggested change
Actually since we are anyway falling through the return -1 case we can simplify this as follows. Then keep the XDECREF for indent. However explicitly suppress the error code of DelItem. |
||||||||||||||||||||||||||
| Py_DECREF(newobj); | ||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| rv = encoder_listencode_obj(s, writer, newobj, indent_level, indent_cache); | ||||||||||||||||||||||||||
| _Py_LeaveRecursiveCall(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
okiemute04 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| Py_DECREF(newobj); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
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. Please revert this
Contributor
Author
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. okay
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. Revert cosmetic changes. Nothing was properly reverted. |
||||||||||||||||||||||||||
| if (rv) { | ||||||||||||||||||||||||||
| _PyErr_FormatNote("when serializing %T object", obj); | ||||||||||||||||||||||||||
|
okiemute04 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| if (ident != NULL) { | ||||||||||||||||||||||||||
| int del_rv = PyDict_DelItem(s->markers, ident); | ||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| if (del_rv < 0) { | ||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+1648
to
+1650
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. This one can benefit from the same optimization as my other comment as we anyway return -1. |
||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
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. Revert this blank line. |
||||||||||||||||||||||||||
| if (ident != NULL) { | ||||||||||||||||||||||||||
| if (PyDict_DelItem(s->markers, ident)) { | ||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| int del_rv = PyDict_DelItem(s->markers, ident); | ||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| if (del_rv < 0) { | ||||||||||||||||||||||||||
| return -1; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Py_XDECREF(ident); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return rv; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.