gh-143544: Fix use-after-free in _json.raise_errmsg #143561

VanshAgarwal24036 · 2026-01-08T13:10:47Z

Summary

When JSONDecodeError is user-replaced and re-enters during JSON parsing,
_raise_errmsg could reuse a freed exception type, leading to a
use-after-free.

This change holds a strong reference across the call and validates the
exception type before setting it, falling back safely when needed.

Issue

Use-after-free in _json.raise_errmsg via re-entrant JSONDecodeError hook #143544

VanshAgarwal24036 · 2026-01-08T13:15:03Z

skip news

ZeroIntensity · 2026-01-08T13:21:12Z

No, this affects user-facing APIs; please add a news entry.

ZeroIntensity

Please avoid making unnecessary code changes. They make it significantly more difficult to review.
This is doing too much. The much simpler and more correct fix would be to simply move the Py_DECREF call on line 426 to after the if (exc) block.
Please add a test case and news entry.

Modules/_json.c

bedevere-app · 2026-01-08T13:30:49Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

VanshAgarwal24036 · 2026-01-08T14:25:50Z

Thanks for the review.

I’ve updated the patch to keep the fix minimal, added a regression test
that restores global state, and included a NEWS entry as requested.

Lib/test/test_json/test_fail.py

Misc/NEWS.d/next/Library/2026-01-08-19-38-02.gh-issue-143544.4jRCBJ.rst

ZeroIntensity · 2026-01-08T14:30:23Z

Modules/_json.c

+
    PyObject *JSONDecodeError =
-         PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError));
+        PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError));


Again please don't make unrelated formatting changes.

Please revert this change.

ZeroIntensity · 2026-01-08T14:30:38Z

Modules/_json.c

-    if (exc) {
+    PyObject *exc = PyObject_CallFunction(JSONDecodeError, "zOn", msg, s, end);
+    if (exc != NULL) {


Same here. The only change we need is the movement of the Py_DECREF call.

Please revert this change, only move Py_DECREF(JSONDecodeError);.

Lib/test/test_json/test_fail.py

VanshAgarwal24036 · 2026-01-08T15:41:10Z

Thanks for the clarification. I’ve updated the test to only assert that the
interpreter doesn’t crash and pushed the changes.

VanshAgarwal24036 · 2026-01-08T15:43:39Z

The NEWS entry was removed per discussion above. Could someone please add the “skip news” label?

VanshAgarwal24036 · 2026-01-09T11:40:58Z

I have made the requested changes; please review again.

bedevere-app · 2026-01-09T11:41:03Z

Thanks for making the requested changes!

@ZeroIntensity: please review the changes made to this pull request.

vstinner · 2026-01-09T11:45:20Z

Modules/_json.c

+
    PyObject *JSONDecodeError =
-         PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError));
+        PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError));


Please revert this change.

vstinner · 2026-01-09T11:45:44Z

Modules/_json.c

-    if (exc) {
+    PyObject *exc = PyObject_CallFunction(JSONDecodeError, "zOn", msg, s, end);
+    if (exc != NULL) {


Please revert this change, only move Py_DECREF(JSONDecodeError);.

vstinner · 2026-01-09T11:46:01Z

Lib/test/test_json/test_fail.py


+    def test_reentrant_jsondecodeerror_does_not_crash(self):
+        # gh-143544
+        import json


Please move this import at the module top level.

Moved import json to the module top level as requested. Thanks.

vstinner · 2026-01-09T12:12:33Z

Modules/_json.c

        Py_DECREF(exc);
    }
+
+    /* Move DECREF after PyErr_SetObject */


I don't think that this comment is useful.

Removed the comment as suggested.

…test

picnixz · 2026-01-09T15:45:04Z

Lib/test/test_json/test_fail.py

+        ):
+            # The exact exception type is not important here;
+            # this test only ensures we don't crash.
+            with self.assertRaises(Exception):


Check the exact exception.

Good point — agreed. I’ll update the test to assert the exact expected exception (JSONDecodeError) instead of a generic Exception, so the regression is checked more precisely.

Lib/test/test_json/test_fail.py

Co-authored-by: Bénédikt Tran <[email protected]>

vstinner · 2026-01-09T16:34:43Z

Lib/test/test_json/test_fail.py

