-
-
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 18 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 |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| from test.test_json import PyTest, CTest | ||
| from test import support | ||
| import json | ||
|
|
||
| # 2007-10-05 | ||
| JSONDOCS = [ | ||
|
|
@@ -236,5 +238,27 @@ def test_linecol(self): | |
| 'Expecting value: line %s column %d (char %d)' % | ||
| (line, col, idx)) | ||
|
|
||
|
|
||
| def test_reentrant_jsondecodeerror_does_not_crash(self): | ||
| # gh-143544 | ||
|
|
||
| class Trigger: | ||
| def __call__(self, *args, **kwargs): | ||
| # Remove JSONDecodeError during construction to trigger re-entrancy | ||
| del json.JSONDecodeError | ||
| del json.decoder.JSONDecodeError | ||
| 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 |
||
| with ( | ||
| support.swap_attr(json, "JSONDecodeError", hook), | ||
| support.swap_attr(json.decoder, "JSONDecodeError", hook) | ||
|
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. 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: Currently, the test doesn't crash if I revert 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.
Oh! that's my bad.
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. However, a comment may be needed so that others like me don't forget.
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. Thanks for the clarification — that makes sense.
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.
Maybe the test should check the reference count:
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 — I’ve added a comment explaining why del hook is required and an explicit sys.getrefcount() assertion before deletion to document the reference state. |
||
| ): | ||
| # The exact exception type is not important here; | ||
| # this test only ensures we don't crash. | ||
| with self.assertRaises(json.JSONDecodeError): | ||
| json.loads('"\\uZZZZ"') | ||
|
|
||
|
VanshAgarwal24036 marked this conversation as resolved.
Outdated
|
||
|
|
||
| class TestPyFail(TestFail, PyTest): pass | ||
| class TestCFail(TestFail, CTest): pass | ||
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.
Do we really need this? I'm asking because
TestFailis a mixin class with aselfattribute forJSONDecoderError. So should we patch this module directly or not?Uh oh!
There was an error while loading. Please reload this page.
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.
This test mutates json.JSONDecodeError and json.decoder.JSONDecodeError directly to trigger the re-entrancy case, so it needs access to the json module object. Using self.JSONDecodeError wouldn’t be sufficient here.
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.
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.pyis better place for this.