This block in exceptions.c:

#if defined _MSC_VER && _MSC_VER >= 1400 && defined(__STDC_SECURE_LIB__)
...
    /* turn off assertions in debug mode */
    prevCrtReportMode = _CrtSetReportMode(_CRT_ASSERT, 0);
#endif

Does exactly what the comment says it does - disables all assertions. 
It disables *all* CRT assertions in the process, which includes some
very useful ones related to memory corruption, and all 'assert' and
'ASSERT' statements in all Python extension modules.

The change was introduced in:
"""
r46894 | kristjan.jonsson | 2006-06-13 01:45:12 +1000 (Tue, 13 Jun 2006)
| 2 lines

Fix the CRT argument error handling for VisualStudio .NET 2005.  Install
a CRT error handler and disable the assertion for debug builds.  This
causes CRT to set errno to EINVAL.
This update fixes crash cases in the test suite where the default CRT
error handler would cause process exit.
"""

which seems like a very large hammer to be using (ie, the problem
causing the assertions to blow should probably have been fixed).  I'd
like to remove these 2 lines.  Any objections, or should I upload the
trivial patch?

But then you get a nasty pop-up window on code like this:

>>> fd = os.open("foo", 0)
>>> os.close(fd)
>>> os.close(fd)

Instead of a gentle "OSError: [Errno 9] Bad file descriptor"

Yes, this is the biggest reason why this is required.
This, and and os.closerange().
I have browsed the CRT source and I find no way to determine if a fd is actually valid, before calling any of the functions that check for it.
Reintroducing the assertion will prevent a debug build python from passing the testsuite.
Much as I think this is useful (turning on the crt tests helped me find a couple of python bugs recently) it unfortunately needs to be off because of os.close().  All other troublesome cases (strftime(), open()) have workarounds.

Kristján

Btw,
It occurred to me that a more gentle way to do this is to disable the functionality only around close().
This is far less intrusive, for example, in an embedded environment.
I must confess that I had completely forgototten about the disabling of the assertion and only stumbled on this recently when I was trying to find out why none of my _ASSERTS in eve worked!  Making wholesale CRT settings certainly should be avoided.

So, as I said, we could very well avoid this by just wrapping a few instances of close() with a CrtSetErrorHandler calls and friends.  With the caveat that these are not threadsafe, that is, it is a process global setting for the CRT.  But toggling it during a limited scope surely is less intrusive than modifying it for the entire process permanently.

Kristján

It would be interesting to know which tests actually fail.  If the tests
are explicitly checking a bad fd, then IMO it makes more sense for that
test to simply be avoided in debug builds and nothing of value would be
left untested.  OTOH, I'd also be happy with just disabling them around
the 2 functions known to cause issues - anything other than leaving
assertions globally disabled for the process!

I guess another option is to expose it via msvcrt and let the test
themselves disable it?

Ok, I'll prepare a patch to the trunk.  I've had some experience with this and should have the code lying around somewhere.  I'll let you know once I´ve created an issue.
K

As long as the CRT contains bogus assertions (that violate standard C),
I think Python rightfully should continue to disable assertions. If
applications desire assertions, they should explicitly turn them on
(provided there is an API to do so).

We've had this discussion before, you know, 2006.
MS is asserting on preconditions that are undefined by the standard.
As such, they are not in violation.  In fact, it is foolish by us to invoke undefined behaviour, because it may, proverbially, result in the formatting of the hard drive :)

K

> We've had this discussion before, you know, 2006.
> MS is asserting on preconditions that are undefined by the standard.
> As such, they are not in violation.

I remember, and you were already wrong back then :-)

MS asserts (in winsig.c) that the signal number in signal() is one
of the explicit list of supported or ignored signals; if it isn't,
it aborts. This is in violation to ISO C, 7.14.1.1p8, which specifies
that SIG_ERR must be returned and errno set.

It is true that Python *also* invokes undefined behavior in certain
cases; contributions to eliminate these are welcome.

Can anyone point me at a test that failed in this case?  A quick look
over winsig.c shows no asserts.  I didn't compare the vs2005 crt source
though, so its highly likely I just missed it...

On the broader point, it could be argued that if it is the apps
responsibility to re-enable assertions, it is also the apps
responsibility to disable them in the first place.  If other libraries
on the system attempted the same as Python, it would obviously be
undefined if Python's attempt had any actual affect - the last called
would win.  As we found with the old locale() issues, we really can't
rely on Python setting a global shared state to keep things nice.  From
a pragmatic POV, if we know the MS crt asserts on certain constants
signal numbers, can't we just avoid passing those numbers?

