(Now that VS 2015 CTP 6 is public, here's the second half of #23314.)

In posixmodule.c is a big section with the following comment:

/* Microsoft CRT in VS2005 and higher will verify that a filehandle is
 * valid and raise an assertion if it isn't.
 * Normally, an invalid fd is likely to be a C program error and therefore
 * an assertion can be useful, but it does contradict the POSIX standard
 * which for write(2) states:
 *    "Otherwise, -1 shall be returned and errno set to indicate the error."
 *    "[EBADF] The fildes argument is not a valid file descriptor open for
 *     writing."
...
 * The structures below must be updated for each version of visual studio
 * according to the file internal.h in the CRT source, until MS comes
 * up with a less hacky way to do this.
 * (all of this is to avoid globally modifying the CRT behaviour using
 * _set_invalid_parameter_handler() and _CrtSetReportMode())
 */

We now have the less hacky way to avoid globally modifying the IPH, which is a new _set_thread_local_invalid_parameter_handler() function. (#23314 has hopefully already dealt with _CrtSetReportMode().)

The attached patch adds new macros within posixmodule.c to handle VS 2015 builds and use the new function. I have left the old code there as a transitional measure while people migrate to the new compiler, but also have no plans to remove it.

The new macros _Py_BEGIN_SUPPRESS_IPH and _Py_END_SUPPRESS_IPH should surround any code that may trigger the invalid parameter handler. The _Py_VERIFY_FD macro calls the _PyVerify_fd function when necessary - when the IPH can be suppressed, this should be a no-op, and when the IPH cannot be suppressed it must be a useful/safe check.

The _PyVerify_fd function remains defined in all cases, but when it's possible to safely call into the CRT to validate the handle we now do that instead of the old approach.

The only visible change should be that in debug builds you will see assertion dialogs if an invalid FD is encountered, which can be safely ignored. The test suite disables these dialogs already, so buildbots should be unaffected. Prior discussions (#3545 and #4804) have decided that it's important to leave these dialogs enabled for debug builds, and there is no way (and won't be any way) to disable these on a per-thread basis.

Turns out the old code no longer compiles without this change, as the internal variable we were previously using is no longer exported from the CRT.

Can I get a review please?

I turned in my Windows developer badge in 2007.  Can I recuse myself, pretty-please?

How about Tim Golden or Zach Ware?  Who I notice are conveniently already added to the nosy list!

The problem is that this isn't an area I'm particularly familiar with
(either in Python nor in Windows) so I need time to ramp up my awareness
of what Steve's proposing plus then assessing the change. I'll try...
but I rather hope Zach gets there first!

Larry - this may hold up the next release, so just keeping you in the loop. You don't have to review (though there are many changes in shared code, so you may not be useless :) )

> there is no way (and won't be any way) to disable these on a per-thread basis.

I don't understand. Is _set_thread_local_invalid_parameter_handler() process-wide, or dos it only affect the current thread?

Just the current thread. When set, it overrides the global setting.

Ah, but the bit you quoted is referring to the assert dialogs, which only exist in debug builds. The invalid parameter handler will normally kill the process even in release builds.

Tim Golden added the comment:
> The problem is that this isn't an area I'm particularly familiar with
> (either in Python nor in Windows) so I need time to ramp up my awareness
> of what Steve's proposing plus then assessing the change. I'll try...
> but I rather hope Zach gets there first!

Same here, Tim ;)

I've looked through the patch, and didn't see anything that scares me.
However, I can't claim to have a deep enough understanding of the
issue or the code to confidently say it's good and the right solution.
Also, I haven't had a chance to build or test it, only read through
it.

I don't understand the issue, can you please elaborate?

Can you please give an example of code which raise the bug, explain the behaviour on VS < 2015 and the behaviour on VS > 2015?

I don't understand why changes are restricted to posixmodule.c. Much more code manipulates file descriptors.

If Microsoft chose to kill a process when you pass an invalid file descriptor, why should Python behave differently? Is it only for Python unit test? If the problem only occurs with unit tests, why not only changing the behaviour with test.support.SuppressCrashReporter?

#4804 has most of the prior discussion, but here's some code that will cause the process to terminate:

import os
os.close(3)

The instant termination rather than OSError is why _PyVerify_fd exists at all, and that's only there because when the behaviour was disabled globally users complained (#3545 and #4804).

You are correct that _PyVerify_fd is used in more places where it should be updated (specifically _io/fileio.c and fileutils.c). It only makes sense where it's protecting a call into a CRT function though, so not all of these places should be changed. I'll make updates.

This is why I beg for reviews - I know that I'll miss things :)

May be include this in Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS?

I considered that, but then we'll be disabling the handler for calls into external modules (assuming whatever pyd layer exists is doing its job correctly), which is exactly where the error is most relevant.

> I considered that, but then we'll be disabling the handler for calls into external modules (assuming whatever pyd layer exists is doing its job correctly), which is exactly where the error is most relevant.

Hum ok. So I try to rephrase the issue.

When Python is compiled in debug mode, the Windows C runtime (CRT) kills the process when a fault is detected. A fault can be an invalid memory access, a C assertion ("assert(...);") which failed, etc. The problem is that the Python test suite explicitly call functions with invalid parameters to check that Python "handles correctly" errors. The problem is that unit tests expect that Python raises an exception, while the CRT kills the process by default.

You propose to modify the behaviour of the CRT of the current thread in the os module to raise a regular exception, instead of killing the process.

I agree with your suggestion :-)

We already use _PyVerify_fd() to raises an OSError(EBADF) exception, instead of killing the process when functions like os.close() are called with an invalid file descriptor.

We cannot disable globally CRT checks because users want them (#3545 and #4804) to validate their own C libraries. So CRT checks should only be disabled temporary when a Python function of the stdlib is called. We expect that calling a third party function kills the process if it is called with an invalid parameter. It's the purpose of the debug build.

(I didn't review the patch yet.)

Note: "kill the process" probably means that Windows opens a popup to ask to debug the application when a debugger is installed, instead of killing silently the application.

That's a pretty good summation, though it misses two points.

1. _PyVerify_fd no longer compiles.

2. The process will terminate in both release builds and debug builds. (In debug builds you also get a dialog letting you attach a debugger, unless those are suppressed as in #23314 and the test suite.)

New patch, which should cover all the other uses of _PyVerify_fd outside of posixmodule.

I've moved _PyVerify_fd into fileutils (but left _PyVerify_fd_dup2 in posixmodule, as it's basically deprecated at this point).

_Py_VERIFY_FD is now in fileutils.h, and is used everywhere it makes sense. I also fixed up some error handling for _Py_fstat that was using errno on Windows rather than GetLastError() - I can split this into a separate issue if it's in the way.

_Py_BEGIN/END_SUPPRESS_IPH are now in pymacro.h as they need to be after PyAPI_DATA is defined - the silent invalid parameter handler is now defined in PC/invalid_parameter_handler.c but setting and restoring it need to be in macros.

Builds are fine on VS 2015 CTP 6 (with this code enabled) and VS 2013 (with the old code enabled), and I'm getting set up to test a Linux build with the patch.

Builds fine on Ubuntu (sample size = 1, but it's about the best I can manage myself :) )

Since we're doing alpha 2 this weekend, I'm keen to get this fix in. Any comments from anyone? Or are people just generally okay with me checking it in and relying on the buildbots?

I can live with that, if you're confident in it and you'll watch the buildbots.

I reviewed 23524_2.patch. I vote -1 on the patch.

> Since we're doing alpha 2 this weekend, I'm keen to get this fix in.

I would prefer a quick&dirty fix: disable "IFD" checks for all threads by default. We will find a better fix for the next release. IMO we should discuss this issue more to check if there is no nicer option.

Happy to discuss further. I'll try and get a new patch done today that disables the handler for any threads Python creates a thread state for. The change to _Py_VERIFY_FD will still be there, but the begin/end suppress will go (using the verify function to protect CRT calls can go completely once we are prepared to force people to build with VC14, but I want to avoid that for as long as possible while we transition).

Also notice that Verify_fd is a race condition but was the best solution available at the time. I'm keen to see it go.

As far as a better solution goes, I've discussed this in depth with the main CRT developer at Microsoft and there won't be one. The best concession I could get was the thread local handler so we can disable it locally rather than process wide.

Added a patch that disables the invalid parameter handler in new_threadstate() so that all Python threads are protected from termination.

_PyVerify_fd is still moved into fileutils.c, but _Py_BEGIN/END_SUPPRESS_IPH and _Py_VERIFY_FD are gone. For VC14, _PyVerify_fd simply calls _get_osfhandle and checks the result.

As it's a hack, I haven't updated any documentation to draw attention to this, but I hope this is simpler enough that we can get Windows builds going again.

I reviewed 23524_hack.patch.

New patch based on review feedback - main change is to _Py_fstat to use errno on Windows instead of GetLastError().

Where to put the invalid parameter handler is still an open question. Victor is not keen on adding a new file, while I don't really want it to be outside of the PC/ folder. msvcrtmodule.c is next best IMHO, but since this won't be getting a Python API it still seems wrong.

Alternatively, I can rename invalid_parameter_handler.c to something like "PC/crtutils.c" so it stands out as a multi-purpose file?

FWIW I'm tagging alpha 2 somewhere around 24-36 hours from now.

23524_hack_2.patch looks good to me.

23524_hack_2.patch is fine to workaround the issue, but I agree that it's only a hack and it should be enhanced later.

New changeset 75aadb4450fd by Steve Dower in branch 'default':
Issue #23524: Replace _PyVerify_fd function with calling _set_thread_local_invalid_parameter_handler on every thread.
https://hg.python.org/cpython/rev/75aadb4450fd

Thanks. I'll be watching the buildbots closely, but hopefully there's no need to push quick fixes with the reduced change.

23524_2.patch is still on the table as the last-known-good fix, but I'll update it as soon as I can to take into account what's just been checked in.

test_os fails on Windows since the changeset 75aadb4450fd:
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5798/steps/test/logs/stdio

======================================================================
FAIL: test_15261 (test.test_os.StatAttributeTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 581, in test_15261
    self.assertEqual(ctx.exception.errno, errno.EBADF)
AssertionError: 22 != 9

======================================================================
FAIL: test_fstat (test.test_os.TestInvalidFD)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 1378, in check
    f(support.make_bad_fd(), *args)
FileNotFoundError: [WinError 18] There are no more files

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 1371, in helper
    self.check(getattr(os, f))
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_os.py", line 1380, in check
    self.assertEqual(e.errno, errno.EBADF)
AssertionError: 2 != 9

This is because of the _Py_fstat change to use errno instead of GetLastError. It seems other callers are relying on GetLastError to raise the correct exception, and that seems to be the standard throughout posixmodule as far as stat calls are concerned.

Best immediate fix is to revert back to how _Py_fstat was originally and update the caller's error handling, which I'll do now. Feel free to propose a more thorough patch that will unify error number handling, but I don't think it's a such a big problem.

New changeset d8e49a2795e7 by Steve Dower in branch 'default':
Issue #23524: Change back to using Windows errors for _Py_fstat instead of the errno shim.
https://hg.python.org/cpython/rev/d8e49a2795e7

fstat_ebadf.patch: _Py_fstat() now also set errno on Windows.

It's a little bit different than your patch, because it still calls SetLastError(ERROR_INVALID_HANDLE) to explicitly set the Windows error on bad file descriptor. We must set errno et SetLastError(): some callers check for errno, some callers uses GetLastError() (ex: path_error()).

I don't have access to Windows right now, I cannot test the patch. And it's maybe safer to wait after Python 3.5 alpha 2 release to apply it, if your latest change is enough to fix test_os on Windows.

I didn't know about winerror_to_errno(), so that might help. But there are other dependencies on _Py_fstat's error code throughout posixmodule.c, so I don't think this is sufficient. (For example, patherror() is already switched on OS to handle it correctly. I gave up tracking down all the uses because it hurt more than the horrific condition in fileio.c.)

New patch.

* Includes the _Py_fstat fix so that callers can use GetLastError for accurate info or errno for approximate info
* Reverts the _Py_VERIFY_FD proposal and just makes _PyVerify_fd a no-op except on VS 2010, 2012 and 2013. (After VS 2015 RC is released, I'm willing to remove _PyVerify_fd completely.)
* Still uses _Py_BEGIN/END_SUPPRESS_IPH to protect calls into the CRT where we can't ensure valid parameters in advance. (Chances are I haven't got all of these yet, but currently the test suite isn't revealing any others for me.)

Any chance of a review on the newest patch?

Updated the patch, mainly because of the changes that have gone in over the last week (especially _Py_read and _Py_write).

I think my patch queue went bad somewhere - a few lines disappeared. New patch.

I will try to take a look next week. If not, ping me.

Victor - can you take a look? I'm keen to get this out of my patch queue :)

I reviewed 23524_5.patch, I made some comments, but I now agree with the overall change (disable temporary the validation of invalid fd, set errno to EBADF instead).

New changeset 32652360d1c3 by Steve Dower in branch 'default':
Issue #23524: Replace _PyVerify_fd function with calls to _set_thread_local_invalid_parameter_handler.
https://hg.python.org/cpython/rev/32652360d1c3

Looks committed a way back to me.

New changeset e88e2049b793 by Steve Dower in branch 'default':
Issue #23524: Finish removing _PyVerify_fd from sources
https://hg.python.org/cpython/rev/e88e2049b793