It looks like the bytes variable should be Py_DECREFed on error cases as the patch shows.

Oh crap, memory leaks on Windows are rarely checked. I recall that I found a huge memory leak on Windows just before Python 3.6 beta 1 release:

changeset:   103956:6232e610e310
branch:      3.6
parent:      103954:c1d9052996f1
user:        Victor Stinner <victor.stinner@gmail.com>
date:        Mon Sep 19 11:55:44 2016 +0200
files:       Misc/NEWS Modules/posixmodule.c
description:
Fix memory leak in path_converter()

Issue #28200: Replace PyUnicode_AsWideCharString() with
PyUnicode_AsUnicodeAndSize().

Ohh, I haven't read all related code so I didn't realize that's a leak. But if that is, there is still a wide = PyUnicode_AsWideCharString around the codes I altered.

v2 applies the comments. And another separate change is to change PyUnicode_AsWideCharString to PyUnicode_AsUnicodeAndSize.

You could just reuse to_cleanup instead of to_cleanup2.

> You could just reuse to_cleanup instead of to_cleanup2.

Ah, it would be better to avoid a new variable, but this code is too complex for my head. I don't understand what is to_cleanup in this code path :-)

Hmm, while considering Victor's comment, I find some new:

path->object refers to the original object o, it owns a borrowed reference. But when received a PathLike object, o is assigned the return value of __fspath__ and decrefed at end. So path->object could refer to a already freed object.

And the PyUnicode_AsWideCharString can't be simply replaced with PyUnicode_AsUnicodeAndSize since wo is decrefed and object->wide could then refer to freed memory.

The logic is complex and I get headache reading the code. Did I miss something?

> The logic is complex ...

Yeah, that's why I first proposed to add a dummy to_cleanup2 object.

Another option is to use your first patch.

I don't care much about the exact implementation :-) Maybe Serhiy has
a better understanding of the code and can help to decide on this
issue ;-)

There are many ways to write a solution of the original issue. You can just add a number of "Py_DECREF(bytes)" (path_converter.patch), or add new variable to_cleanup2 (path_converter-v2.patch), or reuse to_cleanup (path_converter-to_cleanup.patch), or clean ups bytes (path_converter-bytes.patch). All these ways look equivalent.

The issue mentioned in msg283754 is more severe. It can be solved by making path->object referring an original argument (but some code checks the type of path->object), or making path->object owning a reference.

It seems to me that path->narrow can be not initialized for str path on Windows.

The new patch is a try to solve the problems mentioned in this thread.

> It can be solved by making path->object referring an original argument (but some code checks the type of path->object), or making path->object owning a reference.

I prefer the later approach. There are codes relying on this behaviour (readlink).

path->length and path->object are set in each way to success_exit (except error_exit). I think these assignment can be moved after success_exit.

path_converter-new-2.patch addresses Serhiy's comments. :-)

path_converter-new-3.patch fixes the wrong return value on successful cases.

The last patch LGTM. Thanks Xiang! At the end this issue has appeared much more complex that it was looked at the start.

New changeset 553eedb8b247 by Xiang Zhang in branch '3.6':
Issue #29034: Fix memory leak and use-after-free in path_converter.
https://hg.python.org/cpython/rev/553eedb8b247

New changeset 08042f0dbb67 by Xiang Zhang in branch 'default':
Issue #29034: Merge 3.6.
https://hg.python.org/cpython/rev/08042f0dbb67

Is 3.5 free from all these bugs?

> Is 3.5 free from all these bugs?

path_converter is much simpler in 3.5 so I think it's free of these bugs. :-)

New changeset 4e3a16bdadae by Serhiy Storchaka in branch '3.6':
Issue #29513: Fixed a reference leak in os.scandir() added in issue #29034.
https://hg.python.org/cpython/rev/4e3a16bdadae

New changeset be85fd4ef41979dbe68262938da12328fb6cfb8c by Serhiy Storchaka in branch '3.6':
Issue #29513: Fixed a reference leak in os.scandir() added in issue #29034.
https://github.com/python/cpython/commit/be85fd4ef41979dbe68262938da12328fb6cfb8c

Does the latest commit fixes a regression introduced by the first fix? Too
bad that we missed the refleak in an issue fixing a memory leak :-/