> Can anyone point me at a test that failed in this case?

See issue 1350409 (and subsequently #1167262 and #1311784). Python
fails to start when assertions are turned on (haven't tested for
VS 2008, though).

If you were looking for a test case that fails specifically - there
might none. However, try Amaury's example.

> On the broader point, it could be argued that if it is the apps
> responsibility to re-enable assertions, it is also the apps
> responsibility to disable them in the first place.

Ideally, assertions should be enabled in debug builds, and disabled
in release builds, and there should be no runtime configuration.
Unfortunately, Microsoft chose to add bogus assertions, so this general
rule is useless within the context of MSC.

(it's not just that the signal issue is a standards violation. Assuming
that _close apparently follows POSIX close, it should always set
errno to EBADF for a double-close, rather than raising an assertion. It
is conforming in release mode, but loses conformance in debug mode)

> From
> a pragmatic POV, if we know the MS crt asserts on certain constants
> signal numbers, can't we just avoid passing those numbers?

It's the other way 'round: it rejects all but a list of known signals.
Still, it would be possible to hack around - one such hack is currently
implemented.

I submit to you a patch to the trunk which narrows down the problem.  In  
stead of throwing the switch for the whole process, we selectively 
disable the hard error checking for those two cases that require it:  
The file open mode string (unless we want to reimplement the crt mode 
string parser) and the posix file descriptor methods.  The remaining 
case, strftime, we fix with a proper argument checking function.

Yesterday I put in some tests in to the testsuite to excercise this 
functionality (revision 68547)

> I submit to you a patch to the trunk which narrows down the problem.  In  
> stead of throwing the switch for the whole process, we selectively 
> disable the hard error checking for those two cases that require it:  

-1. Setting the CRT report mode is not thread-safe, so this code may
randomly affect concurrent threads.

Martin,
  Would you be happier if this functionality was exposed via _msvcrt and
disabled in the test suite (either globally or selectively)?  Obviously
this is still not thread-safe, but it is closer towards putting this
behaviour in the control of the app itself.

>   Would you be happier if this functionality was exposed via _msvcrt and
> disabled in the test suite (either globally or selectively)?  Obviously
> this is still not thread-safe, but it is closer towards putting this
> behaviour in the control of the app itself.

Adding stuff into _msvcrt is fine with me. I guess the debate then might
be what the default should be. I personally don't care at all, as I
rarely ever use the debug build on Windows in the first place.

It is not an issue with debug builds only.  The crt error handler is 
currently also reset globally for all builds which is very evil.

The current way we do thing is also not thread safe WRT other 
application threads.  On second thought though it is better to at least 
keep this modification within the bounds of the GIL.  I'll change the 
patch for that.

IMHO, the issue for the posix fd functions isn't that great, since 
hardly anyone would want to use them on windows.  The main issue is 
still the open() call.  This one is possible to avoid too without 
invoking changin the crt behaviour, by simply ramping up the 
_sanitize_mode function.

I'll put in some more effort then, because I think it is really 
important for Python to leave the crt settings alone, and still be sure 
that random input can't have unforeseen consequences.

Okay, here is a second patch.
open now verifies the 'mode' string properly, and all uses of a 'fp' 
turn off the CRT error handling temporarily, while holding the GIL

Kristjan, please understanding that setting the CRT error handling is
not thread-safe, and trying to do it in a fine-grained way can cause all
kinds of crazy races. With your patch, the following sequencce of events
is possible if two threads T1 and T2 simultaneously try to access the CRT:

1. T1 saves the old mode (O), installs its own mode (N1)
2. T1 releases the GIL, invokes CRT operation
3. T2 starts running, saves the old mode (N1), installs its own mode (N2)
4. T1 completes, restores the old mode (O)
5. T2 completes, restores the old mode (N1)

As a net result, the original error handling is *not* restored.

I understand thread-safe.  This usage is safe from Python threads 
because it is all done in the context of the GIL.

It is, however, unsafe if some other random thread is also modifying 
these settings during runtime.

In your example, T2 doesn't hold the GIL and so, this is the scenario 
that I believe you are invoking.

This is however not likely to be the case because these settings are 
normally left alone.  The only reason we have to worry about this is 
because we are allowing through file descriptors that are out of the 
control of compiled C code.  Nobody is likely to be messing with this 
stuff except for us.

Furthermore, since your argument assumes a rogue thread modifying the 
CRT settings, this thread may just as well be active during startup 
when we are modifying these values under the current system.  So there 
is not a fundamental difference here, only a difference in scale.

I think that the drawbacks of modifying the CRT behaviour unexpectedly 
for all code in the process far outweigh the risk of there being 
another unknown thread also heavily modifying these obscure settings.

Now, I can see a compromise we could make.  I could make this a compile 
time choice.

Ooops, ignore my last comment.
I just realized the point you were making Martin, and it doesn't 
involve rogue threads at all.

Hmm, back to the drawing board, I suppose.

Here is what I think is a better attempt at the selective-disabling.  I 
did away with the messy macros and added a function in errors.c.  This 
maintans a global 'level' variable, implicitly guarded by the GIL.  Now 
only the thread that first enters a guarded block will set the hendler, 
and the thread that is last to leave will reset it.  This should 
guarantee that between the two macros, our custom handler is set, and 
that no thread pulls the rug from underneath another.

> I understand thread-safe.  This usage is safe from Python threads 
> because it is all done in the context of the GIL.

No, it is not. See below.

> In your example, T2 doesn't hold the GIL and so, this is the scenario 
> that I believe you are invoking.

Assume that T2 is a Python thread. First, it doesn't hold the GIL.
However, when T1 releases the GIL to invoke the CRT, T2 can acquire
the GIL (which it had been waiting for, anyway), and then proceed
as described.

> Furthermore, since your argument assumes a rogue thread modifying the 
> CRT settings

No, it does not. Regular Python threads can break under this patch.

> Ooops, ignore my last comment.

(sorry, too late - I only read this after responding to the earlier one)

I created bug 5116 with a patch to expose CrtSetDebugMode via the msvcrt
module.  I didn't attach it to this patch as it doesn't address the
fundamental point in this bug, just offers a workaround to reinstate the
default if desired.  If there *was* agreement to change Python so only
the test suite disabled the CRT reports, then such a change could
probably now be implemented in .py code using the patch in bug 5116.

I think we ought to make sure that python doesn't crash, even if one 
passes a bogus fd to os.closerange().
My current patch catches all invalid arguments in software, except for 
the file descriptors sometimes allowed.
To that end, I suggest my current granular CrtSetDebugMode method, as 
currently implemented in the latest patch.

Martin has yet to comment on it.  Although he was right that a previous 
version was flawed, I think I've cracked it this time, WRT proper 
python threads.

I believe your new patch suffers the same problem.  Consider the blocks
like:

+	/* turn off crt asserts on windows since we have no control over fd */
+	Py_BEGIN_CRT_ERROR_HANDLING
 	Py_BEGIN_ALLOW_THREADS
 	size = write(fd, pbuf.buf, (size_t)pbuf.len);
 	Py_END_ALLOW_THREADS
+	Py_END_CRT_ERROR_HANDLING

And consider Martin's comments in msg80191: Here you have T1 setting a
global mode, then releasing the GIL.  Other threads are then free to run
and may do the same and depending on the order the threads do things,
the global setting may not end up where it started.

Ah, but I think you missed  the main point of the new patch.
There is a counter incremented and decremented.  So, the State is set 
only when the first thread enters a guarded section, and then restored 
when the final thread leaves the section.  There is no per-thread 
storage of the previous state anymore, only static variables storing 
the "unmodified state" to restore to when the last thread leaves a 
guarded section.  As long as some thread is in a guarded section, 
assertions and stuff is turned off, otherwise, we keep the old ones.

This is similar in some ways to the special version of 
Py_BEGIN_ALLOW_THREADS (FILE_BEGIN_ALLOW_THREADS, was it?) used in 
fileio.c.

The important function is _Py_SetCRTErrorHandler() in errors.c

Fair enough - but assertions are still disabled while your "reference
count" is >0, which is still while other arbitrary code is running. 
While I agree this is an improvement, I'm afraid I'm not playing close
enough attention to understand why your patch is better than, eg, simply
disabling such assertions globally during the test run. Given the patch
in issue5116, this could be implemented in far less lines of code -
certainly less intrusive lines.  Would this not achieve the same result?

FWIW, I'm also -1 on the proposed patch. While it does achieve what it
wants to achieve, I believe that the goal of the patch is undesirable.
It makes things worse, not better, by introducing the risk of race
conditions against other (native) threads - where currently, we would
get reproducable behavior.

Mark, I have two concerns:
1) Python shouldn't (IMHO) crahs, even if you give bogus input to 
python functions.  Making sure that it doesn't crash in the test suite 
is not enough, I think.
2) It shouldn't disable assertions and other runtime tests for other 
code just because of this, as I know you are in agreement with.

