classification
Title: WindowsConsoleIO readall() fails if first line starts with Ctrl+Z
Type: behavior Stage: resolved
Components: IO, Library (Lib), Windows Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: eryksun, paul.moore, python-dev, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2016-09-15 03:25 by eryksun, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
issue_28162_01.patch eryksun, 2016-09-21 05:46 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (7)
msg276508 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-09-15 03:25
For a console readall(), if the first line starts with '\x1a' (i.e. Ctrl+Z), it breaks out of its read loop before incrementing len. Thus the input isn't handled properly as EOF, for which the check requires len > 0. Instead it ends up calling WideCharToMultiByte with len == 0, which fails as follows:

    >>> sys.stdin.buffer.raw.read()
    ^Z
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [WinError 87] The parameter is incorrect
msg276841 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-09-17 20:38
Unfortunately, very hard to test this because it requires interacting with an actual console. But it's an easy fix.
msg276845 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-17 20:51
New changeset d0bab9fda568 by Steve Dower in branch '3.6':
Issue #28161: Opening CON for write access fails
https://hg.python.org/cpython/rev/d0bab9fda568

New changeset 187a114b9ef4 by Steve Dower in branch 'default':
Issue #28161: Opening CON for write access fails
https://hg.python.org/cpython/rev/187a114b9ef4
msg276848 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-09-17 20:54
The fix here was changing "n > 0 && buf[len] == '\x1a'" into "n == 0 || buf[len] == '\x1a'" when returning an empty bytes. Previously we were falling into the conversion code which wasn't happy with n == 0.
msg277093 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-09-21 05:46
For breaking out of the readall while loop, you only need to check if the current read is empty:

        /* when the read is empty we break */
        if (n == 0)
            break;

Also, the logic is wrong here:

    if (len == 0 || buf[0] == '\x1a' && _buflen(self) == 0) {
        /* when the result starts with ^Z we return an empty buffer */
        PyMem_Free(buf);
        return PyBytes_FromStringAndSize(NULL, 0);
    }

This is true when len is 0 or when buf[0] is Ctrl+Z and _buflen(self) is 0. Since buf[0] shouldn't ever be Ctrl+Z here (low-level EOF handling is abstracted in read_console_w), it's never checking the internal buffer. We can easily see this going wrong here:

    >>> a = sys.stdin.buffer.raw.read(1); b = sys.stdin.buffer.raw.read()
    Ā^Z
    >>> a
    b'\xc4'
    >>> b
    b''

It misses the remaining byte in the internal buffer.

This check can be simplified as follows:

    rn = _buflen(self);

    if (len == 0 && rn == 0) {
        /* return an empty buffer */
        PyMem_Free(buf);
        return PyBytes_FromStringAndSize(NULL, 0);
    }

After this the code assumes that len isn't 0, which leads to more WideCharToMultiByte failure cases. 

In the last conversion it's overwrite bytes_size without including rn. 

I'm not sure what's going on with _PyBytes_Resize(&bytes, n * sizeof(wchar_t)). ISTM, it should be resized to bytes_size, and make sure this includes rn.

Finally, _copyfrombuf is repeatedly overwriting buf[0] instead of writing to buf[n]. 

With the attached patch, the behavior seems correct now:

    >>> sys.stdin.buffer.raw.read()
    ^Z
    b''

    >>> sys.stdin.buffer.raw.read()
    abc^Z
    ^Z
    b'abc\x1a\r\n'

Split U+0100:

    >>> a = sys.stdin.buffer.raw.read(1); b = sys.stdin.buffer.raw.read()
    Ā^Z
    >>> a
    b'\xc4'
    >>> b
b'\x80'

Split U+1234:

    >>> a = sys.stdin.buffer.raw.read(1); b = sys.stdin.buffer.raw.read()
    ሴ^Z
    >>> a
    b'\xe1'
    >>> b
    b'\x88\xb4'

The buffer still can't handle splitting an initial non-BMP character, stored as a surrogate pair. Both codes end up as replacement characters because they aren't transcoded as a unit.

Split U+00010000:

    >>> a = sys.stdin.buffer.raw.read(1); b = sys.stdin.buffer.raw.read()
    𐀀^Z
    ^Z
    >>> a
    b'\xef'
    >>> b
    b'\xbf\xbd\xef\xbf\xbd\x1a\r\n'
msg278320 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-08 19:38
New changeset 4d4aefa52f49 by Steve Dower in branch '3.6':
Issue #28162: Fixes Ctrl+Z handling in console readall()
https://hg.python.org/cpython/rev/4d4aefa52f49

New changeset 947fa496ca6f by Steve Dower in branch 'default':
Issue #28162: Fixes Ctrl+Z handling in console readall()
https://hg.python.org/cpython/rev/947fa496ca6f
msg278321 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-10-08 19:44
I didn't see your patch, but I made basically the same fix and added a test.
History
Date User Action Args
2017-03-31 16:36:32dstufftsetpull_requests: + pull_request1057
2016-10-08 19:44:31steve.dowersetstatus: open -> closed

messages: + msg278321
2016-10-08 19:38:08python-devsetmessages: + msg278320
2016-09-21 05:46:33eryksunsetstatus: closed -> open
files: + issue_28162_01.patch
messages: + msg277093

keywords: + patch
2016-09-17 20:54:15steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg276848

stage: needs patch -> resolved
2016-09-17 20:51:44python-devsetnosy: + python-dev
messages: + msg276845
2016-09-17 20:38:02steve.dowersetassignee: steve.dower
messages: + msg276841
stage: test needed -> needs patch
2016-09-15 03:25:17eryksuncreate