[issue4804] Python on Windows disables all C runtime library assertions

Mark Lawrence added the comment: Closed as stated committed r69495. -- nosy: +BreamoreBoy resolution: -> fixed status: open -> closed ___ Python tracker ___

[issue4804] Python on Windows disables all C runtime library assertions

Amaury Forgeot d'Arc added the comment: Why is this issue still open? -- ___ Python tracker ___ ___ Python-bugs-list mailing list Unsu

[issue4804] Python on Windows disables all C runtime library assertions

Changes by Terry J. Reedy : -- versions: +Python 3.2 -Python 2.5, Python 3.0 ___ Python tracker ___ ___ Python-bugs-list mailing list U

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: I opened new issue related to VC6 as issue5204. ___ Python tracker ___ ___ Python-bugs-list mailing list U

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: Committed r69495 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.p

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: The revised patch looks fine to me, please apply. ___ Python tracker ___ ___ Python-bugs-list mailing list U

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 us

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: 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) [...] Othe

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: I agree. Please focus on _MSC_VER >= 1400. I'll post new issue about VC6 after this issue will be solved. ___ Python tracker ___ _

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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. Als

[issue4804] Python on Windows disables all C runtime library assertions

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

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: 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. Added file: http://bugs.python.org/file12942/check_fd.patch ___ Python tracker

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: Checked in r69268, to check strftime() and fopen() ___ Python tracker ___ ___ Python-bugs-list mailin

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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, bu

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: 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 no

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: fopen.patch is fine, please apply. ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: ht

[issue4804] Python on Windows disables all C runtime library assertions

Amaury Forgeot d'Arc added the comment: On VS8.0, _get_osfhandle() begins with three assert statements. This function is not safer than the others... ___ Python tracker ___ __

[issue4804] Python on Windows disables all C runtime library assertions

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

[issue4804] Python on Windows disables all C runtime library assertions

Changes by Hirokazu Yamamoto : ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/pyt

[issue4804] Python on Windows disables all C runtime library assertions

Changes by Hirokazu Yamamoto : ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/pyt

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: Hmm, maybe _lock_fhandle can be used to solve such race condition. I'm not sure. ___ Python tracker ___ __

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: >Only file descriptors seem to have no way to verify them. How about _get_osfhandle()? (Strictly, there is race condition between _get_osfhandle and fdopen etc though) ___ Python tracker

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 obje

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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.

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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 st

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: 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. ___ Python

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: 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 j

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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.

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: > 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 otherwi

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: Of course, this won't work someone else altered hook function. ___ Python tracker ___ ___ Python-bugs-list

[issue4804] Python on Windows disables all C runtime library assertions

Hirokazu Yamamoto added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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. ___ Python tracker ___

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 jus

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 o

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: 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 cu

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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 bette

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: The important function is _Py_SetCRTErrorHandler() in errors.c ___ Python tracker ___ ___ Python-bugs

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 i

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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, (siz

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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 c

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > Ooops, ignore my last comment. (sorry, too late - I only read this after responding to the earlier one) ___ Python tracker ___ __

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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. Ass

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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. ___ Python tracker

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 t

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: 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 simultan

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 Added file: http://bugs.python.org/file12796/crterror.patch _

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 be

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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 its

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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 t

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 wan

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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 spec

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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 responsi

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: > 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 numb

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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

[issue4804] Python on Windows disables all C runtime library assertions

Martin v. Löwis added the comment: 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). -- nosy

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 ___ Python tracker

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: I guess another option is to expose it via msvcrt and let the test themselves disable it? ___ Python tracker ___ __

[issue4804] Python on Windows disables all C runtime library assertions

Mark Hammond added the comment: 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 ju

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 th

[issue4804] Python on Windows disables all C runtime library assertions

Kristján Valur Jónsson added the comment: 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 asserti

[issue4804] Python on Windows disables all C runtime library assertions

Amaury Forgeot d'Arc added the comment: 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" -- nosy: +amaury.forgeotdarc ___

[issue4804] Python on Windows disables all C runtime library assertions

New submission from Mark Hammond : 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 -