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
Changes from 3 commits
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,5 @@ | ||
from test.test_json import CTest | ||
import test.support | ||
|
||
|
||
class BadBool: | ||
|
@@ -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", | ||
None) | ||
|
||
@test.support.cpython_only | ||
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. I think this test should be passed on other implementations that have compatible 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. i thought about using maybe we can assume that every other C implementations implements 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. I think that if other C implementation doesn't implement |
||
def test_issue31505(self): | ||
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. I think this can be named |
||
# There shouldn't be an assertion failure in case c_make_encoder() | ||
# receives a bad encoder() argument. | ||
def _bad_encoder1(*args): | ||
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. No need to use starting underscore. This is a local name. |
||
return None | ||
enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder1, None, | ||
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. Use more meaningful arguments. Otherwise TypeError could be raised for different reason (for example if
|
||
'foo', 'bar', None, None, None) | ||
with self.assertRaises(TypeError): | ||
enc(obj='spam', _current_indent_level=4) | ||
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. Why keyword arguments are used? In the code an encoder is called with positional arguments. 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. no reason, my bad. |
||
with self.assertRaises(TypeError): | ||
enc(obj={'spam': 42}, _current_indent_level=4) | ||
|
||
def _bad_encoder2(*args): | ||
1/0 | ||
enc = self.json.encoder.c_make_encoder(None, None, _bad_encoder2, None, | ||
'foo', 'bar', None, None, None) | ||
with self.assertRaises(ZeroDivisionError): | ||
enc(obj='spam', _current_indent_level=4) | ||
|
||
def test_bad_bool_args(self): | ||
def test(name): | ||
self.json.encoder.JSONEncoder(**{name: BadBool()}).encode({'a': 1}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fix an assertion failure in `json`, in case `_json.make_encoder()` received | ||
a bad `encoder()` argument. Patch by Oren Milman. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1429,10 +1429,19 @@ static PyObject * | |
encoder_encode_string(PyEncoderObject *s, PyObject *obj) | ||
{ | ||
/* Return the JSON representation of a string */ | ||
PyObject *encoded; | ||
|
||
if (s->fast_encode) | ||
return s->fast_encode(NULL, obj); | ||
else | ||
return PyObject_CallFunctionObjArgs(s->encoder, obj, NULL); | ||
encoded = PyObject_CallFunctionObjArgs(s->encoder, obj, NULL); | ||
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. Add braces around |
||
if (encoded != NULL && !PyUnicode_Check(encoded)) { | ||
PyErr_Format(PyExc_TypeError, | ||
"encoder() must return a string, not %.80s", | ||
Py_TYPE(encoded)->tp_name); | ||
Py_DECREF(encoded); | ||
return NULL; | ||
} | ||
return encoded; | ||
} | ||
|
||
static int | ||
|
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 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.
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.
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 newPyEncoderObject
, 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..
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.
Seems your are right. Thank you for a reference.