insufficient error checking causes crash on windows

[maxdeliso]

BPO	18197
Nosy	@ncoghlan, @vstinner
Files	fileobject_fix.patch: patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-06-16.12:02:39.295>
created_at = <Date 2013-06-12.06:25:59.551>
labels = ['invalid', 'expert-IO', 'type-crash']
title = 'insufficient error checking causes crash on windows'
updated_at = <Date 2013-06-16.12:02:39.293>
user = 'https://bugs.python.org/maxdeliso'

bugs.python.org fields:

activity = <Date 2013-06-16.12:02:39.293>
actor = 'ncoghlan'
assignee = 'none'
closed = True
closed_date = <Date 2013-06-16.12:02:39.295>
closer = 'ncoghlan'
components = ['IO']
creation = <Date 2013-06-12.06:25:59.551>
creator = 'maxdeliso'
dependencies = []
files = ['30552']
hgrepos = []
issue_num = 18197
keywords = ['patch']
message_count = 4.0
messages = ['191012', '191098', '191262', '191265']
nosy_count = 3.0
nosy_names = ['ncoghlan', 'vstinner', 'maxdeliso']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue18197'
versions = ['Python 2.7']

[maxdeliso]

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!

[vstinner]

Can you explain why fileno() does fail?

Do you have an idea of how many open file descriptor do you have?

[maxdeliso]

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.

[ncoghlan]

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.