| msg71854 - (view) |
Author: Daniel Diniz (ajaksu2) *  |
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 "", 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) *  |
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) * |
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) * |
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) * |
Date: 2008-08-24 21:22 |
| It won't be resurrected for long if we write a test. :) |
|
|
| msg71892 - (view) |
Author: Neal Norwitz (nnorwitz) * |
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) |
|
|