My patch achieves the former, and also the latter in most cases.  That 
is to say, yes, assertions will be disabled for the whole program 
during the time some thread is running one of the "fd" functions in the 
os module, but left alone otherwise.  For the most part, a typical 
windows application (where this issue exists) will not be using those 
functions much anyway.

If you want, we can split my patch in two:  The fixes for the strftime 
and fopen() (checkable by us) and the fixes for invalid file 
desctiptors.  The workaround for the latter seems more contentious.

Martin, your reproducable behaviour involves consistently disabling 
functionality that those threads you mention may rely on (and since 
this i is a global state that no one messes with, they are unlikely to 
be doing so)

If the alternative is then just to live with invalid file descriptors 
causing crashes, except for the test suite, as Mark suggests, I can 
also live with that, I suppose.  But then we would want the additional 
tests in my patch for strftime and fopen()

Btw, I am going to use our contact at MS to pressure them to expose a 
validation function for file descriptors to avoid this bloody mess.

Sorry for interruption. Maybe is _CrtSetReportHook useful?
http://msdn.microsoft.com/en-us/library/0yysf5e6(VS.80).aspx

1. Call _CrtSetReportHook on startup
2. Py_BEGIN_CRT_ERROR_HANDLING sets flag in thread local storage.
3. In hook function, look at above flag and change return value of hook
function.

