classification
Title: Issues with lazy fd support in _WindowsConsoleIO fileno() and close()
Type: behavior Stage: resolved
Components: IO, Windows Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: duplicate
Dependencies: Superseder: _io._WindowsConsoleIO breaks in the face of fd redirection
View: 30555
Assigned To: Nosy List: augustogoulart, eryksun, paul.moore, steve.dower, taleinat, tim.golden, zach.ware
Priority: normal Keywords: easy (C), patch

Created on 2018-07-22 09:05 by eryksun, last changed 2021-03-16 05:04 by eryksun. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9922 open augustogoulart, 2018-10-17 01:11
Messages (3)
msg322138 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2018-07-22 09:05
The _WindowsConsoleIO implementation of fileno should raise a ValueError if the internal handle value is INVALID_HANDLE_VALUE. Currently it raises io.UnsupportedOperation, and only if closefd=True. 

    >>> f = open('conin$', 'r')
    >>> f.close()
    >>> f.fileno()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    io.UnsupportedOperation: Console buffer does not support fileno

    >>> f = open(0, 'r', closefd=False)
    >>> f.close()
    >>> f.fileno()
    0

Also, if lazily opening an fd fails when opening by name (con, conin$, conout$), it should also close the file handle to maintain a consistent state. 

This can be fixed as follows: error out immediately and call err_closed instead of err_mode; and close the handle if _open_osfhandle fails (i.e. fd == -1) and closehandle is set:

    static PyObject *
    _io__WindowsConsoleIO_fileno_impl(winconsoleio *self)
    {
        if (self->handle == INVALID_HANDLE_VALUE) {
            return err_closed();
        }
        
        if (self->fd < 0) {
            _Py_BEGIN_SUPPRESS_IPH
            if (self->writable) {
                self->fd = _open_osfhandle((intptr_t)self->handle,
                                _O_WRONLY | _O_BINARY);
            } else {
                self->fd = _open_osfhandle((intptr_t)self->handle,
                                _O_RDONLY | _O_BINARY);
            }
            _Py_END_SUPPRESS_IPH
        }

        if (self->fd < 0) {
            if (self->closehandle) {
                CloseHandle(self->handle);
            }
            self->handle = INVALID_HANDLE_VALUE;
            return err_closed();
        }

        return PyLong_FromLong(self->fd);
    }

On a related note, there's a bug in internal_close that could potentially be a race condition that closes a handle to an unrelated object. If an fd has been opened, the CRT takes control of the handle. Calling close() will also close the underlying handle. In this case CloseHandle should not be called. This just needs a minor fix to add an `else` clause:

    static int
    internal_close(winconsoleio *self)
    {
        if (self->handle != INVALID_HANDLE_VALUE) {
            if (self->closehandle) {
                if (self->fd >= 0) {
                    _Py_BEGIN_SUPPRESS_IPH
                    close(self->fd);
                    _Py_END_SUPPRESS_IPH
                } else {
                    CloseHandle(self->handle);
                }
            }
            self->handle = INVALID_HANDLE_VALUE;
            self->fd = -1;
        }
        return 0;
    }
msg327395 - (view) Author: Augusto Goulart (augustogoulart) * Date: 2018-10-09 11:46
Hi everyone, I would like to let you know that I'm starting working on this one. Thanks!
msg388817 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2021-03-16 05:04
The issues brought up here are addressed in a more direct, comprehensive, and productive way in PR 1927 for bpo-30555.
History
Date User Action Args
2021-03-16 05:04:10eryksunsetstatus: open -> closed
superseder: _io._WindowsConsoleIO breaks in the face of fd redirection
messages: + msg388817

resolution: duplicate
stage: patch review -> resolved
2018-10-25 12:07:02taleinatsetnosy: + taleinat
2018-10-17 01:11:40augustogoulartsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request9275
2018-10-09 11:46:03augustogoulartsetnosy: + augustogoulart
messages: + msg327395
2018-07-22 09:05:49eryksuncreate