Issue18197
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 2013-06-12 06:25 by maxdeliso, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
fileobject_fix.patch | maxdeliso, 2013-06-12 06:25 | patch | review |
Messages (4) | |||
---|---|---|---|
msg191012 - (view) | Author: Max DeLiso (maxdeliso) | Date: 2013-06-12 06:25 | |
hi. if you cross compile the mercurial native extensions against python 2.7.5 (x64) on 64 bit windows 7 and then try to clone something, it will crash. I believe the reason for this is that the c runtime functions in the microsoft crt will throw a win32 exception if they are given invalid parameters, and since the return value of fileno() is not checked in Objects/fileobject.c, if a file handle is passed to fileno and the result is not a valid file descriptor, that invalid decriptor will get passed to _fstat64i32, an invalid parameter exception will be raised, and the program will crash. here's the function with the alleged bug: static PyFileObject* dircheck(PyFileObject* f) { #if defined(HAVE_FSTAT) && defined(S_IFDIR) && defined(EISDIR) struct stat buf; if (f->f_fp == NULL) return f; if (fstat(fileno(f->f_fp), &buf) == 0 && // this line is the problem, fileno's return value never gets checked S_ISDIR(buf.st_mode)) { char *msg = strerror(EISDIR); PyObject *exc = PyObject_CallFunction(PyExc_IOError, "(isO)", EISDIR, msg, f->f_name); PyErr_SetObject(PyExc_IOError, exc); Py_XDECREF(exc); return NULL; } #endif return f; } here's the stack trace: > msvcr90.dll!_invalid_parameter() Unknown msvcr90.dll!_fstat64i32() Unknown python27.dll!dircheck(PyFileObject * f) Line 127 C python27.dll!fill_file_fields(PyFileObject * f, _iobuf * fp, _object * name, char * mode, int (_iobuf *) * close) Line 183 C python27.dll!PyFile_FromFile(_iobuf * fp, char * name, char * mode, int (_iobuf *) * close) Line 484 C here's a dump summary: Dump Summary ------------ Process Name: python.exe : c:\Python27\python.exe Process Architecture: x64 Exception Code: 0xC0000417 Exception Information: Heap Information: Present about the patch: the attached patch fixes that behavior and doesn't break any test cases on windows or linux. it applies against the current trunk of cpython. the return value of fileno should get checked for correctness anyways, even on *nix. the extra overhead is tiny, (one comparison and a conditional jump and a few extra bytes of stack space), but you do catch some weird edge cases. here are the steps to reproduce: download the python 2.7.5 installer for windows download the mercurial 2.6.2 source release build the native extensions with 64 bit microsoft compilers try to hg clone any remote repo (it should crash) here are some version strings: Python 2.7.5 (default, May 15 2013, 22:44:16) [MSC v.1500 64 bit (AMD64)] on win32 Microsoft (R) C/C++ Optimizing Compiler Version 17.00.60315.1 for x64 mercurial 2.6.2 here are some links: in particular, read the bits about the invalid parameter exception: _fsta64i32: http://msdn.microsoft.com/en-US/library/221w8e43%28v=vs.80%29.aspx _fileno: http://msdn.microsoft.com/en-US/library/zs6wbdhx%28v=vs.80%29.aspx Please let me know if my patch needs work or if I missed something. Thanks! |
|||
msg191098 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-06-13 23:47 | |
Can you explain why fileno() does fail? Do you have an idea of how many open file descriptor do you have? |
|||
msg191262 - (view) | Author: Max DeLiso (maxdeliso) | Date: 2013-06-16 05:25 | |
ok I checked in to this more deeply and I was wrong about a few things. first, my patch is worthless - there are several more instances where the retval of fileno is passed directly to fstat and that is totally valid (provided the file* points to a valid file). looking deeper in the call stack, this call stack is originating from a PyCFunction_Call from mercurial's native extension, osutil. # Call Site 00 MSVCR90!fstat64i32+0xe8 01 python27!dircheck+0x29 02 python27!fill_file_fields+0x18e 03 python27!PyFile_FromFile+0x89 04 osutil+0x176f 05 python27!PyCFunction_Call+0x76 Here's the code in osutil.c (which is part of mercurial) (osutil.c:554) #ifndef IS_PY3K fp = _fdopen(fd, fpmode); if (fp == NULL) { _close(fd); PyErr_SetFromErrnoWithFilename(PyExc_IOError, name); goto bail; } file_obj = PyFile_FromFile(fp, name, mode, fclose); //this is the call that is the parent if (file_obj == NULL) { fclose(fp); goto bail; } PyFile_SetBufSize(file_obj, bufsize); #else fileno() is actually 'succeeding' and returning a value of 3. fstat is then throwing the invalid parameter exception, presumably because 3 is not a valid file descriptor. the way fileno() is implemented in M$CRT is really simple: it just copies a value at a fixed offset from the pointer passed to it without checking to see if the FILE* is valid. this is why in the docs for _fileno they say "The result is undefined if stream does not specify an open file." anyways, I don't think this is a bug in python, but rather in the mercurial extension. it's a little tricky to debug on windows because the osutil module gets delay-loaded. I'm taking another pass at it now. |
|||
msg191265 - (view) | Author: Nick Coghlan (ncoghlan) * | Date: 2013-06-16 12:02 | |
Closing this on the assumption the bug is in the extension. Feel free to reopen if further investigation shows a problem in the interpreter core. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:46 | admin | set | github: 62397 |
2013-06-16 12:02:39 | ncoghlan | set | status: open -> closed nosy: + ncoghlan messages: + msg191265 resolution: not a bug stage: resolved |
2013-06-16 05:25:58 | maxdeliso | set | messages: + msg191262 |
2013-06-13 23:47:50 | vstinner | set | nosy:
+ vstinner messages: + msg191098 |
2013-06-12 06:26:24 | maxdeliso | set | hgrepos: - hgrepo199 |
2013-06-12 06:25:59 | maxdeliso | create |