classification
Title: _fileio._FileIO segfaults
Type: crash Stage:
Components: Interpreter Core Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: nnorwitz Nosy List: ajaksu2, amaury.forgeotdarc, benjamin.peterson, christian.heimes, nnorwitz
Priority: release blocker Keywords: needs review

Created on 2008-08-24 19:37 by ajaksu2, last changed 2008-08-24 22:08 by nnorwitz. This issue is now closed.

Messages (6)
msg71854 - (view) Author: Daniel Diniz (ajaksu2) (Python triager) Date: 2008-08-24 19:37
This snippet causes a segfault from fileio_init calling PyMem_Free:
    
import _fileio; _fileio._FileIO("1",0, 0 )

Found using Fusil

[Switching to Thread -1210070848 (LWP 10184)]
0x0805f5ff in _PyObject_DebugCheckAddress (p=0xb7b2f0e8) at
Objects/obmalloc.c:1461
1461                    if (tail[i] != FORBIDDENBYTE) {
(gdb) backtrace
0  0x0805f5ff in _PyObject_DebugCheckAddress (p=0xb7b2f0e8) at
Objects/obmalloc.c:1461
1  0x0805f3c4 in _PyObject_DebugFree (p=0xb7b2f0e8) at
Objects/obmalloc.c:1375
2  0x0805de07 in PyMem_Free (p=0xb7b2f0e8) at Objects/object.c:1693
3  0x0810afa9 in fileio_init (oself=0xb7b18238, args=0xb7b815b4,
kwds=0x0) at ./Modules/_fileio.c:281
4  0x0806bdd0 in type_call (type=0x81d3760, args=0xb7b815b4, kwds=0x0)
at Objects/typeobject.c:650
5  0x08118ec5 in PyObject_Call (func=0x81d3760, arg=0xb7b815b4, kw=0x0)
at Objects/abstract.c:2181
6  0x080b42a5 in do_call (func=0x81d3760, pp_stack=0xbfab5c84, na=3,
nk=0) at Python/ceval.c:3616
7  0x080b394a in call_function (pp_stack=0xbfab5c84, oparg=3) at
Python/ceval.c:3426
8  0x080ae8d2 in PyEval_EvalFrameEx (f=0x829bb14, throwflag=0) at
Python/ceval.c:2205
9  0x080b1c24 in PyEval_EvalCodeEx (co=0xb7b5b9e8, globals=0xb7fcc5d4,
locals=0xb7fcc5d4, args=0x0, argcount=0, kws=0x0,
    kwcount=0, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0) at
Python/ceval.c:2840
10 0x080a69cb in PyEval_EvalCode (co=0xb7b5b9e8, globals=0xb7fcc5d4,
locals=0xb7fcc5d4) at Python/ceval.c:519
11 0x080df64b in run_mod (mod=0x82a2ac0, filename=0x819e3be "<string>",
globals=0xb7fcc5d4, locals=0xb7fcc5d4,
    flags=0xbfab6060, arena=0x82b1060) at Python/pythonrun.c:1560
12 0x080df393 in PyRun_StringFlags (str=0x8203fd8 "import _fileio;
_fileio._FileIO('1',0, 0 )\n", start=257,
    globals=0xb7fcc5d4, locals=0xb7fcc5d4, flags=0xbfab6060) at
Python/pythonrun.c:1494
13 0x080ddd37 in PyRun_SimpleStringFlags (command=0x8203fd8 "import
_fileio; _fileio._FileIO('1',0, 0 )\n",
    flags=0xbfab6060) at Python/pythonrun.c:1073
14 0x080ef5ca in Py_Main (argc=2, argv=0xb7f9e028) at Modules/main.c:533
15 0x0805a689 in main (argc=2, argv=0xbfab71b4) at ./Modules/python.c:57
msg71858 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-08-24 20:06
The FileIO construct segfaults because PyArg_ParseTupleAndKeywords()
sets name to an invalid but non NULL value. PyMem_Free() tries to
deallocate name but fails.

Suggestion: Either appy this patch or change
PyArg_ParseTupleAndKeyword()'s behavior for 'e'.

Index: Modules/_fileio.c
===================================================================
--- Modules/_fileio.c   (Revision 66010)
+++ Modules/_fileio.c   (Arbeitskopie)
@@ -174,8 +174,10 @@
                if (!PyArg_ParseTupleAndKeywords(args, kwds, "et|si:fileio",
                                                 kwlist,
                                                
Py_FileSystemDefaultEncoding,
-                                                &name, &mode, &closefd))
+                                                &name, &mode, &closefd)) {
+                       name = NULL;
                        goto error;
+                       }
            }
        }
msg71874 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-24 21:16
The "goto error" is not necessary here. Nothing has been allocated at
this point, and "return -1" is enough.

Index: Modules/_fileio.c
===================================================================
--- Modules/_fileio.c   (revision 65957)
+++ Modules/_fileio.c   (working copy)
@@ -175,7 +175,7 @@
                                                 kwlist,
                                                
Py_FileSystemDefaultEncoding,
                                                 &name, &mode, &closefd))
-                       goto error;
+                       return -1;
            }
        }
msg71876 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-08-24 21:20
You are right. But I'd rather keep the name = NULL assignment or add a
comment. In the future somebody may alter the function and resurrect the
bug accidentally.
msg71877 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-24 21:22
It won't be resurrected for long if we write a test. :)
msg71892 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-08-24 22:08
Daniel, thanks for running the fuzzer!  It would be great if you could
keep running it and find any more problems before releasing 2.6 and 3.0.

I agree with Benjamin and Amaury.  PyArg_ParseTupleAndKeywords()
shouldn't update the pointer if it failed.  Given the test, it's
unlikely for this to create a problem in the future.  Either as a crash
or as a memory leak.

Committed revision 66018. (2.6)
Committed revision 66019. (3.0)
History
Date User Action Args
2008-08-24 22:08:24nnorwitzsetstatus: open -> closed
nosy: + nnorwitz
messages: + msg71892
assignee: nnorwitz
components: + Interpreter Core
resolution: fixed
2008-08-24 21:22:19benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg71877
2008-08-24 21:20:09christian.heimessetmessages: + msg71876
2008-08-24 21:16:27amaury.forgeotdarcsetkeywords: + needs review
nosy: + amaury.forgeotdarc
messages: + msg71874
2008-08-24 20:07:08christian.heimessetpriority: release blocker
2008-08-24 20:06:58christian.heimessetnosy: + christian.heimes
messages: + msg71858
2008-08-24 19:44:50benjamin.petersonsetversions: + Python 2.6
2008-08-24 19:37:10ajaksu2create