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.

classification
Title: insufficient error checking causes crash on windows
Type: crash Stage: resolved
Components: IO Versions: Python 2.7
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: maxdeliso, ncoghlan, vstinner
Priority: normal Keywords: patch

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) * (Python committer) 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) * (Python committer) 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:46adminsetgithub: 62397
2013-06-16 12:02:39ncoghlansetstatus: open -> closed

nosy: + ncoghlan
messages: + msg191265

resolution: not a bug
stage: resolved
2013-06-16 05:25:58maxdelisosetmessages: + msg191262
2013-06-13 23:47:50vstinnersetnosy: + vstinner
messages: + msg191098
2013-06-12 06:26:24maxdelisosethgrepos: - hgrepo199
2013-06-12 06:25:59maxdelisocreate