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 bpo-1350409 (and subsequently bpo-1167262 and bpo-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.

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

Why is this issue still open?

Closed as stated committed r69495.