[New-bugs-announce] [issue18197] insufficient error checking causes crash on windows

[Max DeLiso]

New submission from Max DeLiso:

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!

----------
components: IO
files: fileobject_fix.patch
hgrepos: 199
keywords: patch
messages: 191012
nosy: maxdeliso
priority: normal
severity: normal
status: open
title: insufficient error checking causes crash on windows
type: crash
versions: Python 2.7
Added file: http://bugs.python.org/file30552/fileobject_fix.patch

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue18197>
_______________________________________