-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-143544: Fix possible use-after-free in the JSON decoder when JSONDecodeError disappears during raising it #143561
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 5 commits
9477824
946aee3
55b4850
1940b1e
ed2c55c
099fc4f
b238345
fa69d1e
39767c6
b500563
5841c2c
c397e8e
10c61c4
4a7f323
0b38b8e
0593e2f
b485677
bbc357d
765ffb2
b745fed
0a03442
4257cdc
74cfaed
211cb30
a2e23bc
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 |
|---|---|---|
|
|
@@ -236,5 +236,35 @@ def test_linecol(self): | |
| 'Expecting value: line %s column %d (char %d)' % | ||
| (line, col, idx)) | ||
|
|
||
| def test_reentrant_jsondecodeerror_does_not_crash(self): | ||
| import json | ||
|
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 move this import at the module top level.
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. Moved import json to the module top level as requested. Thanks. |
||
|
|
||
| orig_json_error = json.JSONDecodeError | ||
| orig_decoder_error = json.decoder.JSONDecodeError | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
|
|
||
| class Trigger: | ||
| def __call__(self, *args): | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
| import json as mod | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
| # Remove JSONDecodeError during construction to trigger re-entrancy | ||
| if hasattr(mod, "JSONDecodeError"): | ||
| del mod.JSONDecodeError | ||
| if hasattr(mod.decoder, "JSONDecodeError"): | ||
| del mod.decoder.JSONDecodeError | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
| return ValueError("boom") | ||
|
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. Just for the case, call |
||
|
|
||
| hook = Trigger() | ||
|
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.
class hook(Exception):
def __new__(...):
# ...And now calling |
||
| try: | ||
| json.JSONDecodeError = hook | ||
| json.decoder.JSONDecodeError = hook | ||
|
|
||
| # The exact exception type is not important here; the test | ||
| # only verifies that we do not crash or trigger a SystemError. | ||
| with self.assertRaises(Exception): | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
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. Check the exact exception.
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. 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. |
||
| self.loads('"\\uZZZZ"') | ||
|
|
||
| finally: | ||
| json.JSONDecodeError = orig_json_error | ||
| json.decoder.JSONDecodeError = orig_decoder_error | ||
|
|
||
| class TestPyFail(TestFail, PyTest): pass | ||
| class TestCFail(TestFail, CTest): pass | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fixed a crash in json decoding when JSONDecodeError is replaced with a | ||
| custom callable. | ||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -415,19 +415,20 @@ raise_errmsg(const char *msg, PyObject *s, Py_ssize_t end) | |
| { | ||
| /* Use JSONDecodeError exception to raise a nice looking ValueError subclass */ | ||
|
ZeroIntensity marked this conversation as resolved.
|
||
| _Py_DECLARE_STR(json_decoder, "json.decoder"); | ||
|
|
||
|
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 is an unrelated change. |
||
| PyObject *JSONDecodeError = | ||
| PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError)); | ||
| PyImport_ImportModuleAttr(&_Py_STR(json_decoder), &_Py_ID(JSONDecodeError)); | ||
|
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. Again please don't make unrelated formatting changes.
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 change. |
||
| if (JSONDecodeError == NULL) { | ||
| return; | ||
| } | ||
|
|
||
| PyObject *exc; | ||
| exc = PyObject_CallFunction(JSONDecodeError, "zOn", msg, s, end); | ||
| Py_DECREF(JSONDecodeError); | ||
| if (exc) { | ||
| PyObject *exc = PyObject_CallFunction(JSONDecodeError, "zOn", msg, s, end); | ||
|
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 is also an unrelated change. |
||
| if (exc != NULL) { | ||
|
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. Same here. The only change we need is the movement of the
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 change, only move |
||
| PyErr_SetObject(JSONDecodeError, exc); | ||
| Py_DECREF(exc); | ||
| } | ||
|
|
||
| Py_DECREF(JSONDecodeError); | ||
| } | ||
|
|
||
| static void | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.