bpo-40638: Check for attribute lookup failure in builtin_input_impl #20125
Conversation
153b866 to
8eaabdb
Compare
There was a problem hiding this comment.
Thanks for fixing this @tekknolagi, the change looks correct but does not conform to the coding style.
Python/bltinmodule.c
Outdated
| @@ -2010,16 +2010,16 @@ builtin_input_impl(PyObject *module, PyObject *prompt) | |||
|
|
|||
| /* stdin is a text stream, so it must have an encoding. */ | |||
| stdin_encoding = _PyObject_GetAttrId(fin, &PyId_encoding); | |||
| if (!stdin_encoding || !PyUnicode_Check(stdin_encoding)) | |||
There was a problem hiding this comment.
This should be
if (!stdin_encoding || !PyUnicode_Check(stdin_encoding)) {
goto _readline_disable_tty_and_fall_back;
}as it is specified in PEP 7 (https://www.python.org/dev/peps/pep-0007/#code-lay-out):
All new C code requires braces
Add tests to cover this case.
|
Fixed.
…On 6/5/20, Rémi Lapeyre ***@***.***> wrote:
@remilapeyre commented on this pull request.
Thanks for fixing this @tekknolagi, the change looks correct but does not
conform to the coding style.
> @@ -2010,16 +2010,16 @@ builtin_input_impl(PyObject *module, PyObject
> *prompt)
/* stdin is a text stream, so it must have an encoding. */
stdin_encoding = _PyObject_GetAttrId(fin, &PyId_encoding);
+ if (!stdin_encoding || !PyUnicode_Check(stdin_encoding))
This should be
```c
if (!stdin_encoding || !PyUnicode_Check(stdin_encoding)) {
goto _readline_disable_tty_and_fall_back;
}
```
as it is specified in PEP 7
(https://www.python.org/dev/peps/pep-0007/#code-lay-out):
> All new C code requires braces
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#20125 (review)
|
8eaabdb to
400bef9
Compare
There was a problem hiding this comment.
Thanks, can you add a blurb to document the change? You can add one by using https://blurb-it.herokuapp.com/
There was a problem hiding this comment.
Thanks for taking the time to fix this, @tekknolagi, and sorry for the long delay in reviewing!
I've got a few minor comments.
| # input() in tty mode with a malformed input stream should attempt | ||
| # to call .readline() | ||
| sys.stdin = MalformedInputStream() | ||
| sys.stdout = MalformedOutputStream() | ||
| self.assertEqual(input("baz"), "foobar") # strips \n | ||
| self.assertEqual(sys.stdout.value, "baz") | ||
|
|
There was a problem hiding this comment.
Having this here is slightly confusing, as it appears like it may affect the following assertion as well. Perhaps put this at the end of this method or in a separate method.
| # input() in tty mode with a malformed input stream should attempt | ||
| # to call .readline() | ||
| sys.stdin = MalformedInputStream() | ||
| sys.stdout = MalformedOutputStream() |
There was a problem hiding this comment.
Have you considered using a StringIO object for sys.stdout here?
| @@ -75,6 +75,23 @@ class BitBucket: | |||
| def write(self, line): | |||
| pass | |||
|
|
|||
| class MalformedInputStream: | |||
There was a problem hiding this comment.
"Malformed" in what way? I think this name could be better.
| class MalformedInputStream: | |
| class InputStreamWithReadlineButNoRead: |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
The following commit authors need to sign the Contributor License Agreement: |
Add tests to cover this case.
https://bugs.python.org/issue40638