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.
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
Use _set_thread_local_invalid_parameter_handler in posixmodule #67712
Comments
(Now that VS 2015 CTP 6 is public, here's the second half of bpo-23314.) In posixmodule.c is a big section with the following comment: /* Microsoft CRT in VS2005 and higher will verify that a filehandle is
...
We now have the less hacky way to avoid globally modifying the IPH, which is a new _set_thread_local_invalid_parameter_handler() function. (bpo-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 (bpo-3545 and bpo-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 |
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 :) ) |
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:
Same here, Tim ;) I've looked through the patch, and didn't see anything that scares me. |
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? |
bpo-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 (bpo-3545 and bpo-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. |
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 (bpo-3545 and bpo-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.
|
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. |
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': |
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: ====================================================================== 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 ====================================================================== 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': |
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.
|
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': |
Looks committed a way back to me. |
New changeset e88e2049b793 by Steve Dower in branch 'default': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: