| msg63211 - (view) |
Author: Hirokazu Yamamoto (ocean-city) *  |
Date: 2008-03-03 13:05 |
| Hello. Probably I found memory leak. When first convert_to_unicode succeeds and second one fails, first unicode object is not freed. # ex: os.rename("a", 3) Thank you. |
|
|
| msg63214 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-03-03 15:16 |
| It looks like non-windows code has a similar problem: static PyObject * posix_2str(PyObject *args, char *format, int (*func)(const char *, const char *)) { char *path1 = NULL, *path2 = NULL; int res; if (!PyArg_ParseTuple(args, format, Py_FileSystemDefaultEncoding, &path1, Py_FileSystemDefaultEncoding, &path2)) return NULL; If decoding of path2 fails, path1 is never freed. On the patch itself, arguably Py_XDECREF(o2) is not necessary, but leaving it in is probably good defensive programming (e.g. if more args are added in the future.) I am +1 on the patch as is. Please add a unit test that exercises the new code. Check that the leak is detected when the unit test is ran with gc.set_debug(gc.DEBUG_LEAK). |
|
|
| msg63217 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2008-03-03 17:21 |
| I cannot write patch to use gc.set_debug(gc.DEBUG_LEAK), so I tried regrtest.py -R :: instead. (This functionality is not working now, so I tried after reverted r61098) E:\python-dev\trunk\Lib\test>py regrtest.py -R :: test_os.py test_os beginning 9 repetitions 123456789 ......... test_os leaked [1, 1, 1, 1] references, sum=4 1 test OK. [38282 refs] |
|
|
| msg63251 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-03-04 12:54 |
| The affected code is the only case I could find in stdlib where O& format was used to populate PyObject* variables. Although it appears to be valid usage, the code presents an exception to the following note at http://docs.python.org/dev/c-api/arg.html : "Note that any Python object references which are provided to the caller are borrowed references; do not decrement their reference count!" Should we add that O& a possible exception to this rule? I'll propose a specific change if we agree in principle. I am not sure if O& documentation should make any recommendations to the writers of conversion functions. For example, O& convertors returning a borrowed reference may be discouraged in favor of O or O& variants or returning PyObject* from a convertor may be discouraged altogether. I am adding Georg who accepted my other documentation changes in this area to the "nosy" list. |
|
|
| msg63255 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-03-04 17:14 |
| On the test_os patch: I would rename LeakTest to ErrorTest and leave it on for all platforms. BTW, I cannot produce a definitive proof that posix_2str leaks when second conversion fails (regrtest -R does not catch it because the leaked buffer is not a PyObject). Nevertheless, it would be a good idea to add tests that exercise os.rename("foo", 0), os.link("foo", 0) and os.symlink("foo", 0) errors on all platforms. |
|
|
| msg63348 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2008-03-07 10:16 |
| Alexander, I've looked into Python/getargs.c, I think posix_2str code is fine. (PyArg_ParseTuple with format "et") After conversion succeeded on path1, addcleanup() adds memory buffer for path1 into freelist. When error happend on path2, its memory will be freed on cleanreturn(). |
|
|
| msg63357 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2008-03-07 15:55 |
| Aha! I did not know about the cleanup. Maybe that should be documented as well. This shows that using O& with a converter returning a PyObject* is a bad idea. (General clean-up is not possible for O& because there is no way to tell what type the converter returns.) I am still +1 on your original patch. It is obviously correct and fixes the bug with minimum of changes. However, win32 code in posixmodule.c needs some clean-up. convert_to_unicode should be eliminated in favor of the approach used in win32_1str. It may also make sense to move conversion/api selection code to a win32_2str function. Even if it will be only used for rename, it will make the code more readable by making win32 code structure similar to posix. All this is a topic for another issue. I believe the only thing that needs to be done here is to enable error case testing for all platforms in the unit test. |
|
|
| msg65695 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2008-04-23 03:25 |
| I reimplemented patch without O&, and made test for all platforms. Unfortunately Windows doesn't have os.link and os.symlink, so for os.rename only. |
|
|
| msg70362 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-07-28 17:04 |
| How's this coming along? |
|
|
| msg71263 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2008-08-17 09:32 |
| Fixed in r65745. Will be backported to py3k and release25-maint. |
|
|
| msg71278 - (view) |
Author: Hirokazu Yamamoto (ocean-city) * |
Date: 2008-08-17 15:09 |
| Backported to py3k(r65746) release25-maint(r65747) |
|
|