Fix memory leak and use-after-free in path_converter

[zhangyangyu]

BPO	29034
Nosy	@brettcannon, @vstinner, @serhiy-storchaka, @zooba, @zhangyangyu
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	path_converter.patch path_converter-v2.patch path_converter-to_cleanup.patch path_converter-bytes.patch path_converter-new.patch path_converter-new-2.patch path_converter-new-3.patch path_converter-new-4.patch: fixed wrong constant value on Windows

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/zhangyangyu'
closed_at = <Date 2017-01-08.17:13:47.912>
created_at = <Date 2016-12-21.10:34:58.647>
labels = ['3.7', 'library', 'performance']
title = 'Fix memory leak and use-after-free in path_converter'
updated_at = <Date 2017-03-31.16:36:12.970>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:12.970>
actor = 'dstufft'
assignee = 'xiang.zhang'
closed = True
closed_date = <Date 2017-01-08.17:13:47.912>
closer = 'xiang.zhang'
components = ['Library (Lib)']
creation = <Date 2016-12-21.10:34:58.647>
creator = 'xiang.zhang'
dependencies = []
files = ['45982', '45983', '45985', '45986', '46162', '46170', '46206', '46208']
hgrepos = []
issue_num = 29034
keywords = ['patch']
message_count = 20.0
messages = ['283736', '283737', '283738', '283742', '283743', '283746', '283754', '283757', '283760', '284759', '284778', '284796', '284966', '284969', '284993', '284997', '284999', '287446', '287452', '287458']
nosy_count = 6.0
nosy_names = ['brett.cannon', 'vstinner', 'python-dev', 'serhiy.storchaka', 'steve.dower', 'xiang.zhang']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue29034'
versions = ['Python 3.6', 'Python 3.7']

[zhangyangyu]

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

[vstinner]

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 bpo-28200: Replace PyUnicode_AsWideCharString() with
PyUnicode_AsUnicodeAndSize().

[zhangyangyu]

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.

[zhangyangyu]

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

[serhiy-storchaka]

You could just reuse to_cleanup instead of to_cleanup2.

[vstinner]

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

[zhangyangyu]

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?

[vstinner]

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

[serhiy-storchaka]

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.

[zhangyangyu]

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