Issue4804
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2009-01-02 04:09 by mhammond, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
crterror.patch | kristjan.jonsson, 2009-01-13 10:54 | |||
crterror.patch | kristjan.jonsson, 2009-01-19 15:03 | |||
crterror.patch | kristjan.jonsson, 2009-01-19 21:06 | a thread safe attempt at fine grained CRT error control | ||
fopen.patch | kristjan.jonsson, 2009-02-02 11:36 | |||
crt_report_hook.patch | ocean-city, 2009-02-02 17:01 | |||
check_fd.patch | ocean-city, 2009-02-04 20:46 | |||
__pioinfo.patch | kristjan.jonsson, 2009-02-06 10:45 | |||
__pioinfo.patch | kristjan.jonsson, 2009-02-06 15:57 |
Messages (62) | |||
---|---|---|---|
msg78753 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-02 04:09 | |
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? |
|||
msg78766 - (view) | Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * ![]() |
Date: 2009-01-02 09:17 | |
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" |
|||
msg78767 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-02 09:45 | |
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 |
|||
msg78768 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-02 09:56 | |
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 |
|||
msg78769 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-02 10:24 | |
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! |
|||
msg78770 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-02 10:30 | |
I guess another option is to expose it via msvcrt and let the test themselves disable it? |
|||
msg78780 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-02 11:24 | |
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 |
|||
msg78842 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-02 17:01 | |
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). |
|||
msg78848 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-02 17:31 | |
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 |
|||
msg78854 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-02 18:15 | |
> 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. |
|||
msg78892 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-02 22:21 | |
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? |
|||
msg78897 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-02 22:50 | |
> 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. |
|||
msg79731 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-13 10:54 | |
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) |
|||
msg79762 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-13 18:42 | |
> 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. |
|||
msg79782 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-13 22:33 | |
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. |
|||
msg79786 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-13 22:49 | |
> 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. |
|||
msg79803 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-13 23:38 | |
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. |
|||
msg80172 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-19 15:03 | |
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 |
|||
msg80191 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-19 16:30 | |
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. |
|||
msg80196 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-19 17:45 | |
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. |
|||
msg80197 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-19 17:50 | |
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. |
|||
msg80209 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-19 21:06 | |
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. |
|||
msg80214 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-19 22:14 | |
> 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. |
|||
msg80215 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-01-19 22:14 | |
> Ooops, ignore my last comment. (sorry, too late - I only read this after responding to the earlier one) |
|||
msg80859 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-31 06:55 | |
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. |
|||
msg80865 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-01-31 10:47 | |
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. |
|||
msg80890 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-01-31 23:39 | |
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. |
|||
msg80904 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-01 09:51 | |
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. |
|||
msg80905 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-01 09:55 | |
The important function is _Py_SetCRTErrorHandler() in errors.c |
|||
msg80906 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-02-01 11:01 | |
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? |
|||
msg80911 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-01 17:53 | |
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. |
|||
msg80931 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-02 09:20 | |
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. |
|||
msg80932 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-02 09:35 | |
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() |
|||
msg80933 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-02 09:37 | |
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. |
|||
msg80934 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-02 10:21 | |
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. |
|||
msg80935 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-02 10:26 | |
Of course, this won't work someone else altered hook function. |
|||
msg80936 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-02 10:30 | |
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 |
|||
msg80937 - (view) | Author: Mark Hammond (mhammond) * ![]() |
Date: 2009-02-02 11:16 | |
> 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 :) |
|||
msg80938 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-02 11:36 | |
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. |
|||
msg80956 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-02 17:01 | |
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. ;-) |
|||
msg80959 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-02 17:06 | |
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. |
|||
msg80998 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-02 21:26 | |
> 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 |
|||
msg81000 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-02 21:33 | |
> 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. |
|||
msg81040 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-03 10:33 | |
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. |
|||
msg81072 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-03 17:08 | |
>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. |
|||
msg81074 - (view) | Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * ![]() |
Date: 2009-02-03 17:17 | |
On VS8.0, _get_osfhandle() begins with three assert statements. This function is not safer than the others... |
|||
msg81079 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-03 18:28 | |
fopen.patch is fine, please apply. |
|||
msg81093 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-03 19:52 | |
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. |
|||
msg81133 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-04 09:34 | |
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 |
|||
msg81135 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-04 10:05 | |
Checked in r69268, to check strftime() and fopen() |
|||
msg81160 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-04 20:46 | |
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. |
|||
msg81258 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-06 10:45 | |
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? |
|||
msg81262 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-06 13:09 | |
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. |
|||
msg81271 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-06 15:57 | |
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 |
|||
msg81275 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-06 16:08 | |
I agree. Please focus on _MSC_VER >= 1400. I'll post new issue about VC6 after this issue will be solved. |
|||
msg81321 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-06 23:56 | |
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. |
|||
msg81459 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-09 15:17 | |
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 |
|||
msg81535 - (view) | Author: Martin v. Löwis (loewis) * ![]() |
Date: 2009-02-10 09:25 | |
The revised patch looks fine to me, please apply. |
|||
msg81549 - (view) | Author: Kristján Valur Jónsson (kristjan.jonsson) * ![]() |
Date: 2009-02-10 13:33 | |
Committed r69495 |
|||
msg81552 - (view) | Author: Hirokazu Yamamoto (ocean-city) * ![]() |
Date: 2009-02-10 14:09 | |
I opened new issue related to VC6 as issue5204. |
|||
msg108689 - (view) | Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * ![]() |
Date: 2010-06-26 07:59 | |
Why is this issue still open? |
|||
msg110838 - (view) | Author: Mark Lawrence (BreamoreBoy) * | Date: 2010-07-19 23:20 | |
Closed as stated committed r69495. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:43 | admin | set | github: 49054 |
2010-07-19 23:20:40 | BreamoreBoy | set | status: open -> closed nosy: + BreamoreBoy messages: + msg110838 resolution: fixed |
2010-06-26 07:59:29 | amaury.forgeotdarc | set | messages: + msg108689 |
2010-06-26 00:31:32 | terry.reedy | set | versions: + Python 3.2, - Python 2.5, Python 3.0 |
2009-02-10 14:09:52 | ocean-city | set | messages: + msg81552 |
2009-02-10 13:33:26 | kristjan.jonsson | set | messages: + msg81549 |
2009-02-10 09:25:35 | loewis | set | messages: + msg81535 |
2009-02-09 15:17:27 | kristjan.jonsson | set | messages: + msg81459 |
2009-02-06 23:56:53 | loewis | set | messages: + msg81321 |
2009-02-06 16:08:52 | ocean-city | set | messages: + msg81275 |
2009-02-06 15:57:27 | kristjan.jonsson | set | files:
+ __pioinfo.patch messages: + msg81271 |
2009-02-06 13:09:25 | ocean-city | set | messages: + msg81262 |
2009-02-06 10:45:53 | kristjan.jonsson | set | files:
+ __pioinfo.patch messages: + msg81258 |
2009-02-04 20:46:15 | ocean-city | set | files:
+ check_fd.patch messages: + msg81160 |
2009-02-04 10:06:00 | kristjan.jonsson | set | messages: + msg81135 |
2009-02-04 09:34:09 | kristjan.jonsson | set | messages: + msg81133 |
2009-02-03 19:52:18 | loewis | set | messages: + msg81093 |
2009-02-03 18:28:03 | loewis | set | messages: + msg81079 |
2009-02-03 17:17:35 | amaury.forgeotdarc | set | messages: + msg81074 |
2009-02-03 17:08:23 | ocean-city | set | messages: + msg81072 |
2009-02-03 17:06:13 | ocean-city | set | messages: - msg81054 |
2009-02-03 16:31:51 | ocean-city | set | messages: - msg81055 |
2009-02-03 13:41:53 | ocean-city | set | messages: + msg81055 |
2009-02-03 13:39:25 | ocean-city | set | messages: + msg81054 |
2009-02-03 10:33:11 | kristjan.jonsson | set | messages: + msg81040 |
2009-02-02 21:33:30 | loewis | set | messages: + msg81000 |
2009-02-02 21:26:02 | loewis | set | messages: + msg80998 |
2009-02-02 17:06:35 | ocean-city | set | messages: + msg80959 |
2009-02-02 17:02:00 | ocean-city | set | files:
+ crt_report_hook.patch messages: + msg80956 |
2009-02-02 11:36:21 | kristjan.jonsson | set | files:
+ fopen.patch messages: + msg80938 |
2009-02-02 11:16:52 | mhammond | set | messages: + msg80937 |
2009-02-02 10:30:57 | kristjan.jonsson | set | messages: + msg80936 |
2009-02-02 10:26:27 | ocean-city | set | messages: + msg80935 |
2009-02-02 10:21:36 | ocean-city | set | nosy:
+ ocean-city messages: + msg80934 |
2009-02-02 09:37:17 | kristjan.jonsson | set | messages: + msg80933 |
2009-02-02 09:35:06 | kristjan.jonsson | set | messages: + msg80932 |
2009-02-02 09:20:09 | kristjan.jonsson | set | messages: + msg80931 |
2009-02-01 17:53:21 | loewis | set | messages: + msg80911 |
2009-02-01 11:01:08 | mhammond | set | messages: + msg80906 |
2009-02-01 09:55:06 | kristjan.jonsson | set | messages: + msg80905 |
2009-02-01 09:51:23 | kristjan.jonsson | set | messages: + msg80904 |
2009-01-31 23:39:11 | mhammond | set | messages: + msg80890 |
2009-01-31 10:47:50 | kristjan.jonsson | set | messages: + msg80865 |
2009-01-31 06:55:58 | mhammond | set | messages: + msg80859 |
2009-01-19 22:14:58 | loewis | set | messages: + msg80215 |
2009-01-19 22:14:20 | loewis | set | messages: + msg80214 |
2009-01-19 21:06:44 | kristjan.jonsson | set | files:
+ crterror.patch messages: + msg80209 |
2009-01-19 17:50:54 | kristjan.jonsson | set | messages: + msg80197 |
2009-01-19 17:45:20 | kristjan.jonsson | set | messages: + msg80196 |
2009-01-19 16:30:45 | loewis | set | messages: + msg80191 |
2009-01-19 15:03:45 | kristjan.jonsson | set | files:
+ crterror.patch messages: + msg80172 |
2009-01-13 23:39:00 | kristjan.jonsson | set | messages: + msg79803 |
2009-01-13 22:49:38 | loewis | set | messages: + msg79786 |
2009-01-13 22:33:42 | mhammond | set | messages: + msg79782 |
2009-01-13 18:42:56 | loewis | set | messages: + msg79762 |
2009-01-13 10:54:19 | kristjan.jonsson | set | files:
+ crterror.patch keywords: + patch, needs review messages: + msg79731 stage: patch review |
2009-01-02 22:50:03 | loewis | set | messages: + msg78897 |
2009-01-02 22:21:28 | mhammond | set | messages: + msg78892 |
2009-01-02 18:15:21 | loewis | set | messages: + msg78854 |
2009-01-02 17:31:16 | kristjan.jonsson | set | messages: + msg78848 |
2009-01-02 17:01:14 | loewis | set | nosy:
+ loewis messages: + msg78842 |
2009-01-02 11:24:54 | kristjan.jonsson | set | messages: + msg78780 |
2009-01-02 10:30:17 | mhammond | set | messages: + msg78770 |
2009-01-02 10:24:34 | mhammond | set | messages: + msg78769 |
2009-01-02 09:56:19 | kristjan.jonsson | set | messages: + msg78768 |
2009-01-02 09:45:17 | kristjan.jonsson | set | messages: + msg78767 |
2009-01-02 09:17:10 | amaury.forgeotdarc | set | nosy:
+ amaury.forgeotdarc messages: + msg78766 |
2009-01-02 04:09:39 | mhammond | create |