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

bpo-31505: Fix an assertion failure in json, in case _json.make_encoder() received a bad encoder() argument #3643

Merged
merged 5 commits into from Sep 24, 2017

Conversation

Projects
None yet
6 participants
@orenmn
Contributor

orenmn commented Sep 18, 2017

  • in _json.c: add a check whether encoder() hasn't returned a string.
  • in test_json/test_speedups.py: add a test to verify that the assertion failure is no more, and that the patch doesn't break anything when encoder() fails.

https://bugs.python.org/issue31505

Show outdated Hide outdated Lib/test/test_json/test_speedups.py Outdated
Show outdated Hide outdated Lib/test/test_json/test_speedups.py Outdated
@@ -36,6 +37,26 @@ def test_make_encoder(self):
b"\xCD\x7D\x3D\x4E\x12\x4C\xF9\x79\xD7\x52\xBA\x82\xF2\x27\x4A\x7D\xA0\xCA\x75",

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 18, 2017

Member

This isn't related to this issue, but it seems to me that TypeError is raised for different cause than was originally in this test. This test doesn't work as intended.

@serhiy-storchaka

serhiy-storchaka Sep 18, 2017

Member

This isn't related to this issue, but it seems to me that TypeError is raised for different cause than was originally in this test. This test doesn't work as intended.

This comment has been minimized.

@orenmn

orenmn Sep 18, 2017

Contributor

IIUC, Victor wrote this test to verify that the interpreter doesn't crash (as part of https://bugs.python.org/issue6986, as originally PyArg_ParseTupleAndKeywords() was given actual fields of the new PyEncoderObject, instead of temp vars), so raising a TypeError is the wanted outcome here.
maybe we can just change the name of the test to 'test_issue6986' (as it tests only this specific issue)?
and maybe do the same for test_make_scanner()?
anyway, I guess any such change should be in a PR referring to bpo-6986..

@orenmn

orenmn Sep 18, 2017

Contributor

IIUC, Victor wrote this test to verify that the interpreter doesn't crash (as part of https://bugs.python.org/issue6986, as originally PyArg_ParseTupleAndKeywords() was given actual fields of the new PyEncoderObject, instead of temp vars), so raising a TypeError is the wanted outcome here.
maybe we can just change the name of the test to 'test_issue6986' (as it tests only this specific issue)?
and maybe do the same for test_make_scanner()?
anyway, I guess any such change should be in a PR referring to bpo-6986..

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 19, 2017

Member

Seems your are right. Thank you for a reference.

@serhiy-storchaka

serhiy-storchaka Sep 19, 2017

Member

Seems your are right. Thank you for a reference.

Show outdated Hide outdated Lib/test/test_json/test_speedups.py Outdated
Show outdated Hide outdated Lib/test/test_json/test_speedups.py Outdated
Show outdated Hide outdated Lib/test/test_json/test_speedups.py Outdated
Show outdated Hide outdated Modules/_json.c Outdated
@serhiy-storchaka

LGTM.

I prefer different form for assertions, but will merge the PR in any case.

bad_encoder1, None, ': ', ', ',
False, False, False)
with self.assertRaises(TypeError):
enc('spam', 4)

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Sep 19, 2017

Member

This can be written without context manager:

self.assertRaises(TypeError, enc, 'spam', 4)

I prefer this form for simple function calls.

@serhiy-storchaka

serhiy-storchaka Sep 19, 2017

Member

This can be written without context manager:

self.assertRaises(TypeError, enc, 'spam', 4)

I prefer this form for simple function calls.

@serhiy-storchaka serhiy-storchaka merged commit 2b382dd into python:master Sep 24, 2017

4 checks passed

bedevere/issue-number Issue number 31505 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington Sep 24, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

miss-islington commented Sep 24, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington Sep 26, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖 I'm not a witch! I'm not a witch!

miss-islington commented Sep 26, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖 I'm not a witch! I'm not a witch!

@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington Sep 26, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

miss-islington commented Sep 26, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington Sep 27, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

miss-islington commented Sep 27, 2017

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒🤖

@miss-islington

This comment has been minimized.

Show comment
Hide comment
@miss-islington

miss-islington Sep 27, 2017

Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the 2.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 2b382dd6121bb1e4b75470fb3ef8555665df3eb6 2.7

miss-islington commented Sep 27, 2017

Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the 2.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 2b382dd6121bb1e4b75470fb3ef8555665df3eb6 2.7

@bedevere-bot

This comment has been minimized.

Show comment
Hide comment
@bedevere-bot

bedevere-bot Sep 27, 2017

GH-3777 is a backport of this pull request to the 3.6 branch.

bedevere-bot commented Sep 27, 2017

GH-3777 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Sep 27, 2017

[3.6] bpo-31505: Fix an assertion failure in json, in case _json.make…
…_encoder() received a bad encoder() argument. (pythonGH-3643)

(cherry picked from commit 2b382dd)

serhiy-storchaka added a commit that referenced this pull request Sep 27, 2017

[3.6] bpo-31505: Fix an assertion failure in json, in case _json.make…
…_encoder() received a bad encoder() argument. (GH-3643) (#3777)

(cherry picked from commit 2b382dd)

daxlab added a commit to daxlab/cpython that referenced this pull request Oct 1, 2017

@Mariatta Mariatta removed the awaiting merge label Oct 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment