bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927

segevfiner · 2017-06-03T14:01:10Z

This works by not caching the handle and instead getting the handle from the file descriptor each time, so that if the actual handle changes by fd redirection closing/opening the console handle beneath our feet, we will keep working correctly.

I think I also fixed some resource handling issues along the way since ucrt/msvcrt fds own the HANDLEs they use and this module tried to close them itself.

See pytest-dev/py#103 for the issue it causes in Pytest. Also see: pytest-dev/pytest#2462. I was able to run the Pytest test suite, and my own little test described in the PR, to completion after this change.

Having your console handle closed by fd redirection is likely a change in the ucrt/msvcrt or a change in newer Windows versions (Console handles were rewritten in Windows 8).

This likely also affects any other module caching GetStdHandle or _get_osfhandle. For example colorama (pytest-dev/pytest#2465 - tartley/colorama#131), and I can think of "pyreadline", "win-unicode-console" and "click" probably suffering from the same issue but I haven't tested them.

I'm also not sure of the performance implications of this change.

This affects Python 3.6-3.7

P.S. python -m test -R 3:2 test.test_winconsoleio reports ref leaks on master for me 😛

CC @zooba @eryksun

bpo-30555

mention-bot · 2017-06-03T14:01:11Z

@segevfiner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zooba, @benjaminp and @serhiy-storchaka to be potential reviewers.

eryksun · 2017-06-04T00:42:11Z

Most of the reference leaks are due to initializing decodedname = Py_None followed by Py_INCREF(decodedname). It should be initialized as NULL.

Some of the reported ref leaks are from calling getwindowsversion in test_conout_path. This call can be moved out of the test by using a global variable, or by splitting the test up like this:

def conout_path_test(self, dos_device):
    temp_path = tempfile.mkdtemp()
    self.addCleanup(support.rmtree, temp_path)
    conout_path = os.path.join(temp_path, 'CONOUT$')

    with open(conout_path, 'wb', buffering=0) as f:
        if dos_device:
            self.assertIsInstance(f, ConIO)
        else:
            self.assertNotIsInstance(f, ConIO)

@unittest.skipIf(sys.getwindowsversion()[:2] <= (6, 1),
    "Windows 7 and earlier resolve CONOUT$ in a directory as a disk file")
def test_conout_path_device(self):
    self.conout_path_test(dos_device=True)

@unittest.skipIf(sys.getwindowsversion()[:2] > (6, 1),
    "Windows 8 and later resolve CONOUT$ in a directory as a DOS device")
def test_conout_path_file(self):
    self.conout_path_test(dos_device=False)

eryksun · 2017-06-04T00:56:40Z

Modules/_io/winconsoleio.c

@@ -133,6 +133,21 @@ char _PyIO_get_console_type(PyObject *path_or_fd) {
    return m;
 }

+static HANDLE py_get_osfhandle(int fd)


_Py_get_osfhandle, _Py_get_osfhandle_noraise, _Py_open_osfhandle, and _Py_open_osfhandle_noraise could be added in Python/fileutils.c. These CRT functions are called in fileutils.c, msvcrtmodule.c, posixmodule.c, mmapmodule.c, myreadline.c, _testconsole.c, and here.

Good idea.

Do you wish me to:

Do this entire refactor right here in this PR.

Add the functions and only use them in winconsoleio.c and _testconsole.c which I touched in this PR and leave the rest of the modules to a separate PR.

Leave this to a separate PR entirely.

?

I implemented this, with the full replacement in a separate commit just in case it's frowned upon by a core developer during review.

segevfiner · 2017-06-04T04:26:53Z

@eryksun So the leaks are real than, nice find! 😄 I can submit a PR/issue with your suggested fix if it helps you or just leave it to you. Why would calling sys.getwindowsversion in a test function leak though? Does that function leak? Or is there something else at play?

EDIT: I can also apply your fix right here if you wish me to.

eryksun · 2017-06-04T05:33:02Z

It should be ok to fix the _WindowsConsoleIO leak and rewrite the test in this issue. The ref leak in getwindowsversion needs a new issue. If you want to work on it, the problem is that it calls PyTuple_Pack directly on the int objects created from realMajor, etc. PyTuple_Pack increments the ref count of each object instead of stealing it.

…tion This works by not caching the handle and instead getting the handle from the file descriptor each time, so that if the actual handle changes by fd redirection closing/opening the console handle beneath our feet, we will keep working correctly. I think I also fixed some resource handling issues along the way since ucrt/msvcrt fds *own* the HANDLEs they use and this module tried to close them itself.

The change in test_winconsoleio.py is only to avoid the ref leak in `sys.getwindowsversion`.

eryksun · 2017-06-04T22:59:29Z

Include/fileutils.h

@@ -117,7 +117,15 @@ PyAPI_FUNC(int) _Py_dup(int fd);
 PyAPI_FUNC(int) _Py_get_blocking(int fd);

 PyAPI_FUNC(int) _Py_set_blocking(int fd, int blocking);
-#endif   /* !MS_WINDOWS */
+#else   /* MS_WINDOWS */
+PyAPI_FUNC(intptr_t) _Py_get_osfhandle_noraise(int fd);


I thought one of benefits would be using HANDLE in the signature instead of intptr_t, to avoid all of the casts. I guess we can't. How about void * like _PyOS_SigintEvent?

I think CPython explicity doesn't include Windows.h anywhere in it's public headers. fileutils.h is included by Python.h so I don't think I can really include Windows.h there. Ae such I don't have HANDLE defined and assuming that someone included Windows.h by himself before this header breaks horribly of course.

HANDLE is a typedef for void *, so we can use void * instead, like _PyOS_SigintEvent does.

eryksun · 2017-06-05T21:48:57Z

PC/msvcrtmodule.c

@@ -167,15 +167,7 @@ static long
 msvcrt_open_osfhandle_impl(PyObject *module, intptr_t handle, int flags)


The clinic definition could be updated here, e.g. to use a HANDLE type based on void *:

class HANDLE_converter(CConverter): type = 'void *' format_unit = '"_Py_PARSE_INTPTR"' class HANDLE_return_converter(CReturnConverter): type = 'void *' def render(self, function, data): self.declare(data) self.err_occurred_if("_return_value == INVALID_HANDLE_VALUE", data) data.return_conversion.append( 'return_value = PyLong_FromVoidPtr(_return_value);\n')

Then change the open_osfhandle parameter to handle: HANDLE and the get_osfhandle return type to -> HANDLE.

…f a cast

…consoleio-fd-redirection

segevfiner · 2017-06-08T20:50:24Z

Merged from master since @Haypo fixed the ref leak separately in #2003.

…consoleio-fd-redirection

segevfiner · 2017-06-10T07:44:47Z

Merged from master due to argument clinic changes.

…nverter

…consoleio-fd-redirection

Co-authored-by: Eryk Sun <eryksun@gmail.com>

zooba · 2021-04-23T22:00:58Z

Thanks for persisting with this, and sorry for taking so long! Let's put it in and deal with any fallout in 3.10 before backporting.

miss-islington · 2021-04-23T22:01:39Z

Thanks @segevfiner for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2021-04-23T22:01:39Z

Thanks @segevfiner for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2021-04-23T22:01:43Z

Sorry, @segevfiner and @zooba, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5e437fb872279960992c9a07f1a4c051b4948c53 3.9

miss-islington · 2021-04-23T22:01:45Z

Sorry @segevfiner and @zooba, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5e437fb872279960992c9a07f1a4c051b4948c53 3.8

miss-islington · 2021-04-23T22:02:08Z

Thanks @segevfiner for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2021-04-23T22:02:08Z

Thanks @segevfiner for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2021-04-23T22:02:10Z

Sorry, @segevfiner and @zooba, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5e437fb872279960992c9a07f1a4c051b4948c53 3.9

miss-islington · 2021-04-23T22:02:12Z

Sorry @segevfiner and @zooba, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5e437fb872279960992c9a07f1a4c051b4948c53 3.8

the-knights-who-say-ni added the CLA signed label Jun 3, 2017

nicoddemus mentioned this pull request Jun 3, 2017

[WIP] A workaround for Python 3.6 WindowsConsoleIO breaking with FDCapture pytest-dev/pytest#2462

Merged

4 tasks

eryksun reviewed Jun 4, 2017

View reviewed changes

segevfiner mentioned this pull request Jun 4, 2017

bpo-30567: Fix refleak in sys.getwindowsversion #1940

Merged

segevfiner added 5 commits June 4, 2017 21:32

bpo-30555: Fixed the kludgey PC/_testconsole.c, grumble grumble...

ac242eb

bpo-30555: Fix refleaks

645dfa3

The change in test_winconsoleio.py is only to avoid the ref leak in `sys.getwindowsversion`.

bpo-30555: Add _Py_{get,open}_osfhandle{,_noraise} and use them instead

013d8ea

bpo-30555: Swapped the rest of the uses of _{get,open}_osfhandle

aaf6020

segevfiner force-pushed the bpo-30555-windowsconsoleio-fd-redirection branch from ac3af9e to aaf6020 Compare June 4, 2017 18:35

eryksun reviewed Jun 4, 2017

View reviewed changes

bpo-30555: Swap intptr_t to void* in _Py_{get,open}_osfhandle{,_noraise}

7727bc5

segevfiner force-pushed the bpo-30555-windowsconsoleio-fd-redirection branch from 50ff023 to 7727bc5 Compare June 5, 2017 17:04

eryksun reviewed Jun 5, 2017

View reviewed changes

segevfiner added 2 commits June 6, 2017 01:47

bpo-30555: msvcrtmodule.c: Use an argument clinic convertor instead o…

0644634

…f a cast

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

82a18bb

…consoleio-fd-redirection

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

ad77cd3

…consoleio-fd-redirection

segevfiner added 4 commits June 30, 2017 17:38

bpo-30555: Also consider NULL as a possible error in HANDLE_return_co…

9dd4ea9

…nverter

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

07a979f

…consoleio-fd-redirection

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

69b13f7

…consoleio-fd-redirection

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

e54f9c0

…consoleio-fd-redirection

segevfiner requested a review from a team as a code owner December 16, 2017 10:17

Add NEWS.d entry for bpo-30555

2d252ab

segevfiner force-pushed the bpo-30555-windowsconsoleio-fd-redirection branch from c3953d5 to 2d252ab Compare December 16, 2017 10:26

Merge remote-tracking branch 'upstream/master' into bpo-30555-windows…

7990e20

…consoleio-fd-redirection

segevfiner requested review from lysnikolaou and pablogsal as code owners March 16, 2021 14:23

segevfiner and others added 2 commits March 16, 2021 16:24

fileno should error when closed

bd8233e

Co-authored-by: Eryk Sun <eryksun@gmail.com>

Fix faulty call to IOBase.close introduced by automatic merge

b76da3e

zooba merged commit 5e437fb into python:master Apr 23, 2021

bedevere-bot removed the awaiting review label Apr 23, 2021

zooba added needs backport to 3.8 needs backport to 3.9 only security fixes OS-windows labels Apr 23, 2021

miss-islington assigned zooba Apr 23, 2021

zooba added needs backport to 3.8 needs backport to 3.9 only security fixes and removed needs backport to 3.8 needs backport to 3.9 only security fixes labels Apr 23, 2021

zooba removed needs backport to 3.8 needs backport to 3.9 only security fixes labels Apr 23, 2021

segevfiner deleted the bpo-30555-windowsconsoleio-fd-redirection branch February 21, 2022 09:38

segevfiner mannequin mentioned this pull request Apr 10, 2022

_io._WindowsConsoleIO breaks in the face of fd redirection #74740

Closed

This was referenced Jan 5, 2024

gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls #13739

Closed

[os] pipe() creates inheritable FDs with a bad internal state in Windows #77046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927

bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927

segevfiner commented Jun 3, 2017 •

edited by erlend-aasland

Loading

mention-bot commented Jun 3, 2017

eryksun commented Jun 4, 2017

eryksun Jun 4, 2017

segevfiner Jun 4, 2017 •

edited

Loading

segevfiner Jun 4, 2017

segevfiner commented Jun 4, 2017 •

edited

Loading

eryksun commented Jun 4, 2017

eryksun Jun 4, 2017 •

edited

Loading

segevfiner Jun 5, 2017

eryksun Jun 5, 2017

eryksun Jun 5, 2017

segevfiner commented Jun 8, 2017

segevfiner commented Jun 10, 2017

zooba commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

		@@ -167,15 +167,7 @@ static long
		msvcrt_open_osfhandle_impl(PyObject *module, intptr_t handle, int flags)

bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927

bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927

Conversation

segevfiner commented Jun 3, 2017 • edited by erlend-aasland Loading

mention-bot commented Jun 3, 2017

eryksun commented Jun 4, 2017

eryksun Jun 4, 2017

Choose a reason for hiding this comment

segevfiner Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

segevfiner Jun 4, 2017

Choose a reason for hiding this comment

segevfiner commented Jun 4, 2017 • edited Loading

eryksun commented Jun 4, 2017

eryksun Jun 4, 2017 • edited Loading

Choose a reason for hiding this comment

segevfiner Jun 5, 2017

Choose a reason for hiding this comment

eryksun Jun 5, 2017

Choose a reason for hiding this comment

eryksun Jun 5, 2017

Choose a reason for hiding this comment

segevfiner commented Jun 8, 2017

segevfiner commented Jun 10, 2017

zooba commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

miss-islington commented Apr 23, 2021

segevfiner commented Jun 3, 2017 •

edited by erlend-aasland

Loading

segevfiner Jun 4, 2017 •

edited

Loading

segevfiner commented Jun 4, 2017 •

edited

Loading

eryksun Jun 4, 2017 •

edited

Loading