classification
Title: Use _set_thread_local_invalid_parameter_handler in posixmodule
Type: compile error Stage: resolved
Components: Windows Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: larry, python-dev, rbcollins, serhiy.storchaka, steve.dower, tim.golden, vstinner, zach.ware
Priority: critical Keywords: patch

Created on 2015-02-26 05:54 by steve.dower, last changed 2016-09-08 18:22 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
23524_1.patch steve.dower, 2015-02-26 05:55 review
23524_2.patch steve.dower, 2015-02-27 18:33 review
23524_hack.patch steve.dower, 2015-03-06 00:26 review
23524_hack_2.patch steve.dower, 2015-03-06 16:05 review
fstat_ebadf.patch vstinner, 2015-03-08 02:29 review
23524_3.patch steve.dower, 2015-03-15 02:19 review
23524_4.patch steve.dower, 2015-03-21 03:30 review
23524_5.patch steve.dower, 2015-03-21 03:57 review
Messages (44)
msg236648 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-26 05:54
(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.
msg236757 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 14:56
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?
msg236758 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-02-27 14:58
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!
msg236759 - (view) Author: Tim Golden (tim.golden) * (Python committer) Date: 2015-02-27 15:00
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!
msg236765 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 15:29
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 :) )
msg236768 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-27 15:37
> 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?
msg236769 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 15:41
Just the current thread. When set, it overrides the global setting.
msg236770 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 15:42
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.
msg236771 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-02-27 15:52
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.
msg236772 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-27 15:58
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?
msg236774 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 16:08
#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 :)
msg236778 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-27 16:19
May be include this in Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS?
msg236780 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 16:26
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.
msg236783 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-27 16:40
> 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.
msg236785 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 16:44
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.)
msg236811 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 18:33
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.
msg236818 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-27 19:05
Builds fine on Ubuntu (sample size = 1, but it's about the best I can manage myself :) )
msg237240 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-05 04:35
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?
msg237241 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-03-05 04:46
I can live with that, if you're confident in it and you'll watch the buildbots.
msg237263 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-05 14:10
I reviewed 23524_2.patch. I vote -1 on the patch.
msg237264 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-05 14:11
> 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.
msg237270 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-05 15:32
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.
msg237310 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-06 00:26
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.
msg237343 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 11:27
I reviewed 23524_hack.patch.
msg237366 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-06 16:05
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?
msg237368 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-03-06 18:03
FWIW I'm tagging alpha 2 somewhere around 24-36 hours from now.
msg237390 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-06 22:44
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.
msg237391 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-06 22:47
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
msg237392 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-06 22:49
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.
msg237492 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-08 01:27
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
msg237499 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-08 02:15
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.
msg237500 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-03-08 02:16
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
msg237502 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-08 02:29
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.
msg237506 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-08 02:39
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.)
msg238116 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-15 02:19
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.)
msg238708 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-20 18:26
Any chance of a review on the newest patch?
msg238753 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-21 03:30
Updated the patch, mainly because of the changes that have gone in over the last week (especially _Py_read and _Py_write).
msg238756 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-21 03:57
I think my patch queue went bad somewhere - a few lines disappeared. New patch.
msg238784 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-21 10:55
I will try to take a look next week. If not, ping me.
msg240004 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-04-03 15:51
Victor - can you take a look? I'm keen to get this out of my patch queue :)
msg240389 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-04-09 21:00
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).
msg240538 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-04-12 04:32
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
msg247904 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-08-02 22:24
Looks committed a way back to me.
msg275101 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-08 18:22
New changeset e88e2049b793 by Steve Dower in branch 'default':
Issue #23524: Finish removing _PyVerify_fd from sources
https://hg.python.org/cpython/rev/e88e2049b793
History
Date User Action Args
2016-09-08 18:22:05python-devsetmessages: + msg275101
2015-08-02 22:24:00rbcollinssetstatus: open -> closed

