classification
Title: Disabling CRT asserts in debug build
Type: crash Stage:
Components: Windows Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: db3l, python-dev, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2015-01-24 23:27 by steve.dower, last changed 2015-03-25 06:32 by steve.dower. This issue is now closed.

Files
File name Uploaded Description Edit
23314 Options.patch steve.dower, 2015-02-12 02:24 review
23314_tf_inherit_check.diff steve.dower, 2015-02-21 18:11 review
23314_tf_inhert_check_2.diff steve.dower, 2015-02-23 16:24 review
23314_tf_inhert_check_3.diff steve.dower, 2015-02-23 16:45 review
Messages (15)
msg234640 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-01-24 23:27
When we completely switch Windows builds over to VC14, we're going to encounter some new assert dialogs from the CRT. These won't appear in release builds, but because the buildbots run tests against debug builds they will get in the way. A number of tests attempt operations on bad file descriptors, which will assert and terminate in MSVCRT (I have a fix for the termination coming, but the assertions will still appear).

Currently regrtest has a "-n" flag to disable assertions using an MSVCRT function. This works fine with the exception of subprocesses, that do not inherit the setting.

The setting is process-wide, so we can't just turn assertions off around the code that may assert. We also can't turn them off completely without affecting users (e.g. #3545 and #4804).

#4804 has the discussion leading to the current state where _PyVerify_fd and the -n flag above exist. The _PyVerify_fd function could break with future CRT changes (bearing in mind that Python 3.5 and later will automatically use the latest installed CRT), for which there is an official workaround coming soon, except it won't suppress the assertion dialog for debug builds.

I'd like to propose three options:

1. Make faulthandler.enable() also disable assertions in debug builds. (Assertions are for debugging, and enabling faulthandler - at least on Windows - seems to suggest that you want to override the debugger. We also already enable faulthandler for subprocesses in the test suite.)

2. Add an environment variable (PYTHONDISABLECRTASSERT?) and use it to disable assertions on startup. (It looks like at least some tests deliberately block envvars flowing through to subprocesses, so this may require test changes too.)

3. Add a new xoption and use it to disable assertions on startup.

Obviously we could do all three, though I kind of like the first one best.

Currently, I'm planning to make debug builds available alongside the release versions, as they are necessary for people to build debuggable versions of their extensions, so this is more important than it was in the past. But the problem is very much only on the buildbots/in the test suite and not on user's machines.

Thoughts?
msg235791 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-12 02:24
Attached a patch with options 1 and 2 implemented - both are fairly trivial. Any thoughts/preferences?
msg235814 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-12 11:56
"When we completely switch Windows builds over to VC14, we're going to encounter some new assert dialogs from the CRT. (...) A number of tests attempt operations on bad file descriptors, which will assert and terminate in MSVCRT (I have a fix for the termination coming, but the assertions will still appear)."

Can you give some examples of tests which fail with an assertion error?

_PyVerify_fd() doesn't protect use against the assertion error?

I would prefer to keep CRT error checks, to protect us against bugs.

--

Instead of patching faulthandler, you should patch test.support.SuppressCrashReport.__enter__. This function already calls SetErrorMode(). Instead of ctypes, the function may be exposed in the _winapi module for example. I'm talking about SetErrorMode() *and* _CrtSetReportMode().

+#if defined MS_WINDOWS && defined _DEBUG
+    if ((p = Py_GETENV("PYTHONNOCRTASSERT")) && *p != '\0') {
+        _CrtSetReportMode(_CRT_ASSERT, 0);
+    }
+#endif

The function is not available if _DEBUG is not defined? Why not calling the function if _DEBUG is not defined?
msg235824 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-12 14:17
_Py_verifyfd has to go away, unfortunately. It requires inside knowledge of the exact CRT version, and with VC14 the CRT will automatically upgrade so that we're always using the latest.

I've gotten a function added to the CRT to make it unnecessary for release builds (which terminate on invalid fds). However, debug builds will display a message box still, which affects the buildbots. test already suppresses the dialogs for its own process, but when it creates subprocesses they don't inherit that setting.

An environment variable is an easy way to make sure that all subprocesses also have assert dialogs disabled. In release builds they are always disabled, hence the _DEBUG check.
msg235838 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-12 15:34
"An environment variable is an easy way to make sure that all
subprocesses also have assert dialogs disabled. In release builds they
are always disabled, hence the _DEBUG check."

If _PyVerify_fd() must go, I would prefer to always disable CRT check.
Otherwise, os.dup(<invalid fd>) would open the CRT assertion popup,
instead of raising OSError(EBADF). I prefer to have the same behaviour
with any version of the CRT and any version of the Visual Studio.

You may add an environment variable to explicitly *enable* the check,
if you want.

Programming with Python is different than programming with C. Python
always raise an OSError(EBADF) exception. It's not like the C
language, where you have to explicitly check the result of each
function call. It's difficult to ignore OSError without notifying it.

I don't know other CRT checks. Many other checks make sense for Python too?
msg235841 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-12 15:53
EBADF will still be returned; _PyVerify_fd is only there to prevent the assertion dialogs in debug builds. Release builds will not need _PyVerify_fd at all (though it's public, so it will remain, but it won't be necessary to protect calls into the CRT). As I said, there is a change coming in a CRT update that will make file calls behave more like POSIX, ie. returning EBADF instead of crashing.

See #4804 and #3545 for the discussions (and complaints) about turning off all assertion dialogs. Note that this issue only affects the dialogs in debug builds, and that for Python 3.5 we'll be making debug builds more widely available so that people can debug their extensions/hosts (see e.g. #22411).
msg235844 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-12 16:05
To be clearer (while still respecting the confidentiality agreement I'm under), previously this code would (if _DEBUG) display an assertion dialog and (regardless of _DEBUG) terminate the process:

    close(fd); // succeeds, assuming a good fd
    close(fd); // crashes here

This code would not display the dialog, but would still terminate:

    _CrtSetReportMode(_CRT_ASSERT, 0);
    close(fd);
    close(fd);

This code would not display the dialog or terminate, but depends on knowing implementation details of the CRT:

    if (!_PyVerify_fd(fd)) return posix_error()
    res = close(fd);
    if (res < 0) return posix_error()
    if (!_PyVerify_fd(fd)) return posix_error()
    res = close(fd);
    if (res < 0) return posix_error()

Soon we'll have a safe way (including in the presence of threads) to do this:

    _DONT_TERMINATE_ON_INVALID_PARAMETER_ON_THIS_THREAD
    res = close(fd);
    if (res < 0) return posix_error()
    res = close(fd); // would have terminated, but now returns EBADF
    if (res < 0) return posix_error()
    _ALLOW_TERMINATE_ON_INVALID_PARAMETER_ON_THIS_THREAD

In the last case, we prevent termination but can't safely suppress the _DEBUG dialog in the same way.

SuppressCrashHandler works for its purpose, since the OS error mode is inherited by child processes. The _CrtSetReportMode setting is *not* inherited, and so it needs to be called in every child process created by tests. Maybe it's possible to add the call into every test, but I'm skeptical (especially considering the multiprocessing tests).
msg236383 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-21 18:11
Having run some more tests, it may be that the only regular problem is in the test for inherited file descriptors.

I've attached a patch for tf_inherit_check.py that will prevent assert dialogs. It's not pretty, but it avoids touching the interpreter internals. I still prefer the environment variable option (and maybe add an underscore at the start/making it 'officially' undocumented?), as that will help avoid issues when new tests are added, but for now it looks like only this test needs fixing.
msg236446 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-23 14:41
23314_tf_inherit_check.diff: I would prefer to see this complex code in a function of the support module. For example, the SuppressCrashReport class is a good candidate.
msg236448 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-23 16:24
You're right, SuppressCrashReport also makes more sense here, though it still needs to be used explicitly in every new process. New patch attached.
msg236449 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-23 16:32
I reviewed  23314_tf_inhert_check_2.diff.
msg236451 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-02-23 16:45
Updated patch
msg236452 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-23 16:53
23314_tf_inhert_check_3.diff looks good to me.
msg236461 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-02-23 19:45
New changeset bb67b810aac1 by Steve Dower in branch 'default':
Issue 23314: SuppressCrashReports now disables CRT assertions
https://hg.python.org/cpython/rev/bb67b810aac1
msg239222 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-25 06:32
I haven't seen any of these in a while, and the buildbot at http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x (the only one running VS 2015 right now AFAIK) hasn't either, so I'm calling this fixed.
History
Date User Action Args
2015-03-25 06:32:35steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg239222
2015-02-27 07:33:13db3lsetnosy: + db3l
2015-02-23 19:45:30python-devsetnosy: + python-dev
messages: + msg236461
2015-02-23 16:53:50vstinnersetmessages: + msg236452
2015-02-23 16:45:20steve.dowersetfiles: + 23314_tf_inhert_check_3.diff

messages: + msg236451
2015-02-23 16:32:15vstinnersetmessages: + msg236449
2015-02-23 16:24:57steve.dowersetfiles: + 23314_tf_inhert_check_2.diff

messages: + msg236448
2015-02-23 14:41:10vstinnersetmessages: + msg236446
2015-02-21 18:11:07steve.dowersetfiles: + 23314_tf_inherit_check.diff

messages: + msg236383
2015-02-12 16:05:56steve.dowersetmessages: + msg235844
2015-02-12 15:53:08steve.dowersetmessages: + msg235841
2015-02-12 15:34:04vstinnersetmessages: + msg235838
2015-02-12 14:17:19steve.dowersetmessages: + msg235824
2015-02-12 11:56:15vstinnersetmessages: + msg235814
2015-02-12 02:24:48steve.dowersetfiles: + 23314 Options.patch
assignee: steve.dower
messages: + msg235791

keywords: + patch
2015-01-24 23:27:24steve.dowercreate