+        hook = Trigger()
+        with (
+            support.swap_attr(json, "JSONDecodeError", hook),
+            support.swap_attr(json.decoder, "JSONDecodeError", hook)


Using swap_attr() context manager keeps hook alive which prevents triggering the crash. Please move back to regular assignment using try/finally.

Also, you need to clear hook (ex: del hook) after the assignment to trigger the crash.

Currently, the test doesn't crash if I revert the _json.c fix.

Using swap_attr() context manager keeps hook alive which prevents triggering the crash. Please move back to regular assignment using try/finally.

Oh! that's my bad.

However, a comment may be needed so that others like me don't forget.

Thanks for the clarification — that makes sense.
I’ll revert swap_attr() usage, restore try/finally, and explicitly del hook after assignment so the test actually reproduces the original crash when _json.c is reverted.
I’ll update the test accordingly.

However, a comment may be needed so that others like me don't forget.

Maybe the test should check the reference count: self.assertEqual(sys.getrefcount(hook), 3) before del hook.

Good point — I’ve added a comment explaining why del hook is required and an explicit sys.getrefcount() assertion before deletion to document the reference state.
This should make the intent of the test clearer for future readers.

serhiy-storchaka · 2026-01-09T18:40:48Z

Lib/test/test_json/test_fail.py

@@ -1,4 +1,5 @@
 from test.test_json import PyTest, CTest
+import json


I am not sure this is the right place for such test. This file tests how different decoding errors are handled (and some encoding errors, but I think they are misplaced). But we need to test the bug in the code that raises JSONDecodeError. Maybe test_speedups.py is better place for this.

serhiy-storchaka · 2026-01-09T18:45:09Z

Lib/test/test_json/test_fail.py

+                # Remove JSONDecodeError during construction to trigger re-entrancy
+                del json.JSONDecodeError
+                del json.decoder.JSONDecodeError
+                return ValueError("boom")


Just for the case, call test.support.gc_collect() .

serhiy-storchaka · 2026-01-09T18:59:29Z

Lib/test/test_json/test_fail.py

+                del json.decoder.JSONDecodeError
+                return ValueError("boom")
+
+        hook = Trigger()


hook must be a subclass of BaseException, otherwise PyErr_SetObject() will raise a SystemError.

class hook(Exception): def __new__(...): # ...

And now calling gc_collect() is a necessity, because class creates a reference loop.

VanshAgarwal24036 · 2026-01-10T09:16:25Z

@serhiy-storchaka Thanks for the feedback.

I placed the test in test_fail.py because the regression manifests while raising JSONDecodeError during decoding, and this file already exercises failure paths around decoding errors.

That said, I’m happy to move the test to test_speedups.py or another more appropriate location if you think that’s a better fit — please let me know your preference.

… test

VanshAgarwal24036 · 2026-01-10T09:31:41Z

I have made the requested changes; please review again.

bedevere-app · 2026-01-10T09:31:47Z

Thanks for making the requested changes!

@picnixz, @ZeroIntensity: please review the changes made to this pull request.

serhiy-storchaka

There is no good place for such test, but I think test_speedups is less wrong than test_fail.

But I suspect that it may be impossible to write a test without raising SystemError for the fixed code, which is only slightly better than a crash. If this is so, we will give up and accept a fix without unit test.

serhiy-storchaka · 2026-01-10T09:57:20Z

Lib/test/test_json/test_fail.py

+                raise self

-        hook = Trigger()
+        hook = Trigger("boom")


No, hook should be not an instance of exception, but an exception class itself.

So, you need to define __new__() to trigger execution when it is called, not __call__().

serhiy-storchaka · 2026-01-10T10:06:45Z

Lib/test/test_json/test_fail.py

+            self.assertEqual(sys.getrefcount(hook), 3)
            del hook

+            support.gc_collect()


It does not have effect here, because there are references like json.decoder.JSONDecodeError.

But calling it after del json.decoder.JSONDecodeError in __new__() will also not help, because we still have a reference as a method parameter. So, it is useless.

Maybe we could not create a good test for this issue.

VanshAgarwal24036 · 2026-01-10T11:42:43Z

Thanks for clarifying. I agree that reliably testing this without triggering a SystemError may not be feasible.
If you’re OK with proceeding without a unit test, I can drop the test changes and keep the fix only. Please let me know how you’d prefer to move forward.

serhiy-storchaka · 2026-01-10T13:25:30Z

If this is not feasible, drop the tests.

Thank you for your PR.

VanshAgarwal24036 · 2026-01-10T16:59:08Z

I’ve dropped the test as suggested since it doesn’t seem feasible to reliably reproduce this case without introducing a SystemError.
All CI checks are now passing.

Could you please re-review and let me know if anything else is needed?

pythongh-143544: Fix use-after-free in _json.raise_errmsg

9477824

bedevere-app bot mentioned this pull request Jan 8, 2026

Use-after-free in _json.raise_errmsg via re-entrant JSONDecodeError hook #143544

Open

bedevere-app bot added the awaiting review label Jan 8, 2026

ZeroIntensity requested changes Jan 8, 2026

View reviewed changes

Modules/_json.c Show resolved Hide resolved

Modules/_json.c Outdated Show resolved Hide resolved

Modules/_json.c Outdated Show resolved Hide resolved

Modules/_json.c Outdated Show resolved Hide resolved

Modules/_json.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting changes and removed awaiting review labels Jan 8, 2026

VanshAgarwal24036 added 4 commits January 8, 2026 19:31

pythongh-143544: Fix use-after-free in _json.raise_errmsg

946aee3

pythongh-143544: Add regression test for re-entrant JSONDecodeError

55b4850

pythongh-143544: Add NEWS entry

1940b1e

pythongh-143544: Fix trailing whitespace in test

ed2c55c

vstinner reviewed Jan 8, 2026

View reviewed changes

Lib/test/test_json/test_fail.py Outdated Show resolved Hide resolved

Misc/NEWS.d/next/Library/2026-01-08-19-38-02.gh-issue-143544.4jRCBJ.rst Outdated Show resolved Hide resolved

ZeroIntensity reviewed Jan 8, 2026

View reviewed changes

pythongh-143544: Adjust regression test to allow SystemError

099fc4f

vstinner added the skip news label Jan 8, 2026

VanshAgarwal24036 requested a review from ZeroIntensity January 9, 2026 05:16

bedevere-app bot added awaiting change review and removed awaiting changes labels Jan 9, 2026

vstinner reviewed Jan 9, 2026

View reviewed changes

VanshAgarwal24036 added 2 commits January 9, 2026 17:30

pythongh-143544: Fix use-after-free in _json.raise_errmsg

b238345

pythongh-143544: Move json import to module level in regression test

fa69d1e

vstinner reviewed Jan 9, 2026

View reviewed changes

pythongh-143544: Remove unnecessary comment

39767c6

VanshAgarwal24036 added 2 commits January 9, 2026 20:34

pythongh-143544: Use support.swap_attr in re-entrant JSONDecodeError …

10c61c4

…test

pythongh-143544: Fix use-after-free in _json.raise_errmsg

4a7f323

picnixz reviewed Jan 9, 2026

View reviewed changes

Lib/test/test_json/test_fail.py Outdated Show resolved Hide resolved

picnixz reviewed Jan 9, 2026

View reviewed changes

Lib/test/test_json/test_fail.py Outdated Show resolved Hide resolved

picnixz reviewed Jan 9, 2026

View reviewed changes

Lib/test/test_json/test_fail.py Outdated Show resolved Hide resolved

VanshAgarwal24036 and others added 4 commits January 9, 2026 21:22

Update Lib/test/test_json/test_fail.py

0b38b8e

Co-authored-by: Bénédikt Tran <[email protected]>

Update Lib/test/test_json/test_fail.py

0593e2f

Co-authored-by: Bénédikt Tran <[email protected]>

pythongh-143544: Tighten regression test exception assertion

b485677

pythongh-143544: Fix re-entrant JSONDecodeError regression test

bbc357d

vstinner reviewed Jan 9, 2026

View reviewed changes

VanshAgarwal24036 added 2 commits January 9, 2026 22:16

test_json: add regression test for re-entrant JSONDecodeError crash

765ffb2

test_json: added Comment

b745fed

serhiy-storchaka reviewed Jan 9, 2026

View reviewed changes

test_json: assert TypeError for invalid JSONDecodeError replacement

0a03442

VanshAgarwal24036 added 3 commits January 10, 2026 14:48

test_json: document and assert refcount for reentrant JSONDecodeError…

4257cdc

… test

test_json: fix reentrant JSONDecodeError test

74cfaed

test_json: fix re-entrant JSONDecodeError test

211cb30

bedevere-app bot added awaiting change review and removed awaiting changes labels Jan 10, 2026

bedevere-app bot requested review from ZeroIntensity and picnixz January 10, 2026 09:31

serhiy-storchaka reviewed Jan 10, 2026

View reviewed changes

pythongh-143544: Revert test_json regression test changes

a2e23bc

		@@ -1,4 +1,5 @@
		from test.test_json import PyTest, CTest
		import json

Uh oh!

gh-143544: Fix use-after-free in _json.raise_errmsg #143561

Are you sure you want to change the base?

gh-143544: Fix use-after-free in _json.raise_errmsg #143561

Conversation

VanshAgarwal24036 commented Jan 8, 2026

Summary

Issue

Uh oh!

VanshAgarwal24036 commented Jan 8, 2026

Uh oh!

ZeroIntensity commented Jan 8, 2026

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app bot commented Jan 8, 2026

Uh oh!

VanshAgarwal24036 commented Jan 8, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

VanshAgarwal24036 commented Jan 8, 2026

Uh oh!

VanshAgarwal24036 commented Jan 8, 2026

Uh oh!

VanshAgarwal24036 commented Jan 9, 2026

Uh oh!

bedevere-app bot commented Jan 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

VanshAgarwal24036 commented Jan 10, 2026 •

edited

Loading