This is just impression from MSDN document, I didn't try yet.

Of course, this won't work someone else altered hook function.

Interesting idea, but perhaps even more convoluted than my (admittedly) 
cumbersome patch.

fyi, I've added this issue with MS:
https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?
FeedbackID=409955

> Python shouldn't (IMHO) crahs, even if you give bogus input to
> python functions.  

But it doesn't actually crash does it?  It throws an assertion dialog,
which sucks when the machine is unattended - which is why it is a
problem for the buildbots - but otherwise if you hit 'ignore', things
keep working IIUC?

> Making sure that it doesn't crash in the test suite is 
> not enough, I think.

I think it is enough.  The tests are explicitly checking insane input to
these functions but "in the real world" such assertions blowing would
almost certainly be demonstrating a real bug.  pywin32 has a similar
issue - one or 2 tests cause such an assertion failure, but outside of
the test suite, such assertions should always be enabled IMO.  I'd even
advocate the assertions are disabled only for the individual tests known
to throw such errors and enabled for the rest.

Hirokazu Yamamoto's suggestion sounds reasonable too, but still doesn't
change my mind that it *is* reasonable to leave these assertions enabled
in the general case, and thus the idea isn't really necessary IMO.

Martin has the final say on what actually gets accepted though :)

Well, as long as you think that an assertion is fine for the fd 
functions, I am fine with that too.
I attach a limited patch to the fopen() and strftime() functions that 
avoids the assertions for those functions without messing with the 
runtime state.

Maybe already conclusion is met, so probably I shouldn't post this patch
but... here is the patch implementing msg80934. I believe this is thread
safe. (This patch doesn't include codes of fopen.patch, but it doesn't
mean they are not necessary. My patch is just a minimal patch for CRT hook)

I'm not sure which way is the best, but I'm happy if popup dialog on
test_os.py (test_fdopen) goes away. I'm startled every time. ;-)

Just impression. I feel Martin's "disable globally, and provide API to
turn it on" is simple and good. +1/2. Of course, my impression is biased
by the fact I have never used _ASSERTE even when debugging python.

> Martin, your reproducable behaviour involves consistently disabling 
> functionality that those threads you mention may rely on

Right. So with the current implementation, they notice immediately that
something is wrong.

> (and since 
> this i is a global state that no one messes with, they are unlikely to 
> be doing so)

Except that Python messes with it, so in the small window where the
assertions get disabled, they may run past them

> I'm not sure which way is the best

I think that is easy to answer: Python should validate all parameters,
and report ValueError if any of the parameters would otherwise be
rejected by the CRT. Then, we could leave assertions on, and would never
trigger them.

