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.

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).

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]

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.

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.

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().

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.

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.

How's this coming along?

Fixed in r65745. Will be backported to py3k and release25-maint.

Backported to py3k(r65746) release25-maint(r65747)