bpo-31505: Fix an assertion failure in json, in case _json.make_encoder() received a bad encoder() argument by orenmn · Pull Request #3643 · python/cpython

orenmn · 2017-09-18T09:53:21Z

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

serhiy-storchaka · 2017-09-18T10:30:36Z

            None)

+    @test.support.cpython_only
+    def test_issue31505(self):


I think this can be named test_bad_str_encoder.

serhiy-storchaka · 2017-09-18T10:31:30Z

            b"\xCD\x7D\x3D\x4E\x12\x4C\xF9\x79\xD7\x52\xBA\x82\xF2\x27\x4A\x7D\xA0\xCA\x75",
            None)

+    @test.support.cpython_only


I think this test should be passed on other implementations that have compatible c_make_encoder.

i thought about using test.support.get_attribute() to check whether c_make_encoder() is available, but test_make_encoder() just uses it, and would have failed if it hadn't existed.

maybe we can assume that every other C implementations implements test_make_encoder()?

I think that if other C implementation doesn't implement test_make_encoder() we should get a request for making this test optional.

serhiy-storchaka · 2017-09-18T10:33:16Z

@@ -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 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.

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..

Seems your are right. Thank you for a reference.

serhiy-storchaka · 2017-09-18T10:36:11Z

+        # receives a bad encoder() argument.
+        def _bad_encoder1(*args):
+            return None
+        enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder1, None,


Use more meaningful arguments. Otherwise TypeError could be raised for different reason (for example if default is not a callable).

c_make_encoder(None, default, _bad_encoder1, None, ': ', ', ', False, False, False)

serhiy-storchaka · 2017-09-18T10:44:33Z

+    def test_issue31505(self):
+        # There shouldn't be an assertion failure in case c_make_encoder()
+        # receives a bad encoder() argument.
+        def _bad_encoder1(*args):


No need to use starting underscore. This is a local name.

serhiy-storchaka · 2017-09-18T10:45:18Z

+        enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder1, None,
+                                               'foo', 'bar', None, None, None)
+        with self.assertRaises(TypeError):
+            enc(obj='spam', _current_indent_level=4)


Why keyword arguments are used? In the code an encoder is called with positional arguments.

no reason, my bad.

serhiy-storchaka · 2017-09-18T10:53:46Z

        return s->fast_encode(NULL, obj);
-    else
-        return PyObject_CallFunctionObjArgs(s->encoder, obj, NULL);
+    encoded = PyObject_CallFunctionObjArgs(s->encoder, obj, NULL);


Add braces around return above.

serhiy-storchaka

LGTM.

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

serhiy-storchaka · 2017-09-19T16:27:06Z

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


This can be written without context manager:

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

I prefer this form for simple function calls.

miss-islington · 2017-09-24T09:07:14Z

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 · 2017-09-26T23:15:43Z

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 · 2017-09-26T23:15:44Z

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 · 2017-09-27T04:47:39Z

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 · 2017-09-27T04:47:40Z

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 · 2017-09-27T04:47:46Z

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

…_encoder() received a bad encoder() argument. (pythonGH-3643) (cherry picked from commit 2b382dd)

…_encoder() received a bad encoder() argument. (GH-3643) (#3777) (cherry picked from commit 2b382dd)

orenmn added 3 commits September 18, 2017 12:08

init commit

a043ba6

fix test name

fb91a87

improve patch C code

99244b7

the-knights-who-say-ni added the CLA signed label Sep 18, 2017

bedevere-bot added the awaiting review label Sep 18, 2017

serhiy-storchaka reviewed Sep 18, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 18, 2017

orenmn added 2 commits September 18, 2017 19:53

various fixes according to Serhiy's advice

da90d1e

remove the redundant import

014db74

serhiy-storchaka approved these changes Sep 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 19, 2017

serhiy-storchaka added needs backport to 2.7 type-bug An unexpected behavior, bug, or error labels Sep 24, 2017

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

serhiy-storchaka added needs backport to 2.7 and removed needs backport to 2.7 labels Sep 26, 2017

serhiy-storchaka added needs backport to 3.6 and removed needs backport to 3.6 labels Sep 27, 2017

bedevere-bot removed the needs backport to 3.6 label Sep 27, 2017

miss-islington pushed 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…

4ff71e6

…_encoder() received a bad encoder() argument. (pythonGH-3643) (cherry picked from commit 2b382dd)

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

[3.6] bpo-31505: Fix an assertion failure in json, in case _json.make…

7c24e99

…_encoder() received a bad encoder() argument. (GH-3643) (#3777) (cherry picked from commit 2b382dd)

serhiy-storchaka removed the needs backport to 2.7 label Sep 27, 2017

Mariatta removed the awaiting merge label Oct 8, 2017

		@@ -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",

Uh oh!

Conversation

orenmn commented Sep 18, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

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

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!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 24, 2017

Uh oh!

miss-islington commented Sep 26, 2017

Uh oh!

miss-islington commented Sep 26, 2017

Uh oh!

miss-islington commented Sep 27, 2017

Uh oh!

miss-islington commented Sep 27, 2017

Uh oh!

bedevere-bot commented Sep 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Sep 18, 2017 •

edited by bedevere-bot

Loading