classification
Title: Fix memory leak and use-after-free in path_converter
Type: resource usage Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: xiang.zhang Nosy List: brett.cannon, python-dev, serhiy.storchaka, steve.dower, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-12-21 10:34 by xiang.zhang, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
path_converter.patch xiang.zhang, 2016-12-21 10:34 review
path_converter-v2.patch xiang.zhang, 2016-12-21 11:20 review
path_converter-to_cleanup.patch serhiy.storchaka, 2016-12-21 15:32 review
path_converter-bytes.patch serhiy.storchaka, 2016-12-21 15:33 review
path_converter-new.patch xiang.zhang, 2017-01-05 16:10 review
path_converter-new-2.patch xiang.zhang, 2017-01-06 05:33 review
path_converter-new-3.patch xiang.zhang, 2017-01-08 08:12 review
path_converter-new-4.patch xiang.zhang, 2017-01-08 08:42 fixed wrong constant value on Windows review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (20)
msg283736 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-12-21 10:34
It looks like the bytes variable should be Py_DECREFed on error cases as the patch shows.
msg283737 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-21 10:44
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().
msg283738 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-12-21 10:53
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.
msg283742 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-12-21 11:20
v2 applies the comments. And another separate change is to change PyUnicode_AsWideCharString to PyUnicode_AsUnicodeAndSize.
msg283743 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-21 11:46
You could just reuse to_cleanup instead of to_cleanup2.
msg283746 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-21 11:50
> 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 :-)
msg283754 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-12-21 14:19
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?
msg283757 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-21 14:52
> 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 ;-)
msg283760 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-21 15:32
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.
msg284759 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-01-05 16:10
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).
msg284778 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-05 22:28
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.
msg284796 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-01-06 05:33
path_converter-new-2.patch addresses Serhiy's comments. :-)
msg284966 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-01-08 08:12
path_converter-new-3.patch fixes the wrong return value on successful cases.
msg284969 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-08 08:52
The last patch LGTM. Thanks Xiang! At the end this issue has appeared much more complex that it was looked at the start.
msg284993 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-08 15:31
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
msg284997 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-01-08 16:59
Is 3.5 free from all these bugs?
msg284999 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2017-01-08 17:13
> 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. :-)
msg287446 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-09 18:07
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
msg287452 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-09 19:00
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
msg287458 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-02-09 21:19
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 :-/
History
Date User Action Args
2017-03-31 16:36:12dstufftsetpull_requests: + pull_request884
2017-02-09 21:19:24vstinnersetmessages: + msg287458
2017-02-09 19:00:22python-devsetmessages: + msg287452
2017-02-09 18:07:38python-devsetmessages: + msg287446
2017-01-08 17:13:47xiang.zhangsetstatus: open -> closed
resolution: fixed
messages: + msg284999

stage: commit review -> resolved
2017-01-08 16:59:31serhiy.storchakasetmessages: + msg284997
2017-01-08 15:31:07python-devsetnosy: + python-dev
messages: + msg284993
2017-01-08 08:52:43serhiy.storchakasetassignee: xiang.zhang
messages: + msg284969
stage: patch review -> commit review
2017-01-08 08:42:32xiang.zhangsetfiles: + path_converter-new-4.patch
2017-01-08 08:12:42xiang.zhangsetfiles: + path_converter-new-3.patch

messages: + msg284966
2017-01-06 05:33:03xiang.zhangsetfiles: + path_converter-new-2.patch

messages: + msg284796
2017-01-05 22:28:58serhiy.storchakasetmessages: + msg284778
2017-01-05 16:10:44xiang.zhangsetfiles: + path_converter-new.patch
type: resource usage
messages: + msg284759

title: Fix memory leak in path_converter -> Fix memory leak and use-after-free in path_converter
2016-12-21 15:33:12serhiy.storchakasetfiles: + path_converter-bytes.patch
2016-12-21 15:32:54serhiy.storchakasetfiles: + path_converter-to_cleanup.patch
nosy: + brett.cannon
messages: + msg283760

2016-12-21 14:52:43vstinnersetmessages: + msg283757
2016-12-21 14:19:07xiang.zhangsetmessages: + msg283754
2016-12-21 11:50:16vstinnersetmessages: + msg283746
2016-12-21 11:46:44serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg283743
2016-12-21 11:20:03xiang.zhangsetfiles: + path_converter-v2.patch

messages: + msg283742
title: refleak in path_converter on error case -> Fix memory leak in path_converter
2016-12-21 10:53:44xiang.zhangsetmessages: + msg283738
2016-12-21 10:44:13vstinnersetmessages: + msg283737
2016-12-21 10:34:58xiang.zhangcreate