Skip to content

bpo-34780: Fix potential hang during stdin initialization on Windows #9520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
izbyshev wants to merge 1 commit into from
Closed

bpo-34780: Fix potential hang during stdin initialization on Windows #9520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions Lib/test/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import warnings
import abc
import signal
import subprocess
import textwrap
import errno
from itertools import cycle, count
from collections import deque
Expand Down Expand Up @@ -3132,6 +3134,43 @@ def _test_nonblock_pipe_write(self, bufsize):
self.assertTrue(wf.closed)
self.assertTrue(rf.closed)

# bpo-34780
@unittest.skipUnless(sys.platform == 'win32', 'Win32-specific test')
def test_stdin_init_hang(self):
r, w = os.pipe()
self.addCleanup(os.close, r)
self.addCleanup(os.close, w)
code = textwrap.dedent("""
import sys
sys.stdout.write('OK')
sys.stdout.flush()
sys.stdin.read()
""")
p = subprocess.Popen([sys.executable, '-c', code],
stdin=r, stdout=subprocess.PIPE, bufsize=0)
try:
# Synchronize to increase chances that p is blocked in ReadFile()
# when p2 is started.
self.assertEqual(p.stdout.read(2), b'OK')
p.stdout.close()
p2 = subprocess.Popen([sys.executable, '-c', ''], stdin=r)
try:
start = time.time()
while time.time() - start < 10:
exit_code = p2.poll()
if exit_code is not None:
self.assertEqual(exit_code, 0)
break
time.sleep(0.1)
else:
self.fail('Child interpreter appears to hang')
finally:
p2.kill()
p2.wait()
finally:
p.kill()
p.wait()

class CMiscIOTest(MiscIOTest):
io = io
shutdown_error = "RuntimeError: could not find io module state"
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Windows/2018-09-23-23-02-46.bpo-34780.PIJ3OU.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix hang on startup if stdin refers to a pipe with an outstanding concurrent
operation on Windows.
3 changes: 2 additions & 1 deletion Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6915,7 +6915,8 @@ posix_fdopen(PyObject *self, PyObject *args)
PyMem_FREE(mode);
return posix_error();
}
#if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR)
#if !defined(MS_WINDOWS) && defined(HAVE_FSTAT) && defined(S_IFDIR) && \

@eryksun eryksun Sep 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unlikely but still possible that fd is a directory. For example, say the directory was opened with CreateFile (via ctypes, PyWin32, etc) and the file descriptor was opened with msvcrt.open_osfhandle. Granted the fstat check has never worked on Windows, but we could actually make it work rather than skip it:

BY_HANDLE_FILE_INFORMATION info;
HANDLE hfile = (HANDLE)_get_osfhandle(fd);
if (GetFileType(hfile) == FILE_TYPE_DISK && 
    GetFileInformationByHandle(hfile, &info) && 
    info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
    ...

@izbyshev izbyshev Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I considered that fd can refer to a directory, but I wasn't sure whether we should bother to fix dircheck in 2.7. I mentioned in the bug report that if we do want to fix dircheck, we may use 3.x approach, which is the same that you propose (in _Py_fstat_noraise). If I get more feedback suggesting to fix dircheck, I'll update the patch. Thank you!

@eryksun eryksun Sep 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion was only for this inline check in os.fdopen. I don't think dircheck in fileobject.c should even be called on Windows.

defined(EISDIR)
{
struct stat buf;
const char *msg;
Expand Down
3 changes: 2 additions & 1 deletion Objects/fileobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ file_PyObject_Print(PyObject *op, PyFileObject *f, int flags)
static PyFileObject*
dircheck(PyFileObject* f)
{
#if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR)
#if !defined(MS_WINDOWS) && defined(HAVE_FSTAT) && defined(S_IFDIR) && \

@eryksun eryksun Sep 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't open a directory via [_w]fopen in Windows, so how about just skipping the dircheck calls in fill_file_fields and open_the_file?

@izbyshev izbyshev Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's possible if we don't want to make dircheck work on Windows (I changed dircheck instead of its callers just because it makes a smaller patch). It also makes sense to put dircheck under #ifndef MS_WINDOWS in this case.

If dircheck is to be fixed instead, the call in fill_file_fields can't be removed because fill_file_fields is called by PyFile_FromFile which accepts FILE * of potentially unknown origin (not necessarily from fopen).

I may update the patch once there is a decision about fixing dircheck for Windows.

@eryksun eryksun Sep 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PyFile_FromFile I'm not really concerned about a C extension doing something silly. I don't think the check is necessary. It never worked, and no one ever complained that it didn't work, so my vote is to retain the status quo by either skipping the dircheck call or making it a no-op.

@izbyshev izbyshev Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see your points here and above, but is there any reason to make os.fdopen behave differently compared to PyFile_FromFile (and, as a result, stdin check)? Directory checks never worked in both of them.

@eryksun eryksun Sep 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning was that we can end up with an fd for a directory using PyWin32 and msvcrt.open_osfhandle. OTOH, C FILE streams are definitely outside of the domain of Python. If we consider code that directly calls CreateFile (via PyWin32 or ctypes) to be on the periphery, then the check in os.fdopen doesn't need to be fixed. It never worked, so the status quo is preserved by skipping it.

@izbyshev izbyshev Sep 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I understand your reasoning. A nitpick: we can still abuse PyFile_FromFile without a third-party extension module since it's used to wrap std streams.

import ctypes, msvcrt, subprocess, sys
h = ctypes.windll.kernel32.CreateFileA(
  ".",
  0,
  0,
  0,
  3,          # OPEN_EXISTING
  0x02000000, # FILE_FLAG_BACKUP_SEMANTICS 
  0,
)
fd = msvcrt.open_osfhandle(h, 0)
subprocess.check_call([sys.executable, '-c', 'import sys; sys.stdout.write("OK")'],
                      stdout=fd)

The above prints the following:

close failed in file object destructor:
sys.excepthook is missing
lost sys.stderr

Personally, I'd either fix both os.fdopen and PyFile_FromFile or preserve the status quo, and I'm more inclined to the latter.

defined(EISDIR)
struct stat buf;
int res;
if (f->f_fp == NULL)
Expand Down