[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Roundup Robot]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Robert Collins]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Roundup Robot]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

Changes by Steve Dower steve.do...@microsoft.com:


--
stage: patch review - commit review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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, 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.)

--
Added file: http://bugs.python.org/file38490/23524_3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 gave up tracking down all the uses because it 
hurt more than the horrific condition in fileio.c.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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

--
Added file: http://bugs.python.org/file38384/fstat_ebadf.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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

--
Added file: http://bugs.python.org/file38383/fstat_ebadf.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Roundup Robot]

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 tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Roundup Robot]

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

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 checked in.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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

--
Added file: http://bugs.python.org/file38360/23524_hack_2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

STINNER Victor added the comment:

I reviewed 23524_hack.patch.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Larry Hastings]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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, _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.

--
Added file: http://bugs.python.org/file38350/23524_hack.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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 nicer option.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Larry Hastings]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Larry Hastings]

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 rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 suppressed as in #23314 and the test suite.)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 - compile error

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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

--
Added file: http://bugs.python.org/file38264/23524_2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Serhiy Storchaka]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Tim Golden]

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!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 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 :)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[STINNER Victor]

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?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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 rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Zachary Ware]

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue23524] Use _set_thread_local_invalid_parameter_handler in posixmodule

[Steve Dower]

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, 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.

--
assignee: steve.dower
components: Windows
keywords: patch
messages: 236648
nosy: haypo, steve.dower, tim.golden, zach.ware
priority: high
severity: normal
stage: patch review
status: open
title: Use _set_thread_local_invalid_parameter_handler in posixmodule
type: crash
versions: Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue23524
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com