[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Roundup Robot added the comment: New changeset e88e2049b793 by Steve Dower in branch 'default': Issue #23524: Finish removing _PyVerify_fd from sources https://hg.python.org/cpython/rev/e88e2049b793 -- ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Robert Collins added the comment: Looks committed a way back to me. -- nosy: +rbcollins resolution: - fixed stage: commit review - resolved status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Roundup Robot added the comment: 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 --

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Changes by Steve Dower steve.do...@microsoft.com: -- stage: patch review - commit review ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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). -- ___ Python tracker rep...@bugs.python.org

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: Victor - can you take a look? I'm keen to get this out of my patch queue :) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: I will try to take a look next week. If not, ping me. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: Updated the patch, mainly because of the changes that have gone in over the last week (especially _Py_read and _Py_write). -- Added file: http://bugs.python.org/file38615/23524_4.patch ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: I think my patch queue went bad somewhere - a few lines disappeared. New patch. -- Added file: http://bugs.python.org/file38616/23524_5.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: Any chance of a review on the newest patch? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___ ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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,

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Changes by STINNER Victor victor.stin...@gmail.com: -- Removed message: http://bugs.python.org/msg237501 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Changes by STINNER Victor victor.stin...@gmail.com: Removed file: http://bugs.python.org/file38383/fstat_ebadf.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Changes by STINNER Victor victor.stin...@gmail.com: -- Removed message: http://bugs.python.org/msg237501 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Roundup Robot added the comment: 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 -- ___ Python

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Roundup Robot added the comment: 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 -- nosy: +python-dev

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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. -- ___ Python tracker rep...@bugs.python.org

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: I reviewed 23524_hack.patch. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___ ___ Python-bugs-list

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Larry Hastings added the comment: FWIW I'm tagging alpha 2 somewhere around 24-36 hours from now. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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,

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: I reviewed 23524_2.patch. I vote -1 on the patch. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___ ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: Since we're doing alpha 2 this weekend, I'm keen to get this fix in. I would prefer a quickdirty 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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? -- ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Larry Hastings added the comment: I can live with that, if you're confident in it and you'll watch the buildbots. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Larry Hastings added the comment: 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! -- ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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. -- ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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? -- nosy: +larry, serhiy.storchaka priority: high - critical type: crash -

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: Builds fine on Ubuntu (sample size = 1, but it's about the best I can manage myself :) ) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Serhiy Storchaka added the comment: May be include this in Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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 :) ) -- ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

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! --

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: Just the current thread. When set, it overrides the global setting. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: #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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

STINNER Victor added the comment: 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? -- ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Steve Dower added the comment: 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. -- ___ Python tracker

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Zachary Ware added the comment: 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

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

Changes by Steve Dower steve.do...@microsoft.com: Added file: http://bugs.python.org/file38239/23524_1.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23524 ___

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

New submission from Steve Dower: (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,