Mark Lawrence breamore...@yahoo.co.uk added the comment:
Closed as stated committed r69495.
--
nosy: +BreamoreBoy
resolution: - fixed
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
Amaury Forgeot d'Arc amaur...@gmail.com added the comment:
Why is this issue still open?
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Changes by Terry J. Reedy tjre...@udel.edu:
--
versions: +Python 3.2 -Python 2.5, Python 3.0
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Martin v. Löwis mar...@v.loewis.de added the comment:
The revised patch looks fine to me, please apply.
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Committed r69495
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
___
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp added the comment:
I opened new issue related to VC6 as issue5204.
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Kristján Valur Jónsson krist...@ccpgames.com 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
*
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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.
Kristján Valur Jónsson krist...@ccpgames.com 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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 rep...@bugs.python.org
http://bugs.python.org/issue4804
Kristján Valur Jónsson krist...@ccpgames.com 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)
Martin v. Löwis mar...@v.loewis.de 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,
Kristján Valur Jónsson krist...@ccpgames.com 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
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
Checked in r69268, to check strftime() and fopen()
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp added the comment:
Hmm, maybe _lock_fhandle can be used to solve such race condition. I'm
not sure.
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Changes by Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp:
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
___
Python-bugs-list mailing list
Changes by Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp:
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
___
Python-bugs-list mailing list
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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
Amaury Forgeot d'Arc amaur...@gmail.com added the comment:
On VS8.0, _get_osfhandle() begins with three assert statements.
This function is not safer than the others...
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
Kristján Valur Jónsson krist...@ccpgames.com 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
Martin v. Löwis mar...@v.loewis.de added the comment:
fopen.patch is fine, please apply.
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
___
Martin v. Löwis mar...@v.loewis.de 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.
Kristján Valur Jónsson krist...@ccpgames.com 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
Kristján Valur Jónsson krist...@ccpgames.com 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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.
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp added the comment:
Of course, this won't work someone else altered hook function.
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Martin v. Löwis mar...@v.loewis.de 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
Martin v. Löwis mar...@v.loewis.de 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
Kristján Valur Jónsson krist...@ccpgames.com 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 rep...@bugs.python.org
Kristján Valur Jónsson krist...@ccpgames.com 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
Hirokazu Yamamoto ocean-c...@m2.ccsnet.ne.jp 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
Mark Hammond mhamm...@users.sourceforge.net 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
Kristján Valur Jónsson krist...@ccpgames.com 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
Kristján Valur Jónsson krist...@ccpgames.com added the comment:
The important function is _Py_SetCRTErrorHandler() in errors.c
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
___
Mark Hammond mhamm...@users.sourceforge.net 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
Martin v. Löwis mar...@v.loewis.de 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)
Kristján Valur Jónsson krist...@ccpgames.com 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,
Mark Hammond mhamm...@users.sourceforge.net 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
Mark Hammond mhamm...@users.sourceforge.net 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.
Kristján Valur Jónsson krist...@ccpgames.com 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
Martin v. Löwis mar...@v.loewis.de 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
Kristján Valur Jónsson krist...@ccpgames.com 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
Kristján Valur Jónsson krist...@ccpgames.com 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
Martin v. Löwis mar...@v.loewis.de 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
Martin v. Löwis mar...@v.loewis.de added the comment:
Ooops, ignore my last comment.
(sorry, too late - I only read this after responding to the earlier one)
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
Kristján Valur Jónsson krist...@ccpgames.com 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
Martin v. Löwis mar...@v.loewis.de 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
Mark Hammond mhamm...@users.sourceforge.net 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
Amaury Forgeot d'Arc amaur...@gmail.com 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
Kristján Valur Jónsson krist...@ccpgames.com 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.
Kristján Valur Jónsson krist...@ccpgames.com 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
Mark Hammond mhamm...@users.sourceforge.net 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.
Mark Hammond mhamm...@users.sourceforge.net added the comment:
I guess another option is to expose it via msvcrt and let the test
themselves disable it?
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue4804
Kristján Valur Jónsson krist...@ccpgames.com 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
Martin v. Löwis mar...@v.loewis.de 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
Kristján Valur Jónsson krist...@ccpgames.com 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,
Martin v. Löwis mar...@v.loewis.de 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
Mark Hammond mhamm...@users.sourceforge.net 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
Martin v. Löwis mar...@v.loewis.de 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
New submission from Mark Hammond mhamm...@users.sourceforge.net:
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
63 matches
Mail list logo