nosy: + rbcollins
messages: + msg247904

resolution: fixed
stage: commit review -> resolved
2015-04-12 04:33:13steve.dowersetstage: patch review -> commit review
2015-04-12 04:32:35python-devsetmessages: + msg240538
2015-04-09 21:00:03vstinnersetmessages: + msg240389
2015-04-03 15:51:28steve.dowersetmessages: + msg240004
2015-03-21 10:55:09vstinnersetmessages: + msg238784
2015-03-21 03:57:40steve.dowersetfiles: + 23524_5.patch

messages: + msg238756
2015-03-21 03:30:13steve.dowersetfiles: + 23524_4.patch

messages: + msg238753
2015-03-20 18:26:11steve.dowersetmessages: + msg238708
2015-03-15 02:19:52steve.dowersetfiles: + 23524_3.patch

messages: + msg238116
2015-03-08 02:39:29steve.dowersetmessages: + msg237506
2015-03-08 02:30:15vstinnersetmessages: - msg237501
2015-03-08 02:29:55vstinnersetfiles: - fstat_ebadf.patch
2015-03-08 02:29:24vstinnersetfiles: + fstat_ebadf.patch

messages: + msg237502
2015-03-08 02:29:23vstinnersetfiles: + fstat_ebadf.patch

messages: + msg237501
2015-03-08 02:16:53python-devsetmessages: + msg237500
2015-03-08 02:15:23steve.dowersetmessages: + msg237499
2015-03-08 01:27:39vstinnersetmessages: + msg237492
2015-03-06 22:49:26steve.dowersetmessages: + msg237392
2015-03-06 22:47:42python-devsetnosy: + python-dev
messages: + msg237391
2015-03-06 22:44:19vstinnersetmessages: + msg237390
2015-03-06 18:03:59larrysetmessages: + msg237368
2015-03-06 16:05:40steve.dowersetfiles: + 23524_hack_2.patch

messages: + msg237366
2015-03-06 11:27:47vstinnersetmessages: + msg237343
2015-03-06 00:26:43steve.dowersetfiles: + 23524_hack.patch

messages: + msg237310
2015-03-05 15:32:05steve.dowersetmessages: + msg237270
2015-03-05 14:11:17vstinnersetmessages: + msg237264
2015-03-05 14:10:01vstinnersetmessages: + msg237263
2015-03-05 04:46:02larrysetmessages: + msg237241
2015-03-05 04:35:14steve.dowersetmessages: + msg237240
2015-02-27 19:05:16steve.dowersetmessages: + msg236818
2015-02-27 18:33:51steve.dowersetfiles: + 23524_2.patch

messages: + msg236811
2015-02-27 16:44:37steve.dowersetmessages: + msg236785
2015-02-27 16:40:59vstinnersetmessages: + msg236783
2015-02-27 16:26:45steve.dowersetmessages: + msg236780
2015-02-27 16:19:44serhiy.storchakasetmessages: + msg236778
2015-02-27 16:08:53steve.dowersetmessages: + msg236774
2015-02-27 15:58:01vstinnersetmessages: + msg236772
2015-02-27 15:52:19zach.waresetmessages: + msg236771
2015-02-27 15:42:57steve.dowersetmessages: + msg236770
2015-02-27 15:41:45steve.dowersetmessages: + msg236769
2015-02-27 15:37:54vstinnersetmessages: + msg236768
2015-02-27 15:29:33steve.dowersetmessages: + msg236765
2015-02-27 15:00:39tim.goldensetmessages: + msg236759
2015-02-27 14:58:51larrysetmessages: + msg236758
2015-02-27 14:56:04steve.dowersetpriority: high -> critical

nosy: + larry, serhiy.storchaka
messages: + msg236757

type: crash -> compile error
2015-02-26 05:55:50steve.dowersetfiles: + 23524_1.patch
2015-02-26 05:54:17steve.dowercreate