Fix memory leak and use-after-free in path_converter #73220

zhangyangyu · 2016-12-21T10:34:59Z

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']

The text was updated successfully, but these errors were encountered:

zhangyangyu · 2016-12-21T10:34:58Z

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

vstinner · 2016-12-21T10:44:13Z

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 · 2016-12-21T10:53:45Z

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 · 2016-12-21T11:20:03Z

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

serhiy-storchaka · 2016-12-21T11:46:45Z

You could just reuse to_cleanup instead of to_cleanup2.

vstinner · 2016-12-21T11:50:16Z

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 · 2016-12-21T14:19:08Z

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 · 2016-12-21T14:52:44Z

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 · 2016-12-21T15:32:55Z

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 · 2017-01-05T16:10:44Z

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

serhiy-storchaka · 2017-01-05T22:28:58Z

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.

zhangyangyu · 2017-01-06T05:33:01Z

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

zhangyangyu · 2017-01-08T08:12:42Z

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

serhiy-storchaka · 2017-01-08T08:52:43Z

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

python-dev · 2017-01-08T15:31:08Z

New changeset 553eedb8b247 by Xiang Zhang in branch '3.6':
Issue bpo-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 bpo-29034: Merge 3.6.
https://hg.python.org/cpython/rev/08042f0dbb67

serhiy-storchaka · 2017-01-08T16:59:32Z

Is 3.5 free from all these bugs?

zhangyangyu · 2017-01-08T17:13:48Z

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

python-dev · 2017-02-09T18:07:38Z

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

python-dev · 2017-02-09T19:00:22Z

New changeset be85fd4ef41979dbe68262938da12328fb6cfb8c by Serhiy Storchaka in branch '3.6':
Issue bpo-29513: Fixed a reference leak in os.scandir() added in issue bpo-29034.
be85fd4

vstinner · 2017-02-09T21:19:25Z

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

zhangyangyu added 3.7 (EOL) end of life stdlib Python modules in the Lib dir labels Dec 21, 2016

zhangyangyu changed the title ~~refleak in path_converter on error case~~ Fix memory leak in path_converter Dec 21, 2016

zhangyangyu changed the title ~~Fix memory leak in path_converter~~ Fix memory leak and use-after-free in path_converter Jan 5, 2017

zhangyangyu added the performance Performance or resource usage label Jan 5, 2017

serhiy-storchaka assigned zhangyangyu Jan 8, 2017

zhangyangyu closed this as completed Jan 8, 2017

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak and use-after-free in path_converter #73220

Fix memory leak and use-after-free in path_converter #73220

zhangyangyu commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

vstinner commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

serhiy-storchaka commented Dec 21, 2016

vstinner commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

vstinner commented Dec 21, 2016

serhiy-storchaka commented Dec 21, 2016

zhangyangyu commented Jan 5, 2017

serhiy-storchaka commented Jan 5, 2017

zhangyangyu commented Jan 6, 2017

zhangyangyu commented Jan 8, 2017

serhiy-storchaka commented Jan 8, 2017

python-dev mannequin commented Jan 8, 2017

serhiy-storchaka commented Jan 8, 2017

zhangyangyu commented Jan 8, 2017

python-dev mannequin commented Feb 9, 2017

python-dev mannequin commented Feb 9, 2017

vstinner commented Feb 9, 2017

Fix memory leak and use-after-free in path_converter #73220

Fix memory leak and use-after-free in path_converter #73220

Comments

zhangyangyu commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

vstinner commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

serhiy-storchaka commented Dec 21, 2016

vstinner commented Dec 21, 2016

zhangyangyu commented Dec 21, 2016

vstinner commented Dec 21, 2016

serhiy-storchaka commented Dec 21, 2016

zhangyangyu commented Jan 5, 2017

serhiy-storchaka commented Jan 5, 2017

zhangyangyu commented Jan 6, 2017

zhangyangyu commented Jan 8, 2017

serhiy-storchaka commented Jan 8, 2017

python-dev mannequin commented Jan 8, 2017

serhiy-storchaka commented Jan 8, 2017

zhangyangyu commented Jan 8, 2017

python-dev mannequin commented Feb 9, 2017

python-dev mannequin commented Feb 9, 2017

vstinner commented Feb 9, 2017