bpo-31243: fix PyArg_ParseTuple failure checks #3171

orenmn · 2017-08-21T19:00:56Z

according to http://bugs.python.org/issue31243, replace checks whether PyArg_ParseTuple returned a negative value with checks whether PyArg_ParseTuple returned 0.

while we are here, make sure the call to PyArg_ParseTuple in textiowrapper_read_chunk() won't cause producing a wrong error message, as explained in http://bugs.python.org/issue28261, or a SystemError.

https://bugs.python.org/issue31243

orenmn · 2017-08-22T11:53:24Z

Modules/_io/textio.c

@@ -1498,15 +1498,26 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint)
        /* Given this, we know there was a valid snapshot point


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?

What do you think @benjaminp?

serhiy-storchaka · 2017-08-23T08:02:17Z

Lib/test/test_io.py

+            class BadDecoder:
+                def getstate(self):
+                    return getstate_ret_val
+            class BadIncrementalDecoder:


Can this be just a function?

definitely. sorry for that.

serhiy-storchaka · 2017-08-23T08:03:08Z

Lib/test/test_io.py

+                def __call__(self, dummy):
+                    return BadDecoder()
+            quopri = codecs.lookup("quopri")
+            quopri_decoder = quopri.incrementaldecoder


You can use test.support.swap_attr().

that's awesome.
BTW, except for its docstring, could it be that swap_attr() is not documented?

The test.support module is for internal use, and most of its content is not documented. If you want to add a documentation, open a separate issue.

serhiy-storchaka · 2017-08-23T08:08:06Z

Modules/_io/textio.c

            Py_DECREF(state);
            return -1;
        }

        if (!PyBytes_Check(dec_buffer)) {
            PyErr_Format(PyExc_TypeError,
-                         "decoder getstate() should have returned a bytes "
-                         "object, not '%.200s'",
+                         "decoder getstate() should have returned a tuple "


Isn't there too much attention for rare errors? Wouldn't be enough just "illegal decoder state" for all three checks?

I thought it would be more helpful for users, but if you think this is an overkill, I would change it.
BTW, there is another "decoder getstate() should have returned ..." error message, in DECODER_GETSTATE(). should I change it also to "illegal decoder state" for consistency?
(the PyArg_ParseTuple before that one is part of bpo-28261. I am working on a patch for it, and I guessed we shouldn't fix it here.)

Good point. Similar changes should be made in DECODER_GETSTATE().

I think it is worth to keep the wrong type name in this error, since returning a bytearray or a memoryview is more common error. But it would be nice to keep it short and make all three error messages similar.

For example, just "illegal decoder state" for first two cases and "illegal decoder state: the first item should be a bytes object, not '%.200s'" in the third case.

serhiy-storchaka · 2017-08-24T08:59:36Z

Modules/_io/textio.c

@@ -2395,8 +2392,8 @@ _io_TextIOWrapper_tell_impl(textio *self)
        } \


Add also an explicit check for a tuple.

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

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

of course, I would add it soon

serhiy-storchaka · 2017-08-24T09:05:17Z

Please add also a NEWS.d entry. This change fixes bugs and the information about this can be important for end users.

orenmn · 2017-08-24T10:37:08Z

please ignore this NEWS.d. it is not accurate. I would fix it soon.

(cherry picked from commit ba7d736)

bedevere-bot · 2017-08-29T10:54:58Z

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

) (cherry picked from commit ba7d736)

bedevere-bot · 2017-08-29T15:05:37Z

GH-3235 is a backport of this pull request to the 2.7 branch.

)

orenmn added 2 commits August 21, 2017 16:44

init commit

14add40

improve a comment, and remove parenthese from class definition

d5575f6

the-knights-who-say-ni added the CLA signed label Aug 21, 2017

bedevere-bot added the awaiting review label Aug 21, 2017

orenmn changed the title ~~bpo-31243 fix PyArg_ParseTuple failure checks~~ bpo-31243: fix PyArg_ParseTuple failure checks Aug 21, 2017

orenmn commented Aug 22, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 22, 2017

serhiy-storchaka reviewed Aug 23, 2017

View reviewed changes

serhiy-storchaka requested a review from benjaminp August 23, 2017 08:09

shorten the error messages and improve the test code.

99e8405

serhiy-storchaka reviewed Aug 24, 2017

View reviewed changes

added NEWS.d

c8a21ba

orenmn and others added 2 commits August 24, 2017 14:12

fix NEWS.d item

a3a1212

No need to mention my name specially.

f7f38c7

serhiy-storchaka approved these changes Aug 28, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 28, 2017

serhiy-storchaka merged commit ba7d736 into python:master Aug 29, 2017

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

orenmn added a commit to orenmn/cpython that referenced this pull request Aug 29, 2017

[3.6] bpo-31243: Fixed PyArg_ParseTuple failure checks. (pythonGH-3171)

725c2e1

(cherry picked from commit ba7d736)

bedevere-bot removed the needs backport to 3.6 label Aug 29, 2017

serhiy-storchaka pushed a commit that referenced this pull request Aug 29, 2017

[3.6] bpo-31243: Fixed PyArg_ParseTuple failure checks. (GH-3171) (#3233

c7750c2

) (cherry picked from commit ba7d736)

orenmn mentioned this pull request Aug 29, 2017

[2.7] bpo-31243: Fixed PyArg_ParseTuple failure checks. (GH-3171) #3235

Merged

bedevere-bot removed the needs backport to 2.7 label Aug 29, 2017

serhiy-storchaka pushed a commit that referenced this pull request Aug 29, 2017

[2.7] bpo-31243: Fixed PyArg_ParseTuple failure checks. (GH-3171) (#3235

20958e6

)

GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017

bpo-31243: Fixed PyArg_ParseTuple failure checks. (python#3171)

5c3f910

Mariatta removed the awaiting merge label Oct 8, 2017

		@@ -1498,15 +1498,26 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint)
		/* Given this, we know there was a valid snapshot point

		@@ -2395,8 +2392,8 @@ _io_TextIOWrapper_tell_impl(textio *self)
		} \

Uh oh!

bpo-31243: fix PyArg_ParseTuple failure checks #3171

bpo-31243: fix PyArg_ParseTuple failure checks #3171

Uh oh!

Conversation

orenmn commented Aug 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Aug 24, 2017

Uh oh!

orenmn commented Aug 24, 2017

Uh oh!

bedevere-bot commented Aug 29, 2017

Uh oh!

bedevere-bot commented Aug 29, 2017

Uh oh!

Uh oh!

orenmn commented Aug 21, 2017 •

edited by bedevere-bot

Loading