Ok.  I have submitted methods to verify the input to strftime() and 
fopen().  Only file descriptors seem to have no way to verify them.  
Mark Hammond seems to think this is ok (if tests can be selectively 
turned off), and if that is so, I have no objections.  My chief concern 
is to get the global CRT setting out of the way.

>Only file descriptors seem to have no way to verify them.

How about _get_osfhandle()? I cannot see _ASSERTE in that function on
VC6, but maybe there on VC9.

On VS8.0, _get_osfhandle() begins with three assert statements.
This function is not safer than the others...

fopen.patch is fine, please apply.

As for checking CRT handles: it seems that __pioinfo is exported from
msvcr90.dll. So we could check whether osfile has the FOPEN flag set,
and we could check whether fh >= 0. Unfortunately, there seems to be no
way to check for _nhandle. However, that might not be necessary: IIUC,
we just need to check whether M=fh>>IOINFO_L2E is smaller than
IOINFO_ARRAYS and __pioinfo[M] is not null.

FYI, Microsoft has responded to my query:
https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?
FeedbackID=409955

In short:
Comments
Hello,

Thanks for the report. We will probably not be able to address this in 
our current release, but we will reconsider for future versions of the 
CRT.

Pat Brenner
Visual C++ Libraries Development

Checked in r69268, to check strftime() and fopen()

Here is the patch implementing msg81093. (check_fd.patch)
I tested this on VC6. (test_os passed) I hope this also works on VC9 as
well.

I've taken the patch from Hirokazu and enhanced it:
1) it needed work to function with Visual Studio 2008
2) It now exposes a function so that _fileio.c can make use of it too.
3) Turned off the CRT manipulation in exceptions.c
4) Fixed minor problems, e.g. with dup2
5) Added comments

A VS2008 compilation runs the testsuite just fine. Maybe Hirozaku can 
test this with his VS2005?

Sorry, I don't have VS2005.

By the way, _PyVerify_fd seems to be required to VC6 too. Because
fdopen(fd >= _NHANDLE_) crashes on debug build and fdopen(bad fd <
_NHANDLE_) won't set errno to EBADF.

I see. I had thought your code was for VS2005 (VC8)
I have changed the patch to work with VS2005 as well.
I don't think we need to worry about VS2003 so much, as I don't think 
it is an officially supported compiler for the current python 
versions.  Also, the problem with close() et al only started when we 
ported the code to Visual Studio 2005, and this is the version of 
VisualStudio that starts to make assertions about its file descriptors.
Anyway, I upload an updated version of the patch, which copes with 
VS2005 as well as VS2008

I agree. Please focus on _MSC_VER >= 1400. I'll post new issue about VC6
after this issue will be solved.

I think the comment (an invalid fd would be a C program bug)
misrepresents the facts. Please don't check it in as-is. An invalid file
descriptor is *not* assertable. The authority on file descriptors, the
POSIX standard, specifies for, say, write(2)

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

This is consistent throughout the entire API: the behavior for invalid
file descriptors is *not* undefined, and not even
implementation-defined. If the CRT would conform to the relevant,
long-existing specifications it tries to emulate, it would just set
errno to EBADF, and be done.

There should be a global declaration of _Py_verify_fd. I would also
suggest that it becomes a constant macro unless _MSC_VER is defined.

Ok, how about this comment, Martin?

/* Microsoft CRT in VS2005 and higher will verify that a filehandle is
 * valid and throw 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."
 * Furthermore, python allows the user to enter any old integer
 * as a fd and should merely raise a python exception on error.
 * The Microsoft CRT doesn't provide an official way to check for the
 * validity of a file descriptor, but we can emulate its internal 
behaviour
 * by using the exported __pinfo data member and knowledge of the 
 * internal structures involved.
 * 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())
 */

Also, I've added the following to fileobject.h, not coming up with a 
better place:

#if defined _MSC_VER && _MSC_VER >= 1400
/* A routine to check if a file descriptor is valid on Windows.  
Returns 0
 * and sets errno to EBADF if it isn't.  This is to avoid Assertions
 * from various functions in the Windows CRT beginning with
 * Visual Studio 2005
 */
int _PyVerify_fd(int fd);
#else
#define _PyVerify_fd(A) (1) /* dummy */
#endif

The revised patch looks fine to me, please apply.

Committed r69495

I opened new issue related to VC6 as issue5204.

Why is this issue still open?

Closed as stated committed r69495.