Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
zhangyangyu opened this issue Dec 21, 2016 · 20 comments
Closed

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

zhangyangyu opened this issue Dec 21, 2016 · 20 comments
Assignees
Labels
3.7 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@zhangyangyu
Copy link
Member

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
    Copy link
    Member Author

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

    @zhangyangyu zhangyangyu added 3.7 only security fixes stdlib Python modules in the Lib dir labels Dec 21, 2016
    @vstinner
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member Author

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

    @zhangyangyu zhangyangyu changed the title refleak in path_converter on error case Fix memory leak in path_converter Dec 21, 2016
    @serhiy-storchaka
    Copy link
    Member

    You could just reuse to_cleanup instead of to_cleanup2.

    @vstinner
    Copy link
    Member

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member Author

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

    @zhangyangyu zhangyangyu changed the title Fix memory leak in path_converter Fix memory leak and use-after-free in path_converter Jan 5, 2017
    @zhangyangyu zhangyangyu added the performance Performance or resource usage label Jan 5, 2017
    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Member Author

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

    @zhangyangyu
    Copy link
    Member Author

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

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jan 8, 2017

    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
    Copy link
    Member

    Is 3.5 free from all these bugs?

    @zhangyangyu
    Copy link
    Member Author

    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
    Copy link
    Mannequin

    python-dev mannequin commented Feb 9, 2017

    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
    Copy link
    Mannequin

    python-dev mannequin commented Feb 9, 2017

    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
    Copy link
    Member

    vstinner commented Feb 9, 2017

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 only security fixes performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants