msg283736 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2016-12-21 11:46 |
You could just reuse to_cleanup instead of to_cleanup2.
|
msg283746 - (view) |
Author: STINNER Victor (vstinner) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
Date: 2017-01-06 05:33 |
path_converter-new-2.patch addresses Serhiy's comments. :-)
|
msg284966 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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) *  |
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)  |
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) *  |
Date: 2017-01-08 16:59 |
Is 3.5 free from all these bugs?
|
msg284999 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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)  |
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)  |
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) *  |
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 :-/
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:40 | admin | set | github: 73220 |
2017-03-31 16:36:12 | dstufft | set | pull_requests:
+ pull_request884 |
2017-02-09 21:19:24 | vstinner | set | messages:
+ msg287458 |
2017-02-09 19:00:22 | python-dev | set | messages:
+ msg287452 |
2017-02-09 18:07:38 | python-dev | set | messages:
+ msg287446 |
2017-01-08 17:13:47 | xiang.zhang | set | status: open -> closed resolution: fixed messages:
+ msg284999
stage: commit review -> resolved |
2017-01-08 16:59:31 | serhiy.storchaka | set | messages:
+ msg284997 |
2017-01-08 15:31:07 | python-dev | set | nosy:
+ python-dev messages:
+ msg284993
|
2017-01-08 08:52:43 | serhiy.storchaka | set | assignee: xiang.zhang messages:
+ msg284969 stage: patch review -> commit review |
2017-01-08 08:42:32 | xiang.zhang | set | files:
+ path_converter-new-4.patch |
2017-01-08 08:12:42 | xiang.zhang | set | files:
+ path_converter-new-3.patch
messages:
+ msg284966 |
2017-01-06 05:33:03 | xiang.zhang | set | files:
+ path_converter-new-2.patch
messages:
+ msg284796 |
2017-01-05 22:28:58 | serhiy.storchaka | set | messages:
+ msg284778 |
2017-01-05 16:10:44 | xiang.zhang | set | files:
+ 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:12 | serhiy.storchaka | set | files:
+ path_converter-bytes.patch |
2016-12-21 15:32:54 | serhiy.storchaka | set | files:
+ path_converter-to_cleanup.patch nosy:
+ brett.cannon messages:
+ msg283760
|
2016-12-21 14:52:43 | vstinner | set | messages:
+ msg283757 |
2016-12-21 14:19:07 | xiang.zhang | set | messages:
+ msg283754 |
2016-12-21 11:50:16 | vstinner | set | messages:
+ msg283746 |
2016-12-21 11:46:44 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg283743
|
2016-12-21 11:20:03 | xiang.zhang | set | files:
+ 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:44 | xiang.zhang | set | messages:
+ msg283738 |
2016-12-21 10:44:13 | vstinner | set | messages:
+ msg283737 |
2016-12-21 10:34:58 | xiang.zhang | create | |