Skip to content

bpo-31243: fix PyArg_ParseTuple failure checks #3171

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Aug 29, 2017
Merged

bpo-31243: fix PyArg_ParseTuple failure checks #3171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
14add40
init commit
orenmn Aug 21, 2017
d5575f6
improve a comment, and remove parenthese from class definition
orenmn Aug 21, 2017
99e8405
shorten the error messages and improve the test code.
orenmn Aug 24, 2017
c8a21ba
added NEWS.d
orenmn Aug 24, 2017
a3a1212
fix NEWS.d item
orenmn Aug 24, 2017
f7f38c7
No need to mention my name specially.
serhiy-storchaka Aug 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3245,6 +3245,26 @@ def _make_illegal_wrapper():
t = _make_illegal_wrapper()
self.assertRaises(TypeError, t.read)

# Issue 31243: calling read() while the return value of decoder's
# getstate() is invalid should neither crash the interpreter nor
# raise a SystemError.
def _make_very_illegal_wrapper(getstate_ret_val):
class BadDecoder:
def getstate(self):
return getstate_ret_val
def _get_bad_decoder(dummy):
return BadDecoder()
quopri = codecs.lookup("quopri")
with support.swap_attr(quopri, 'incrementaldecoder',
_get_bad_decoder):
return _make_illegal_wrapper()
t = _make_very_illegal_wrapper(42)
self.assertRaises(TypeError, t.read, 42)
t = _make_very_illegal_wrapper(())
self.assertRaises(TypeError, t.read, 42)
t = _make_very_illegal_wrapper((1, 2))
self.assertRaises(TypeError, t.read, 42)

def _check_create_at_shutdown(self, **kwargs):
# Issue #20037: creating a TextIOWrapper at shutdown
# shouldn't crash the interpreter.
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a crash in some methods of `io.TextIOWrapper`, when the decoder's state
is invalid. Patch by Oren Milman.
18 changes: 13 additions & 5 deletions Modules/_io/textio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,15 +1498,23 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint)
/* Given this, we know there was a valid snapshot point
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTM that this comment is wrong, but I didn't manage to find its origin (benjaminp created _textio.c with that comment in it (https://github.com/python/cpython/tree/4fa88fa0ba35e25ad9be66ebbdaba9aca553dc8b), but I don't know where it came from. BTW, the same comment can also be found in Lib/_pyio.py).
In addition, I am not familiar with the io module, so I am reluctant to remove this comment from here.

what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think @benjaminp?

* len(dec_buffer) bytes ago with decoder state (b'', dec_flags).
*/
if (PyArg_ParseTuple(state, "OO", &dec_buffer, &dec_flags) < 0) {
if (!PyTuple_Check(state)) {
PyErr_SetString(PyExc_TypeError,
"illegal decoder state");
Py_DECREF(state);
return -1;
}
if (!PyArg_ParseTuple(state,
"OO;illegal decoder state", &dec_buffer, &dec_flags))
{
Py_DECREF(state);
return -1;
}

if (!PyBytes_Check(dec_buffer)) {
PyErr_Format(PyExc_TypeError,
"decoder getstate() should have returned a bytes "
"object, not '%.200s'",
"illegal decoder state: the first item should be a "
"bytes object, not '%.200s'",
Py_TYPE(dec_buffer)->tp_name);
Py_DECREF(state);
return -1;
Expand Down Expand Up @@ -2384,8 +2392,8 @@ _io_TextIOWrapper_tell_impl(textio *self)
} \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add also an explicit check for a tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this should be fixed as part of bpo-28261, as bpo31243 is about PyArg_ParseTuple returning 0 or 1.
ISTM that if we add that check for a tuple, we might as well add ";illegal decoder state" to the argument of PyArg_ParseTuple.
Don't you think that leaving it to bpo-28261 is preferable? (I would open a PR for leftovers of bpo-28261 anyway today.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this can be done in a separate PR. But in any case I think it is worth to add a NEWS.d entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, I would add it soon

if (!PyBytes_Check(dec_buffer)) { \
PyErr_Format(PyExc_TypeError, \
"decoder getstate() should have returned a bytes " \
"object, not '%.200s'", \
"illegal decoder state: the first item should be a " \
"bytes object, not '%.200s'", \
Py_TYPE(dec_buffer)->tp_name); \
Py_DECREF(_state); \
goto fail; \
Expand Down
46 changes: 30 additions & 16 deletions Modules/_testcapimodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,8 +864,9 @@ test_L_code(PyObject *self)
PyTuple_SET_ITEM(tuple, 0, num);

value = -1;
if (PyArg_ParseTuple(tuple, "L:test_L_code", &value) < 0)
if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) {
return NULL;
}
if (value != 42)
return raiseTestError("test_L_code",
"L code returned wrong value for long 42");
Expand All @@ -878,8 +879,9 @@ test_L_code(PyObject *self)
PyTuple_SET_ITEM(tuple, 0, num);

value = -1;
if (PyArg_ParseTuple(tuple, "L:test_L_code", &value) < 0)
if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) {
return NULL;
}
if (value != 42)
return raiseTestError("test_L_code",
"L code returned wrong value for int 42");
Expand Down Expand Up @@ -1195,8 +1197,9 @@ test_k_code(PyObject *self)
PyTuple_SET_ITEM(tuple, 0, num);

value = 0;
if (PyArg_ParseTuple(tuple, "k:test_k_code", &value) < 0)
if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) {
return NULL;
}
if (value != ULONG_MAX)
return raiseTestError("test_k_code",
"k code returned wrong value for long 0xFFF...FFF");
Expand All @@ -1215,8 +1218,9 @@ test_k_code(PyObject *self)
PyTuple_SET_ITEM(tuple, 0, num);

value = 0;
if (PyArg_ParseTuple(tuple, "k:test_k_code", &value) < 0)
if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) {
return NULL;
}
if (value != (unsigned long)-0x42)
return raiseTestError("test_k_code",
"k code returned wrong value for long -0xFFF..000042");
Expand Down Expand Up @@ -1549,11 +1553,13 @@ test_s_code(PyObject *self)
/* These two blocks used to raise a TypeError:
* "argument must be string without null bytes, not str"
*/
if (PyArg_ParseTuple(tuple, "s:test_s_code1", &value) < 0)
return NULL;
if (!PyArg_ParseTuple(tuple, "s:test_s_code1", &value)) {
return NULL;
}

if (PyArg_ParseTuple(tuple, "z:test_s_code2", &value) < 0)
return NULL;
if (!PyArg_ParseTuple(tuple, "z:test_s_code2", &value)) {
return NULL;
}

Py_DECREF(tuple);
Py_RETURN_NONE;
Expand Down Expand Up @@ -1655,14 +1661,16 @@ test_u_code(PyObject *self)
PyTuple_SET_ITEM(tuple, 0, obj);

value = 0;
if (PyArg_ParseTuple(tuple, "u:test_u_code", &value) < 0)
if (!PyArg_ParseTuple(tuple, "u:test_u_code", &value)) {
return NULL;
}
if (value != PyUnicode_AS_UNICODE(obj))
return raiseTestError("test_u_code",
"u code returned wrong value for u'test'");
value = 0;
if (PyArg_ParseTuple(tuple, "u#:test_u_code", &value, &len) < 0)
if (!PyArg_ParseTuple(tuple, "u#:test_u_code", &value, &len)) {
return NULL;
}
if (value != PyUnicode_AS_UNICODE(obj) ||
len != PyUnicode_GET_SIZE(obj))
return raiseTestError("test_u_code",
Expand Down Expand Up @@ -1694,8 +1702,9 @@ test_Z_code(PyObject *self)
value2 = PyUnicode_AS_UNICODE(obj);

/* Test Z for both values */
if (PyArg_ParseTuple(tuple, "ZZ:test_Z_code", &value1, &value2) < 0)
if (!PyArg_ParseTuple(tuple, "ZZ:test_Z_code", &value1, &value2)) {
return NULL;
}
if (value1 != PyUnicode_AS_UNICODE(obj))
return raiseTestError("test_Z_code",
"Z code returned wrong value for 'test'");
Expand All @@ -1709,9 +1718,11 @@ test_Z_code(PyObject *self)
len2 = -1;

/* Test Z# for both values */
if (PyArg_ParseTuple(tuple, "Z#Z#:test_Z_code", &value1, &len1,
&value2, &len2) < 0)
if (!PyArg_ParseTuple(tuple, "Z#Z#:test_Z_code", &value1, &len1,
&value2, &len2))
{
return NULL;
}
if (value1 != PyUnicode_AS_UNICODE(obj) ||
len1 != PyUnicode_GET_SIZE(obj))
return raiseTestError("test_Z_code",
Expand Down Expand Up @@ -2033,17 +2044,19 @@ test_empty_argparse(PyObject *self)
tuple = PyTuple_New(0);
if (!tuple)
return NULL;
if ((result = PyArg_ParseTuple(tuple, "|:test_empty_argparse")) < 0)
if (!(result = PyArg_ParseTuple(tuple, "|:test_empty_argparse"))) {
goto done;
}
dict = PyDict_New();
if (!dict)
goto done;
result = PyArg_ParseTupleAndKeywords(tuple, dict, "|:test_empty_argparse", kwlist);
done:
Py_DECREF(tuple);
Py_XDECREF(dict);
if (result < 0)
if (!result) {
return NULL;
}
else {
Py_RETURN_NONE;
}
Expand Down Expand Up @@ -3698,8 +3711,9 @@ test_raise_signal(PyObject* self, PyObject *args)
{
int signum, err;

if (PyArg_ParseTuple(args, "i:raise_signal", &signum) < 0)
if (!PyArg_ParseTuple(args, "i:raise_signal", &signum)) {
return NULL;
}

err = raise(signum);
if (err)
Expand Down